diff mbox series

libgcc/crtstuff.c: Fix incorrect alignment of entries in CRT data structures (PR target/91306)

Message ID 20190821143420.42e074d6@jozef-kubuntu
State New
Headers show
Series libgcc/crtstuff.c: Fix incorrect alignment of entries in CRT data structures (PR target/91306) | expand

Commit Message

Jozef Lawrynowicz Aug. 21, 2019, 1:34 p.m. UTC 
As described in PR target/91306, the alignment of entries in CRT data structures
as set in libgcc/crtstuff.c can be too large for msp430-elf.

crtstuff.c assumes the correct alignment of entries with a type of (void *) or
int32 is the "sizeof" that type.

However, for msp430-elf in the large memory model, pointer size is 4 bytes, but
they only need to be 2-byte aligned. Likewise for int32.

This is causing problems for __frame_dummy_init_array_entry, which gets 
inserted into .init_array.
It is being forced into a 4 byte alignment, which can result in a gap between
__frame_dummy_init_array_entry and the previous entry (or the start of the
table). So when the startup code comes to run through the entries
in .init_array, it interprets 2-bytes of padding 0s as part of a function
address, resulting in a function call to the incorrect location.

This issue has only recently appeared because msp430-elf was just transitioned
to use init/fini_array by default, as mandated by the mspabi.

The other CRT array entries in crtstuff.c are not used for msp430-elf, so aren't
causing problems.

As suggested in the PR, I tried changing the alignment of the array entry from
sizeof(func_ptr) to __alignof__(func_ptr), and this fixed the issue. In the
attached patch I also updated the other uses of sizeof in "aligned" attributes
to use __alignof__ instead.

I understand the alignment to sizeof(func_ptr) was originally added to fix an
issue on mips64 (r182066
https://gcc.gnu.org/ml/gcc-patches/2011-12/msg00393.html), however I do
not have access to a mips64 platform for which to bootstrap on. I tried building
a cross-compiler with the "aligned" attributes removed to see if I could
reproduce the failure, but the build completed successfully.
I built the mips64 cross-compiler with my patch applied and it
also completed successfully.

I successfully regtested the patch for msp430-elf. It fixes many execution
failures for msp430-elf/-mlarge.
I also successfully bootstrapped and regtested x86_64-pc-linux-gnu (g++ gcc
gfortran libatomic libgomp libitm libstdc++ objc).

According to the mips eabi (is this the right one?
http://www.cygwin.com/ml/binutils/2003-06/msg00436.html), pointers have 64-bit
size and alignment in 64-bit mode, so I am assuming using __alignof__ instead of
sizeof isn't going to cause problems.

Ok for trunk?

Comments

Jeff Law Aug. 22, 2019, 9:30 p.m. UTC | #1 
On 8/21/19 7:34 AM, Jozef Lawrynowicz wrote:
> As described in PR target/91306, the alignment of entries in CRT data structures
> as set in libgcc/crtstuff.c can be too large for msp430-elf.
> 
> crtstuff.c assumes the correct alignment of entries with a type of (void *) or
> int32 is the "sizeof" that type.
> 
> However, for msp430-elf in the large memory model, pointer size is 4 bytes, but
> they only need to be 2-byte aligned. Likewise for int32.
> 
> This is causing problems for __frame_dummy_init_array_entry, which gets 
> inserted into .init_array.
> It is being forced into a 4 byte alignment, which can result in a gap between
> __frame_dummy_init_array_entry and the previous entry (or the start of the
> table). So when the startup code comes to run through the entries
> in .init_array, it interprets 2-bytes of padding 0s as part of a function
> address, resulting in a function call to the incorrect location.
> 
> This issue has only recently appeared because msp430-elf was just transitioned
> to use init/fini_array by default, as mandated by the mspabi.
> 
> The other CRT array entries in crtstuff.c are not used for msp430-elf, so aren't
> causing problems.
> 
> As suggested in the PR, I tried changing the alignment of the array entry from
> sizeof(func_ptr) to __alignof__(func_ptr), and this fixed the issue. In the
> attached patch I also updated the other uses of sizeof in "aligned" attributes
> to use __alignof__ instead.
> 
> I understand the alignment to sizeof(func_ptr) was originally added to fix an
> issue on mips64 (r182066
> https://gcc.gnu.org/ml/gcc-patches/2011-12/msg00393.html), however I do
> not have access to a mips64 platform for which to bootstrap on. I tried building
> a cross-compiler with the "aligned" attributes removed to see if I could
> reproduce the failure, but the build completed successfully.
> I built the mips64 cross-compiler with my patch applied and it
> also completed successfully.
> 
> I successfully regtested the patch for msp430-elf. It fixes many execution
> failures for msp430-elf/-mlarge.
> I also successfully bootstrapped and regtested x86_64-pc-linux-gnu (g++ gcc
> gfortran libatomic libgomp libitm libstdc++ objc).
> 
> According to the mips eabi (is this the right one?
> http://www.cygwin.com/ml/binutils/2003-06/msg00436.html), pointers have 64-bit
> size and alignment in 64-bit mode, so I am assuming using __alignof__ instead of
> sizeof isn't going to cause problems.
> 
> Ok for trunk?
> 
> 
> 0001-libgcc-crtstuff.c-Fix-incorrect-alignment-of-entries.patch
> 
> From 636ffa0c4f15047dc27c829d9a3c8fea9ad5d635 Mon Sep 17 00:00:00 2001
> From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
> Date: Thu, 15 Aug 2019 14:17:25 +0100
> Subject: [PATCH] libgcc/crtstuff.c: Fix incorrect alignment of entries in CRT
>  data structures
> 
> libgcc/ChangeLog:
> 
> 2019-08-21  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
> 
> 	* crtstuff.c (__CTOR_LIST__): Align to the "__alignof__" the array
> 	element type, instead of "sizeof" the element type.
> 	(__DTOR_LIST__): Likewise.
> 	(__TMC_LIST__): Likewise.
> 	(__do_global_dtors_aux_fini_array_entry): Likewise.
> 	(__frame_dummy_init_array_entry): Likewise.
> 	(__CTOR_END__): Likewise.
> 	(__DTOR_END__): Likweise.
> 	(__FRAME_END__): Likewise.
> 	(__TMC_END__): Likewise.
OK
jeff
diff mbox series

Patch

From 636ffa0c4f15047dc27c829d9a3c8fea9ad5d635 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Thu, 15 Aug 2019 14:17:25 +0100
Subject: [PATCH] libgcc/crtstuff.c: Fix incorrect alignment of entries in CRT
 data structures

libgcc/ChangeLog:

2019-08-21  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	* crtstuff.c (__CTOR_LIST__): Align to the "__alignof__" the array
	element type, instead of "sizeof" the element type.
	(__DTOR_LIST__): Likewise.
	(__TMC_LIST__): Likewise.
	(__do_global_dtors_aux_fini_array_entry): Likewise.
	(__frame_dummy_init_array_entry): Likewise.
	(__CTOR_END__): Likewise.
	(__DTOR_END__): Likweise.
	(__FRAME_END__): Likewise.
	(__TMC_END__): Likewise.
---
 libgcc/crtstuff.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/libgcc/crtstuff.c b/libgcc/crtstuff.c
index 4927a9f8977..c8a8e2c87a5 100644
--- a/libgcc/crtstuff.c
+++ b/libgcc/crtstuff.c
@@ -233,11 +233,11 @@  CTOR_LIST_BEGIN;
 static func_ptr force_to_data[1] __attribute__ ((__used__)) = { };
 asm (__LIBGCC_CTORS_SECTION_ASM_OP__);
 STATIC func_ptr __CTOR_LIST__[1]
-  __attribute__ ((__used__, aligned(sizeof(func_ptr))))
+  __attribute__ ((__used__, aligned(__alignof__(func_ptr))))
   = { (func_ptr) (-1) };
 #else
 STATIC func_ptr __CTOR_LIST__[1]
-  __attribute__ ((__used__, section(".ctors"), aligned(sizeof(func_ptr))))
+  __attribute__ ((__used__, section(".ctors"), aligned(__alignof__(func_ptr))))
   = { (func_ptr) (-1) };
 #endif /* __CTOR_LIST__ alternatives */
 
@@ -246,11 +246,11 @@  DTOR_LIST_BEGIN;
 #elif defined(__LIBGCC_DTORS_SECTION_ASM_OP__)
 asm (__LIBGCC_DTORS_SECTION_ASM_OP__);
 STATIC func_ptr __DTOR_LIST__[1]
-  __attribute__ ((aligned(sizeof(func_ptr))))
+  __attribute__ ((aligned(__alignof__(func_ptr))))
   = { (func_ptr) (-1) };
 #else
 STATIC func_ptr __DTOR_LIST__[1]
-  __attribute__((section(".dtors"), aligned(sizeof(func_ptr))))
+  __attribute__((section(".dtors"), aligned(__alignof__(func_ptr))))
   = { (func_ptr) (-1) };
 #endif /* __DTOR_LIST__ alternatives */
 #endif /* USE_INITFINI_ARRAY */
@@ -265,7 +265,7 @@  STATIC EH_FRAME_SECTION_CONST char __EH_FRAME_BEGIN__[]
 
 #if USE_TM_CLONE_REGISTRY
 STATIC func_ptr __TMC_LIST__[]
-  __attribute__((used, section(".tm_clone_table"), aligned(sizeof(void*))))
+  __attribute__((used, section(".tm_clone_table"), aligned(__alignof__(void*))))
   = { };
 # ifdef HAVE_GAS_HIDDEN
 extern func_ptr __TMC_END__[] __attribute__((__visibility__ ("hidden")));
@@ -430,8 +430,8 @@  __do_global_dtors_aux (void)
 CRT_CALL_STATIC_FUNCTION (FINI_SECTION_ASM_OP, __do_global_dtors_aux)
 #elif defined (FINI_ARRAY_SECTION_ASM_OP)
 static func_ptr __do_global_dtors_aux_fini_array_entry[]
-  __attribute__ ((__used__, section(".fini_array"), aligned(sizeof(func_ptr))))
-  = { __do_global_dtors_aux };
+  __attribute__ ((__used__, section(".fini_array"),
+		  aligned(__alignof__(func_ptr)))) = { __do_global_dtors_aux };
 #else /* !FINI_SECTION_ASM_OP && !FINI_ARRAY_SECTION_ASM_OP */
 static void __attribute__((used))
 __do_global_dtors_aux_1 (void)
@@ -474,8 +474,8 @@  frame_dummy (void)
 CRT_CALL_STATIC_FUNCTION (__LIBGCC_INIT_SECTION_ASM_OP__, frame_dummy)
 #else /* defined(__LIBGCC_INIT_SECTION_ASM_OP__) */
 static func_ptr __frame_dummy_init_array_entry[]
-  __attribute__ ((__used__, section(".init_array"), aligned(sizeof(func_ptr))))
-  = { frame_dummy };
+  __attribute__ ((__used__, section(".init_array"),
+		  aligned(__alignof__(func_ptr)))) = { frame_dummy };
 #endif /* !defined(__LIBGCC_INIT_SECTION_ASM_OP__) */
 #endif /* USE_EH_FRAME_REGISTRY || USE_TM_CLONE_REGISTRY */
 
@@ -588,11 +588,11 @@  CTOR_LIST_END;
 static func_ptr force_to_data[1] __attribute__ ((__used__)) = { };
 asm (__LIBGCC_CTORS_SECTION_ASM_OP__);
 STATIC func_ptr __CTOR_END__[1]
-  __attribute__((aligned(sizeof(func_ptr))))
+  __attribute__((aligned(__alignof__(func_ptr))))
   = { (func_ptr) 0 };
 #else
 STATIC func_ptr __CTOR_END__[1]
-  __attribute__((section(".ctors"), aligned(sizeof(func_ptr))))
+  __attribute__((section(".ctors"), aligned(__alignof__(func_ptr))))
   = { (func_ptr) 0 };
 #endif
 
@@ -607,16 +607,16 @@  func_ptr __DTOR_END__[1]
 #ifndef __LIBGCC_DTORS_SECTION_ASM_OP__
 		  section(".dtors"),
 #endif
-		  aligned(sizeof(func_ptr)), visibility ("hidden")))
+		  aligned(__alignof__(func_ptr)), visibility ("hidden")))
   = { (func_ptr) 0 };
 #elif defined(__LIBGCC_DTORS_SECTION_ASM_OP__)
 asm (__LIBGCC_DTORS_SECTION_ASM_OP__);
 STATIC func_ptr __DTOR_END__[1]
-  __attribute__ ((used, aligned(sizeof(func_ptr))))
+  __attribute__ ((used, aligned(__alignof__(func_ptr))))
   = { (func_ptr) 0 };
 #else
 STATIC func_ptr __DTOR_END__[1]
-  __attribute__((used, section(".dtors"), aligned(sizeof(func_ptr))))
+  __attribute__((used, section(".dtors"), aligned(__alignof__(func_ptr))))
   = { (func_ptr) 0 };
 #endif
 #endif /* USE_INITFINI_ARRAY */
@@ -635,7 +635,7 @@  typedef short int32;
 # endif
 STATIC EH_FRAME_SECTION_CONST int32 __FRAME_END__[]
      __attribute__ ((used, section(__LIBGCC_EH_FRAME_SECTION_NAME__),
-		     aligned(sizeof(int32))))
+		     aligned(__alignof__(int32))))
      = { 0 };
 #endif /* __LIBGCC_EH_FRAME_SECTION_NAME__ */
 
@@ -644,7 +644,8 @@  STATIC EH_FRAME_SECTION_CONST int32 __FRAME_END__[]
 static
 # endif
 func_ptr __TMC_END__[]
-  __attribute__((used, section(".tm_clone_table"), aligned(sizeof(void *))))
+  __attribute__((used, section(".tm_clone_table"),
+		 aligned(__alignof__(void *))))
 # ifdef HAVE_GAS_HIDDEN
   __attribute__((__visibility__ ("hidden"))) = { };
 # else
-- 
2.17.1