diff mbox series

Mitigation for PR target/88469 on arm-based systems bootstrapping with gcc-6/7/8

Message ID 09d6d715-5fe0-e120-3f28-49297e3535a0@arm.com
State New
Headers show
Series Mitigation for PR target/88469 on arm-based systems bootstrapping with gcc-6/7/8 | expand

Commit Message

Richard Earnshaw (lists) Jan. 22, 2019, 2:43 p.m. UTC 
This patch, for gcc 8/9 is a mitigation patch for PR target/88469 where
gcc-6/7/8 miscompile a structure whose alignment is dominated by a
64-bit bitfield member.  Since the PCS rules for such a type must ignore
any overalignment of the base type we cannot address this by simply
adding a larger alignment to the class.  The simplest fix, therefore, is
to insert a dummy field that has 64-bit alignment.  Although that field
is never used, it does fix the overall alignment of the class at the
expense of adding an extra dword of data on ARM systems (I've bounded
the range of GCC versions that will lead to this mitigation, so only a
stage-1 gcc-9 will see the impact of this change - though gcc-8 will see
this in full).

OK for trunk/gcc-8?

	PR target/88469
	* profile-count.h (profile_count): Add dummy file with 64-bit alignment
	on arm-based systems using gcc-6/7/8.

Comments

Richard Biener Jan. 22, 2019, 3:20 p.m. UTC | #1 
On Tue, 22 Jan 2019, Richard Earnshaw (lists) wrote:

> This patch, for gcc 8/9 is a mitigation patch for PR target/88469 where

> gcc-6/7/8 miscompile a structure whose alignment is dominated by a

> 64-bit bitfield member.  Since the PCS rules for such a type must ignore

> any overalignment of the base type we cannot address this by simply

> adding a larger alignment to the class.  The simplest fix, therefore, is

> to insert a dummy field that has 64-bit alignment.  Although that field

> is never used, it does fix the overall alignment of the class at the

> expense of adding an extra dword of data on ARM systems (I've bounded

> the range of GCC versions that will lead to this mitigation, so only a

> stage-1 gcc-9 will see the impact of this change - though gcc-8 will see

> this in full).

> 

> OK for trunk/gcc-8?


Can you place some aligned attribute on the struct instead?

> 	PR target/88469

> 	* profile-count.h (profile_count): Add dummy file with 64-bit alignment

> 	on arm-based systems using gcc-6/7/8.

> 

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Richard Earnshaw (lists) Jan. 22, 2019, 3:44 p.m. UTC | #2 
On 22/01/2019 15:20, Richard Biener wrote:
> On Tue, 22 Jan 2019, Richard Earnshaw (lists) wrote:

> 

>> This patch, for gcc 8/9 is a mitigation patch for PR target/88469 where

>> gcc-6/7/8 miscompile a structure whose alignment is dominated by a

>> 64-bit bitfield member.  Since the PCS rules for such a type must ignore

>> any overalignment of the base type we cannot address this by simply

>> adding a larger alignment to the class.  The simplest fix, therefore, is

>> to insert a dummy field that has 64-bit alignment.  Although that field

>> is never used, it does fix the overall alignment of the class at the

>> expense of adding an extra dword of data on ARM systems (I've bounded

>> the range of GCC versions that will lead to this mitigation, so only a

>> stage-1 gcc-9 will see the impact of this change - though gcc-8 will see

>> this in full).

>>

>> OK for trunk/gcc-8?

> 

> Can you place some aligned attribute on the struct instead?

> 

No, the PCS says that overalignment on the struct should be ignored for
the purposes of parameter passing, so it has no effect.

R.

>> 	PR target/88469

>> 	* profile-count.h (profile_count): Add dummy file with 64-bit alignment

>> 	on arm-based systems using gcc-6/7/8.

>>

>>

>
Jakub Jelinek Jan. 22, 2019, 3:46 p.m. UTC | #3 
On Tue, Jan 22, 2019 at 02:43:38PM +0000, Richard Earnshaw (lists) wrote:
> 	PR target/88469

> 	* profile-count.h (profile_count): Add dummy file with 64-bit alignment

> 	on arm-based systems using gcc-6/7/8.

> 


> diff --git a/gcc/profile-count.h b/gcc/profile-count.h

> index c83fa3beb8f..ddfda2cddf4 100644

> --- a/gcc/profile-count.h

> +++ b/gcc/profile-count.h

> @@ -645,6 +645,12 @@ private:

>  

>    uint64_t m_val : n_bits;

>    enum profile_quality m_quality : 3;

> +#if defined (__arm__) && (__GNUC__ >= 6 && __GNUC__ <= 8)

> +  /* Work-around for PR88469.  A bug in the gcc-6/7/8 PCS layout code

> +     incorrectly detects the alignment of a structure where the only

> +     64-bit aligned element is a bit-field.  */

> +  uint64_t m_force_alignment;

> +#endif


Adding another member is very costly.
Can't you do something like:
#if defined (__arm__) && (__GNUC__ >= 6 && __GNUC__ <= 8)
#define UINT64_BITFIELD_ALIGN \
  __attribute__((__aligned__ (__alignof__ (uint64_t))))
#else
#define UINT64_BITFIELD_ALIGN
#endif
and use
  uint64_t m_val UINT64_BITFIELD_ALIGN : n_bits;
?

	Jakub
Richard Earnshaw (lists) Jan. 23, 2019, 9:34 a.m. UTC | #4 
On 22/01/2019 15:46, Jakub Jelinek wrote:
> On Tue, Jan 22, 2019 at 02:43:38PM +0000, Richard Earnshaw (lists) wrote:

>> 	PR target/88469

>> 	* profile-count.h (profile_count): Add dummy file with 64-bit alignment

>> 	on arm-based systems using gcc-6/7/8.

>>

> 

>> diff --git a/gcc/profile-count.h b/gcc/profile-count.h

>> index c83fa3beb8f..ddfda2cddf4 100644

>> --- a/gcc/profile-count.h

>> +++ b/gcc/profile-count.h

>> @@ -645,6 +645,12 @@ private:

>>  

>>    uint64_t m_val : n_bits;

>>    enum profile_quality m_quality : 3;

>> +#if defined (__arm__) && (__GNUC__ >= 6 && __GNUC__ <= 8)

>> +  /* Work-around for PR88469.  A bug in the gcc-6/7/8 PCS layout code

>> +     incorrectly detects the alignment of a structure where the only

>> +     64-bit aligned element is a bit-field.  */

>> +  uint64_t m_force_alignment;

>> +#endif

> 

> Adding another member is very costly.

> Can't you do something like:

> #if defined (__arm__) && (__GNUC__ >= 6 && __GNUC__ <= 8)

> #define UINT64_BITFIELD_ALIGN \

>   __attribute__((__aligned__ (__alignof__ (uint64_t))))

> #else

> #define UINT64_BITFIELD_ALIGN

> #endif

> and use

>   uint64_t m_val UINT64_BITFIELD_ALIGN : n_bits;

> ?

> 

> 	Jakub

> 


Close.  The alignment has to come before the m_val...


PR target/88469
 	* profile-count.h (profile_count): Force alignment of
	m_val when building on Arm-based systems with gcc-6/7/8.

OK for trunk/gcc-8?

R.
diff --git a/gcc/profile-count.h b/gcc/profile-count.h
index 06564ddf4bd..63076476400 100644
--- a/gcc/profile-count.h
+++ b/gcc/profile-count.h
@@ -649,9 +649,17 @@ public:
 private:
   static const uint64_t uninitialized_count = ((uint64_t) 1 << n_bits) - 1;
 
-  uint64_t m_val : n_bits;
+#if defined (__arm__) && (__GNUC__ >= 6 && __GNUC__ <= 8)
+  /* Work-around for PR88469.  A bug in the gcc-7/8 PCS layout code
+     incorrectly detects the alignment of a structure where the only
+     64-bit aligned object is a bit-field.  We force the alignment
+     of the entire field to mitigate this.  */
+#define UINT64_BITFIELD_ALIGN __attribute__((aligned(8)))
+#else
+#define UINT64_BITFIELD_ALIGN
+#endif
+  uint64_t UINT64_BITFIELD_ALIGN m_val : n_bits;
   enum profile_quality m_quality : 3;
-
   /* Return true if both values can meaningfully appear in single function
      body.  We have either all counters in function local or global, otherwise
      operations between them are not really defined well.  */
Richard Biener Jan. 24, 2019, 10:50 a.m. UTC | #5 
On Wed, 23 Jan 2019, Richard Earnshaw (lists) wrote:

> On 22/01/2019 15:46, Jakub Jelinek wrote:

> > On Tue, Jan 22, 2019 at 02:43:38PM +0000, Richard Earnshaw (lists) wrote:

> >> 	PR target/88469

> >> 	* profile-count.h (profile_count): Add dummy file with 64-bit alignment

> >> 	on arm-based systems using gcc-6/7/8.

> >>

> > 

> >> diff --git a/gcc/profile-count.h b/gcc/profile-count.h

> >> index c83fa3beb8f..ddfda2cddf4 100644

> >> --- a/gcc/profile-count.h

> >> +++ b/gcc/profile-count.h

> >> @@ -645,6 +645,12 @@ private:

> >>  

> >>    uint64_t m_val : n_bits;

> >>    enum profile_quality m_quality : 3;

> >> +#if defined (__arm__) && (__GNUC__ >= 6 && __GNUC__ <= 8)

> >> +  /* Work-around for PR88469.  A bug in the gcc-6/7/8 PCS layout code

> >> +     incorrectly detects the alignment of a structure where the only

> >> +     64-bit aligned element is a bit-field.  */

> >> +  uint64_t m_force_alignment;

> >> +#endif

> > 

> > Adding another member is very costly.

> > Can't you do something like:

> > #if defined (__arm__) && (__GNUC__ >= 6 && __GNUC__ <= 8)

> > #define UINT64_BITFIELD_ALIGN \

> >   __attribute__((__aligned__ (__alignof__ (uint64_t))))

> > #else

> > #define UINT64_BITFIELD_ALIGN

> > #endif

> > and use

> >   uint64_t m_val UINT64_BITFIELD_ALIGN : n_bits;

> > ?

> > 

> > 	Jakub

> > 

> 

> Close.  The alignment has to come before the m_val...

> 

> 

> PR target/88469

>  	* profile-count.h (profile_count): Force alignment of

> 	m_val when building on Arm-based systems with gcc-6/7/8.

> 

> OK for trunk/gcc-8?


+#define UINT64_BITFIELD_ALIGN
+#endif
+  uint64_t UINT64_BITFIELD_ALIGN m_val : n_bits;
   enum profile_quality m_quality : 3;
-

avoid the stray vertical whitespace change and please #undef
UINT64_BITFIELD_ALIGN after use.

OK with that change.

Richard.
diff mbox series

Patch

diff --git a/gcc/profile-count.h b/gcc/profile-count.h
index c83fa3beb8f..ddfda2cddf4 100644
--- a/gcc/profile-count.h
+++ b/gcc/profile-count.h
@@ -645,6 +645,12 @@  private:
 
   uint64_t m_val : n_bits;
   enum profile_quality m_quality : 3;
+#if defined (__arm__) && (__GNUC__ >= 6 && __GNUC__ <= 8)
+  /* Work-around for PR88469.  A bug in the gcc-6/7/8 PCS layout code
+     incorrectly detects the alignment of a structure where the only
+     64-bit aligned element is a bit-field.  */
+  uint64_t m_force_alignment;
+#endif
 
   /* Return true if both values can meaningfully appear in single function
      body.  We have either all counters in function local or global, otherwise