| Message ID | 27ca627f-3b5c-a1ef-8a2a-80de75599eeb@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
I forgot to mention that this patch is needed for the optional -mthumb patch [1] to bootstrap. [1] https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00735.html Best regards, Thomas On 14/11/16 14:07, Thomas Preudhomme wrote: > Hi, > > Currently arm_feature_set is defined in gcc/config/arm/arm-flags as an array of > 2 unsigned long. However, the flags stored in these two entries are (signed) > int, being combinations of bits set via expression of the form 1 << bitno. This > creates 3 issues: > > 1) undefined behavior when setting the msb (1 << 31) > 2) undefined behavior when storing a flag with msb set (negative int) into one > of the unsigned array entries (positive int) > 3) waste of space since the top 32 bits of each entry is not used > > This patch changes the definition of FL_* macro to be unsigned int by using the > form 1U << bitno instead and changes the definition of arm_feature_set to be an > array of 2 unsigned (int) entries. > > Bootstrapped on arm-linux-gnueabihf targeting Thumb-2 state. > > Is this ok for trunk? > > Best regards, > > Thomas
On 14/11/16 14:07, Thomas Preudhomme wrote: > Hi, > > Currently arm_feature_set is defined in gcc/config/arm/arm-flags as an array of 2 unsigned long. However, the flags stored in these two entries are (signed) int, being combinations of bits set via expression of the form 1 << bitno. This > creates 3 issues: > > 1) undefined behavior when setting the msb (1 << 31) > 2) undefined behavior when storing a flag with msb set (negative int) into one of the unsigned array entries (positive int) > 3) waste of space since the top 32 bits of each entry is not used > > This patch changes the definition of FL_* macro to be unsigned int by using the form 1U << bitno instead and changes the definition of arm_feature_set to be an array of 2 unsigned (int) entries. > > Bootstrapped on arm-linux-gnueabihf targeting Thumb-2 state. > > Is this ok for trunk? > Ok. Thanks, Kyrill > Best regards, > > Thomas
Hi, On 14 November 2016 at 15:07, Thomas Preudhomme <thomas.preudhomme@foss.arm.com> wrote: > Hi, > > Currently arm_feature_set is defined in gcc/config/arm/arm-flags as an array > of 2 unsigned long. However, the flags stored in these two entries are > (signed) int, being combinations of bits set via expression of the form 1 << > bitno. This creates 3 issues: > > 1) undefined behavior when setting the msb (1 << 31) > 2) undefined behavior when storing a flag with msb set (negative int) into > one of the unsigned array entries (positive int) Just curious: are these problems seen when building with ubsan enabled? > 3) waste of space since the top 32 bits of each entry is not used > > This patch changes the definition of FL_* macro to be unsigned int by using > the form 1U << bitno instead and changes the definition of arm_feature_set > to be an array of 2 unsigned (int) entries. > > Bootstrapped on arm-linux-gnueabihf targeting Thumb-2 state. > > Is this ok for trunk? > > Best regards, > > Thomas
Hi Christophe, No, they were seen when bootstrapping on arm-linux-gnueabihf with the patch to to make -mthumb optional. The patch store flags in an arm_feature_set array and GCC -Wnarrowing complained about the difference of type. Best regards, Thomas On 14/11/16 17:00, Christophe Lyon wrote: > Hi, > > > On 14 November 2016 at 15:07, Thomas Preudhomme > <thomas.preudhomme@foss.arm.com> wrote: >> Hi, >> >> Currently arm_feature_set is defined in gcc/config/arm/arm-flags as an array >> of 2 unsigned long. However, the flags stored in these two entries are >> (signed) int, being combinations of bits set via expression of the form 1 << >> bitno. This creates 3 issues: >> >> 1) undefined behavior when setting the msb (1 << 31) >> 2) undefined behavior when storing a flag with msb set (negative int) into >> one of the unsigned array entries (positive int) > > Just curious: are these problems seen when building with ubsan enabled? > >> 3) waste of space since the top 32 bits of each entry is not used >> >> This patch changes the definition of FL_* macro to be unsigned int by using >> the form 1U << bitno instead and changes the definition of arm_feature_set >> to be an array of 2 unsigned (int) entries. >> >> Bootstrapped on arm-linux-gnueabihf targeting Thumb-2 state. >> >> Is this ok for trunk? >> >> Best regards, >> >> Thomas
My apologize, I realized when trying to apply the patch that I wrote it on top of the optional -mthumb patch instead of the reverse. I'll rebase it to not screw up bisect. Best regards, Thomas On 14/11/16 14:47, Kyrill Tkachov wrote: > > On 14/11/16 14:07, Thomas Preudhomme wrote: >> Hi, >> >> Currently arm_feature_set is defined in gcc/config/arm/arm-flags as an array >> of 2 unsigned long. However, the flags stored in these two entries are >> (signed) int, being combinations of bits set via expression of the form 1 << >> bitno. This creates 3 issues: >> >> 1) undefined behavior when setting the msb (1 << 31) >> 2) undefined behavior when storing a flag with msb set (negative int) into one >> of the unsigned array entries (positive int) >> 3) waste of space since the top 32 bits of each entry is not used >> >> This patch changes the definition of FL_* macro to be unsigned int by using >> the form 1U << bitno instead and changes the definition of arm_feature_set to >> be an array of 2 unsigned (int) entries. >> >> Bootstrapped on arm-linux-gnueabihf targeting Thumb-2 state. >> >> Is this ok for trunk? >> > > Ok. > Thanks, > Kyrill > >> Best regards, >> >> Thomas >
diff --git a/gcc/config/arm/arm-flags.h b/gcc/config/arm/arm-flags.h index 9a5991aa07a229a7741e526c2876e7e0e4749db4..136a36e403dd3207deb91adf8c36e568bc08fd9e 100644 --- a/gcc/config/arm/arm-flags.h +++ b/gcc/config/arm/arm-flags.h @@ -25,49 +25,49 @@ /* Flags used to identify the presence of processor capabilities. */ /* Bit values used to identify processor capabilities. */ -#define FL_NONE (0) /* No flags. */ -#define FL_ANY (0xffffffff) /* All flags. */ -#define FL_CO_PROC (1 << 0) /* Has external co-processor bus */ -#define FL_ARCH3M (1 << 1) /* Extended multiply */ -#define FL_MODE26 (1 << 2) /* 26-bit mode support */ -#define FL_MODE32 (1 << 3) /* 32-bit mode support */ -#define FL_ARCH4 (1 << 4) /* Architecture rel 4 */ -#define FL_ARCH5 (1 << 5) /* Architecture rel 5 */ -#define FL_THUMB (1 << 6) /* Thumb aware */ -#define FL_LDSCHED (1 << 7) /* Load scheduling necessary */ -#define FL_STRONG (1 << 8) /* StrongARM */ -#define FL_ARCH5E (1 << 9) /* DSP extensions to v5 */ -#define FL_XSCALE (1 << 10) /* XScale */ -/* spare (1 << 11) */ -#define FL_ARCH6 (1 << 12) /* Architecture rel 6. Adds +#define FL_NONE (0U) /* No flags. */ +#define FL_ANY (0xffffffffU) /* All flags. */ +#define FL_CO_PROC (1U << 0) /* Has external co-processor bus */ +#define FL_ARCH3M (1U << 1) /* Extended multiply */ +#define FL_MODE26 (1U << 2) /* 26-bit mode support */ +#define FL_MODE32 (1U << 3) /* 32-bit mode support */ +#define FL_ARCH4 (1U << 4) /* Architecture rel 4 */ +#define FL_ARCH5 (1U << 5) /* Architecture rel 5 */ +#define FL_THUMB (1U << 6) /* Thumb aware */ +#define FL_LDSCHED (1U << 7) /* Load scheduling necessary */ +#define FL_STRONG (1U << 8) /* StrongARM */ +#define FL_ARCH5E (1U << 9) /* DSP extensions to v5 */ +#define FL_XSCALE (1U << 10) /* XScale */ +/* spare (1U << 11)*/ +#define FL_ARCH6 (1U << 12) /* Architecture rel 6. Adds media instructions. */ -#define FL_VFPV2 (1 << 13) /* Vector Floating Point V2. */ -#define FL_WBUF (1 << 14) /* Schedule for write buffer ops. +#define FL_VFPV2 (1U << 13) /* Vector Floating Point V2. */ +#define FL_WBUF (1U << 14) /* Schedule for write buffer ops. Note: ARM6 & 7 derivatives only. */ -#define FL_ARCH6K (1 << 15) /* Architecture rel 6 K extensions. */ -#define FL_THUMB2 (1 << 16) /* Thumb-2. */ -#define FL_NOTM (1 << 17) /* Instructions not present in the 'M' +#define FL_ARCH6K (1U << 15) /* Architecture rel 6 K extensions. */ +#define FL_THUMB2 (1U << 16) /* Thumb-2. */ +#define FL_NOTM (1U << 17) /* Instructions not present in the 'M' profile. */ -#define FL_THUMB_DIV (1 << 18) /* Hardware divide (Thumb mode). */ -#define FL_VFPV3 (1 << 19) /* Vector Floating Point V3. */ -#define FL_NEON (1 << 20) /* Neon instructions. */ -#define FL_ARCH7EM (1 << 21) /* Instructions present in the ARMv7E-M +#define FL_THUMB_DIV (1U << 18) /* Hardware divide (Thumb mode). */ +#define FL_VFPV3 (1U << 19) /* Vector Floating Point V3. */ +#define FL_NEON (1U << 20) /* Neon instructions. */ +#define FL_ARCH7EM (1U << 21) /* Instructions present in the ARMv7E-M architecture. */ -#define FL_ARCH7 (1 << 22) /* Architecture 7. */ -#define FL_ARM_DIV (1 << 23) /* Hardware divide (ARM mode). */ -#define FL_ARCH8 (1 << 24) /* Architecture 8. */ -#define FL_CRC32 (1 << 25) /* ARMv8 CRC32 instructions. */ +#define FL_ARCH7 (1U << 22) /* Architecture 7. */ +#define FL_ARM_DIV (1U << 23) /* Hardware divide (ARM mode). */ +#define FL_ARCH8 (1U << 24) /* Architecture 8. */ +#define FL_CRC32 (1U << 25) /* ARMv8 CRC32 instructions. */ -#define FL_SMALLMUL (1 << 26) /* Small multiply supported. */ -#define FL_NO_VOLATILE_CE (1 << 27) /* No volatile memory in IT block. */ +#define FL_SMALLMUL (1U << 26) /* Small multiply supported. */ +#define FL_NO_VOLATILE_CE (1U << 27) /* No volatile memory in IT block. */ -#define FL_IWMMXT (1 << 29) /* XScale v2 or "Intel Wireless MMX technology". */ -#define FL_IWMMXT2 (1 << 30) /* "Intel Wireless MMX2 technology". */ -#define FL_ARCH6KZ (1 << 31) /* ARMv6KZ architecture. */ +#define FL_IWMMXT (1U << 29) /* XScale v2 or "Intel Wireless MMX technology". */ +#define FL_IWMMXT2 (1U << 30) /* "Intel Wireless MMX2 technology". */ +#define FL_ARCH6KZ (1U << 31) /* ARMv6KZ architecture. */ -#define FL2_ARCH8_1 (1 << 0) /* Architecture 8.1. */ -#define FL2_ARCH8_2 (1 << 1) /* Architecture 8.2. */ -#define FL2_FP16INST (1 << 2) /* FP16 Instructions for ARMv8.2 and +#define FL2_ARCH8_1 (1U << 0) /* Architecture 8.1. */ +#define FL2_ARCH8_2 (1U << 1) /* Architecture 8.2. */ +#define FL2_FP16INST (1U << 2) /* FP16 Instructions for ARMv8.2 and later. */ /* Flags that only effect tuning, not available instructions. */ @@ -110,7 +110,7 @@ typedef struct { - unsigned long cpu[2]; + unsigned cpu[2]; } arm_feature_set;