Re: Mitigation for PR target/88469 on arm-based systems bootstrapping with gcc-6/7/8
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 +, 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.
Re: Mitigation for PR target/88469 on arm-based systems bootstrapping with gcc-6/7/8
On 22/01/2019 15:46, Jakub Jelinek wrote: > On Tue, Jan 22, 2019 at 02:43:38PM +, 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. */
Re: Mitigation for PR target/88469 on arm-based systems bootstrapping with gcc-6/7/8
On Tue, Jan 22, 2019 at 02:43:38PM +, 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
Re: Mitigation for PR target/88469 on arm-based systems bootstrapping with gcc-6/7/8
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. >> >> >
Re: Mitigation for PR target/88469 on arm-based systems bootstrapping with gcc-6/7/8
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 SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
