| Message ID | 20180826231332.2491-8-erosca@de.adit-jv.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Import Undefined Behavior Sanitizer | expand |
Hi Eugeniu, On Mon, Aug 27, 2018 at 7:19 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > Fix the following UBSAN report: > ====================================================================== > UBSAN: Undefined behaviour in arch/x86/cpu/lapic.c:73:14 > left shift of 1048575 by 12 places cannot be represented in type 'int' > ====================================================================== > > Steps to reproduce the above: > * echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig > * make ARCH=x86 qemu-x86_defconfig all > * qemu-system-i386 --version > QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31) > * qemu-system-i386 --nographic -bios u-boot.rom > > Fixes: 98568f0fa96b ("x86: Import MSR/MTRR code from Linux") > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > --- > > Changes in v2: > - None. Newly pushed. > --- > arch/x86/include/asm/msr-index.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h > index 9c1dbe61d596..d8b7b8013c74 100644 > --- a/arch/x86/include/asm/msr-index.h > +++ b/arch/x86/include/asm/msr-index.h > @@ -370,7 +370,7 @@ > #define MSR_IA32_APICBASE 0x0000001b > #define MSR_IA32_APICBASE_BSP (1<<8) > #define MSR_IA32_APICBASE_ENABLE (1<<11) > -#define MSR_IA32_APICBASE_BASE (0xfffff<<12) > +#define MSR_IA32_APICBASE_BASE (0xfffffUL << 12) I don't understand why such warnings is emitted: "left shift of 1048575 by 12 places cannot be represented in type 'int'" Compilers don't complain this code and Linux kernel has the same definition here. Regards, Bin
Hi Bin, cc: Masahiro, Andrey On Tue, Aug 28, 2018 at 10:05:51AM +0800, Bin Meng wrote: > Hi Eugeniu, > > On Mon, Aug 27, 2018 at 7:19 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > > > Fix the following UBSAN report: > > ====================================================================== > > UBSAN: Undefined behaviour in arch/x86/cpu/lapic.c:73:14 > > left shift of 1048575 by 12 places cannot be represented in type 'int' > > ====================================================================== > > > > Steps to reproduce the above: > > * echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig > > * make ARCH=x86 qemu-x86_defconfig all > > * qemu-system-i386 --version > > QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31) > > * qemu-system-i386 --nographic -bios u-boot.rom > > > > Fixes: 98568f0fa96b ("x86: Import MSR/MTRR code from Linux") > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > --- > > > > Changes in v2: > > - None. Newly pushed. > > --- > > arch/x86/include/asm/msr-index.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h > > index 9c1dbe61d596..d8b7b8013c74 100644 > > --- a/arch/x86/include/asm/msr-index.h > > +++ b/arch/x86/include/asm/msr-index.h > > @@ -370,7 +370,7 @@ > > #define MSR_IA32_APICBASE 0x0000001b > > #define MSR_IA32_APICBASE_BSP (1<<8) > > #define MSR_IA32_APICBASE_ENABLE (1<<11) > > -#define MSR_IA32_APICBASE_BASE (0xfffff<<12) > > +#define MSR_IA32_APICBASE_BASE (0xfffffUL << 12) > > I don't understand why such warnings is emitted: "left shift of > 1048575 by 12 places cannot be represented in type 'int'" > > Compilers don't complain this code and Linux kernel has the same > definition here. I wrote a basic kernel module printing the result of "(0xfffff << 12)" and kernel UBSAN doesn't complain indeed. I started to compare the compiler flags between Linux and U-Boot and nailed down empirically that Linux UBSAN warning is inhibited by the -fno-strict-overflow gcc option, introduced in Linux commit [1]. The latter actually replaces another gcc option -fwrapv, introduced in [2]. Any of the two flags makes the UBSAN error vanish in the kernel. Neither of the two flags is used in U-Boot. I am in the process of browsing some documentation related to -fwrapv and -fno-strict-overflow (e.g. [3]). Please, feel free to share any thoughts and/or cc anybody who might have dealt with these topics in the past. I will come back with more feedback later. [1] v2.6.31 commit a137802ee839 ("Don't use '-fwrapv' compiler option: it's buggy in gcc-4.1.x") [2] v2.6.29 commit 68df3755e383 ("Add '-fwrapv' to gcc CFLAGS") [3] https://www.airs.com/blog/archives/120 > Regards, > Bin Thanks, Eugeniu.
On Tue, Aug 28, 2018 at 5:06 AM Bin Meng <bmeng.cn@gmail.com> wrote: > On Mon, Aug 27, 2018 at 7:19 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > -#define MSR_IA32_APICBASE_BASE (0xfffff<<12) > > +#define MSR_IA32_APICBASE_BASE (0xfffffUL << 12) > > I don't understand why such warnings is emitted: "left shift of > 1048575 by 12 places cannot be represented in type 'int'" Because it can't. 1 << 30 (fine) 1 << 31 (UB!) This is good explained in the C standard for ages. > Compilers don't complain this code and Linux kernel has the same > definition here. Someone suppressed those warnings. But they are valid.
Hi there, On Tue, Aug 28, 2018 at 08:42:01AM +0200, Eugeniu Rosca wrote: > Hi Bin, > > cc: Masahiro, Andrey > > On Tue, Aug 28, 2018 at 10:05:51AM +0800, Bin Meng wrote: > > Hi Eugeniu, > > > > On Mon, Aug 27, 2018 at 7:19 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > > > > > Fix the following UBSAN report: > > > ====================================================================== > > > UBSAN: Undefined behaviour in arch/x86/cpu/lapic.c:73:14 > > > left shift of 1048575 by 12 places cannot be represented in type 'int' > > > ====================================================================== > > > > > > Steps to reproduce the above: > > > * echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig > > > * make ARCH=x86 qemu-x86_defconfig all > > > * qemu-system-i386 --version > > > QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31) > > > * qemu-system-i386 --nographic -bios u-boot.rom > > > > > > Fixes: 98568f0fa96b ("x86: Import MSR/MTRR code from Linux") > > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > > --- > > > > > > Changes in v2: > > > - None. Newly pushed. > > > --- > > > arch/x86/include/asm/msr-index.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h > > > index 9c1dbe61d596..d8b7b8013c74 100644 > > > --- a/arch/x86/include/asm/msr-index.h > > > +++ b/arch/x86/include/asm/msr-index.h > > > @@ -370,7 +370,7 @@ > > > #define MSR_IA32_APICBASE 0x0000001b > > > #define MSR_IA32_APICBASE_BSP (1<<8) > > > #define MSR_IA32_APICBASE_ENABLE (1<<11) > > > -#define MSR_IA32_APICBASE_BASE (0xfffff<<12) > > > +#define MSR_IA32_APICBASE_BASE (0xfffffUL << 12) > > > > I don't understand why such warnings is emitted: "left shift of > > 1048575 by 12 places cannot be represented in type 'int'" > > > > Compilers don't complain this code and Linux kernel has the same > > definition here. > > I wrote a basic kernel module printing the result of "(0xfffff << 12)" > and kernel UBSAN doesn't complain indeed. > > I started to compare the compiler flags between Linux and U-Boot and > nailed down empirically that Linux UBSAN warning is inhibited by the > -fno-strict-overflow gcc option, introduced in Linux commit [1]. The > latter actually replaces another gcc option -fwrapv, introduced in [2]. > > Any of the two flags makes the UBSAN error vanish in the kernel. > Neither of the two flags is used in U-Boot. > > I am in the process of browsing some documentation related to -fwrapv > and -fno-strict-overflow (e.g. [3]). Please, feel free to share any > thoughts and/or cc anybody who might have dealt with these topics > in the past. I will come back with more feedback later. > > [1] v2.6.31 commit a137802ee839 ("Don't use '-fwrapv' compiler option: it's buggy in gcc-4.1.x") > [2] v2.6.29 commit 68df3755e383 ("Add '-fwrapv' to gcc CFLAGS") > [3] https://www.airs.com/blog/archives/120 > > > Regards, > > Bin Just wanted to let you know that coreboot folks are going through similar discussions in [1]. Also, experimenting with various gcc versions and flags in my spare time, I collected some evidence [2] showing that the behavior of GCC UBSAN (-fsanitize=undefined & friends) may differ a lot depending on the gcc version and below flags (none used by U-Boot, but some used in Linux kernel): -fwrapv -fstrict-overflow -fno-strict-overflow Checking how -fno-strict-overflow and -fwrapv compare to each other (since they seem to accomplish similar goals according to many sources), I've used the sample app from [3] to see how gcc handles signed integer wraparound depending on gcc version, flags, optimization level and on whether UBSAN is enabled or not. The variance/inconsistency of the results [4] is very high in my opinion. One clear conclusion of [4] is that questions like why gcc UBSAN complains in U-Boot but not in the Kernel require knowing at least the parameters tracked in [4] (and maybe more). [1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086146.html [2] UBSAN behavior (printing 1 << 31) is highly dependent on gcc version and flags +----------------------+-------------+-----+ | gcc flags | gcc version | UB? | |----------------------|-------------|-----| | | gcc-4.9.4 | - | | -fsanitize=undefined | gcc-5.5.0 | y | | | gcc-7.3.0 | y | | | gcc-8.1.0 | y | +------------------------------------------+ | | gcc-4.9.4 | - | | -fsanitize=undefined | gcc-5.5.0 | y | | -fstrict-overflow | gcc-7.3.0 | y | | | gcc-8.1.0 | y | +------------------------------------------+ | | gcc-4.9.4 | - | | -fsanitize=undefined | gcc-5.5.0 | y | | -fno-strict-overflow | gcc-7.3.0 | y | | | gcc-8.1.0 | - | +------------------------------------------+ | | gcc-4.9.4 | - | | -fsanitize=undefined | gcc-5.5.0 | y | | -fwrapv | gcc-7.3.0 | - | | | gcc-8.1.0 | - | +----------------------+-------------+-----+ [3] http://thiemonagel.de/2010/01/signed-integer-overflow/ [4] Wraparound [3] dependency on gcc version, flags, optimization level and -fsanitize=undefined | gcc flags | gcc | Wrapped? (UB!) | |-------------------------|-------|------|-----|-----|-----|-----| | | | -O0 | -O1 | -O2 | -O3 | -Os | | | 4.9.4 | y/y! | y/y | n/n | n/n | n/n | | none | 5.5.0 | y/y! | y/y | n/y | n/y | n/y | | (/-fsanitize=undefined) | 7.3.0 | y/y! | y/y | n/y | n/y | n/y | | | 8.1.0 | n/n | n/n | n/n | n/n | n/n | +----------------------------------------------------------------+ | | 4.9.4 | n/n | n/n | n/n | n/n | n/n | | -fstrict-overflow | 5.5.0 | n/y! | n/y | n/y | n/y | n/y | | (/-fsanitize=undefined) | 7.3.0 | n/y! | n/y | n/y | n/y | n/y | | | 8.1.0 | n/n | n/n | n/n | n/n | n/n | +----------------------------------------------------------------+ | | 4.9.4 | y/y! | y/y | y/y | y/y | y/y | | -fno-strict-overflow | 5.5.0 | y/y! | y/y | y/y | y/y | y/y | | (/-fsanitize=undefined) | 7.3.0 | y/y! | y/y | y/y | y/y | y/y | | | 8.1.0 | y/y | y/y | y/y | y/y | y/y | +----------------------------------------------------------------+ | | 4.9.4 | y/y | y/y | y/y | y/y | y/y | | -fwrapv | 5.5.0 | y/y | y/y | y/y | y/y | y/y | | (/-fsanitize=undefined) | 7.3.0 | y/y | y/y | y/y | y/y | y/y | | | 8.1.0 | y/y | y/y | y/y | y/y | y/y | +----------------------------------------------------------------+ Comments/suggestions appreciated. Best regards, Eugeniu.
Hi Eugeniu, On Sat, Sep 1, 2018 at 6:59 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > Hi there, > > On Tue, Aug 28, 2018 at 08:42:01AM +0200, Eugeniu Rosca wrote: > > Hi Bin, > > > > cc: Masahiro, Andrey > > > > On Tue, Aug 28, 2018 at 10:05:51AM +0800, Bin Meng wrote: > > > Hi Eugeniu, > > > > > > On Mon, Aug 27, 2018 at 7:19 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > > > > > > > Fix the following UBSAN report: > > > > ====================================================================== > > > > UBSAN: Undefined behaviour in arch/x86/cpu/lapic.c:73:14 > > > > left shift of 1048575 by 12 places cannot be represented in type 'int' > > > > ====================================================================== > > > > > > > > Steps to reproduce the above: > > > > * echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig > > > > * make ARCH=x86 qemu-x86_defconfig all > > > > * qemu-system-i386 --version > > > > QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31) > > > > * qemu-system-i386 --nographic -bios u-boot.rom > > > > > > > > Fixes: 98568f0fa96b ("x86: Import MSR/MTRR code from Linux") > > > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > > > > --- > > > > > > > > Changes in v2: > > > > - None. Newly pushed. > > > > --- > > > > arch/x86/include/asm/msr-index.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h > > > > index 9c1dbe61d596..d8b7b8013c74 100644 > > > > --- a/arch/x86/include/asm/msr-index.h > > > > +++ b/arch/x86/include/asm/msr-index.h > > > > @@ -370,7 +370,7 @@ > > > > #define MSR_IA32_APICBASE 0x0000001b > > > > #define MSR_IA32_APICBASE_BSP (1<<8) > > > > #define MSR_IA32_APICBASE_ENABLE (1<<11) > > > > -#define MSR_IA32_APICBASE_BASE (0xfffff<<12) > > > > +#define MSR_IA32_APICBASE_BASE (0xfffffUL << 12) > > > > > > I don't understand why such warnings is emitted: "left shift of > > > 1048575 by 12 places cannot be represented in type 'int'" > > > > > > Compilers don't complain this code and Linux kernel has the same > > > definition here. > > > > I wrote a basic kernel module printing the result of "(0xfffff << 12)" > > and kernel UBSAN doesn't complain indeed. > > > > I started to compare the compiler flags between Linux and U-Boot and > > nailed down empirically that Linux UBSAN warning is inhibited by the > > -fno-strict-overflow gcc option, introduced in Linux commit [1]. The > > latter actually replaces another gcc option -fwrapv, introduced in [2]. > > > > Any of the two flags makes the UBSAN error vanish in the kernel. > > Neither of the two flags is used in U-Boot. > > > > I am in the process of browsing some documentation related to -fwrapv > > and -fno-strict-overflow (e.g. [3]). Please, feel free to share any > > thoughts and/or cc anybody who might have dealt with these topics > > in the past. I will come back with more feedback later. > > > > [1] v2.6.31 commit a137802ee839 ("Don't use '-fwrapv' compiler option: it's buggy in gcc-4.1.x") > > [2] v2.6.29 commit 68df3755e383 ("Add '-fwrapv' to gcc CFLAGS") > > [3] https://www.airs.com/blog/archives/120 > > > > > Regards, > > > Bin > > Just wanted to let you know that coreboot folks are going through > similar discussions in [1]. Also, experimenting with various gcc > versions and flags in my spare time, I collected some evidence [2] > showing that the behavior of GCC UBSAN (-fsanitize=undefined & > friends) may differ a lot depending on the gcc version and below > flags (none used by U-Boot, but some used in Linux kernel): > -fwrapv > -fstrict-overflow > -fno-strict-overflow > > Checking how -fno-strict-overflow and -fwrapv compare to each other > (since they seem to accomplish similar goals according to many sources), > I've used the sample app from [3] to see how gcc handles signed integer > wraparound depending on gcc version, flags, optimization level and > on whether UBSAN is enabled or not. The variance/inconsistency of the > results [4] is very high in my opinion. > > One clear conclusion of [4] is that questions like why gcc UBSAN > complains in U-Boot but not in the Kernel require knowing at least the > parameters tracked in [4] (and maybe more). > > [1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086146.html > [2] UBSAN behavior (printing 1 << 31) is highly dependent on gcc version and flags > > +----------------------+-------------+-----+ > | gcc flags | gcc version | UB? | > |----------------------|-------------|-----| > | | gcc-4.9.4 | - | > | -fsanitize=undefined | gcc-5.5.0 | y | > | | gcc-7.3.0 | y | > | | gcc-8.1.0 | y | > +------------------------------------------+ > | | gcc-4.9.4 | - | > | -fsanitize=undefined | gcc-5.5.0 | y | > | -fstrict-overflow | gcc-7.3.0 | y | > | | gcc-8.1.0 | y | > +------------------------------------------+ > | | gcc-4.9.4 | - | > | -fsanitize=undefined | gcc-5.5.0 | y | > | -fno-strict-overflow | gcc-7.3.0 | y | > | | gcc-8.1.0 | - | > +------------------------------------------+ > | | gcc-4.9.4 | - | > | -fsanitize=undefined | gcc-5.5.0 | y | > | -fwrapv | gcc-7.3.0 | - | > | | gcc-8.1.0 | - | > +----------------------+-------------+-----+ > > [3] http://thiemonagel.de/2010/01/signed-integer-overflow/ > > [4] Wraparound [3] dependency on gcc version, flags, optimization level and -fsanitize=undefined > > | gcc flags | gcc | Wrapped? (UB!) | > |-------------------------|-------|------|-----|-----|-----|-----| > | | | -O0 | -O1 | -O2 | -O3 | -Os | > | | 4.9.4 | y/y! | y/y | n/n | n/n | n/n | > | none | 5.5.0 | y/y! | y/y | n/y | n/y | n/y | > | (/-fsanitize=undefined) | 7.3.0 | y/y! | y/y | n/y | n/y | n/y | > | | 8.1.0 | n/n | n/n | n/n | n/n | n/n | > +----------------------------------------------------------------+ > | | 4.9.4 | n/n | n/n | n/n | n/n | n/n | > | -fstrict-overflow | 5.5.0 | n/y! | n/y | n/y | n/y | n/y | > | (/-fsanitize=undefined) | 7.3.0 | n/y! | n/y | n/y | n/y | n/y | > | | 8.1.0 | n/n | n/n | n/n | n/n | n/n | > +----------------------------------------------------------------+ > | | 4.9.4 | y/y! | y/y | y/y | y/y | y/y | > | -fno-strict-overflow | 5.5.0 | y/y! | y/y | y/y | y/y | y/y | > | (/-fsanitize=undefined) | 7.3.0 | y/y! | y/y | y/y | y/y | y/y | > | | 8.1.0 | y/y | y/y | y/y | y/y | y/y | > +----------------------------------------------------------------+ > | | 4.9.4 | y/y | y/y | y/y | y/y | y/y | > | -fwrapv | 5.5.0 | y/y | y/y | y/y | y/y | y/y | > | (/-fsanitize=undefined) | 7.3.0 | y/y | y/y | y/y | y/y | y/y | > | | 8.1.0 | y/y | y/y | y/y | y/y | y/y | > +----------------------------------------------------------------+ > > Comments/suggestions appreciated. Thank you very much for all these details and links. I learned a lot from this thread. It looks to me that coreboot folks are not in favor of this UBSAN thing. I personally have no preference, but I suspect there are a lot more in the U-Boot tree that have such warnings. Given the behavior is quite dependent on GCC versions and flags, do kernel folks have any final solution on this? Or do we have to ask GCC folks to fix these inconsistencies? Regards, Bin
Hi Bin, Apologize for the delay. I came back from vacation a few days ago. On Tue, Sep 04, 2018 at 12:00:14PM +0800, Bin Meng wrote: > Hi Eugeniu, > > On Sat, Sep 1, 2018 at 6:59 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: [..] > > Just wanted to let you know that coreboot folks are going through > > similar discussions in [1]. Also, experimenting with various gcc > > versions and flags in my spare time, I collected some evidence [2] > > showing that the behavior of GCC UBSAN (-fsanitize=undefined & > > friends) may differ a lot depending on the gcc version and below > > flags (none used by U-Boot, but some used in Linux kernel): > > -fwrapv > > -fstrict-overflow > > -fno-strict-overflow > > > > Checking how -fno-strict-overflow and -fwrapv compare to each other > > (since they seem to accomplish similar goals according to many sources), > > I've used the sample app from [3] to see how gcc handles signed integer > > wraparound depending on gcc version, flags, optimization level and > > on whether UBSAN is enabled or not. The variance/inconsistency of the > > results [4] is very high in my opinion. > > > > One clear conclusion of [4] is that questions like why gcc UBSAN > > complains in U-Boot but not in the Kernel require knowing at least the > > parameters tracked in [4] (and maybe more). > > > > [1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086146.html > > [2] UBSAN behavior (printing 1 << 31) is highly dependent on gcc version and flags [..] > Thank you very much for all these details and links. I learned a lot > from this thread. It looks to me that coreboot folks are not in favor > of this UBSAN thing. I personally have no preference, but I suspect > there are a lot more in the U-Boot tree that have such warnings. I don't think they question the usefulness of UBSAN as a whole. Rather, they seem to be annoyed by one particular (frequently reproduced) UBSAN error, specifically what gcc man pages refer to as "left-shifting 1 into the sign bit". Note that shifting *past* the sign bit is a different (presumably more serious) subclass of signed integer shift overflow. Both the coreboot discussion and the fixes from my series are limited to "left-shifting into (not past) the sign bit" behavior. Both the C11 [6] standard (to which U-Boot "adhered" via commit [5]) and the later C18 [7] define the left shifts as follows (§6.5.7p4): ------8<------ The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are filled with zeros. If E1 has an unsigned type, the value of the result is E1 × 2^E2, reduced modulo one more than the maximum value representable in the result type. If E1 has a signed type and nonnegative value, and E1 × 2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined. ------8<------ With respect to the type of the result, both C11/C18 standards state in §6.5.7p3: ------8<------ The type of the result is that of the promoted left operand. ------8<------ My understanding of the above is that, purely from C11/C18 standard perspective, (1 << 31) is undefined behavior since the result can't be represented in the type of the left operand (signed int). In spite of this, things are not as simple as we would like them to be and the DR463 entry of the "Defect Report Summary for C11" [8] tackles exactly the "Left-shifting into the sign bit" by saying that it should be harmonized with C++-14 [9]. The latter suffered a change in "§5.8.2 Shift operators" chapter due to DR1457 ("Undefined behavior in left-shift") [10], a defect report raised back in 2012. As a result, C++-14 now considers the "left-shifting into the sign bit" as defined behavior: ------8<------ ...if E1 has a signed type and non-negative value, and E1 ⨯ 2^E2 is representable in the **corresponding unsigned type of the result type**, then that value, **converted to the result type**, is the resulting value; otherwise, the behavior is undefined. ------8<------ To emphasize that the things are far from being settled, I will just reference the UB-related talk [11] of Chandler Carruth at CppCon 2016, which (amongst other topics) touches the left shifting of signed integers and, to my understanding, conveys the message that there are holes in the mental model of shifting signed integers to the left and the solution to overcome those is still unclear. > Given > the behavior is quite dependent on GCC versions and flags, do kernel > folks have any final solution on this? I still didn't fully answer one of your first questions, more exactly why UBSAN complains about (1 << 31) in U-Boot, but not in Linux kernel. It turns out there is another gcc option heavily affecting the UBSAN behavior and it is (somewhat expectedly) the language standard "-std=" [12]. As already mentioned, U-Boot recently switched to C11 via commit [5], while Linux kernel still sticks to ANSI/C89 C via commit [13]. Just for the record, the definition of left shift operator provided by C89/C90 [14] is indeed different compared to C11/C18 and it sounds like: ------8<------ The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are filled with zeros. If E1 has an unsigned type, the value of the result is E1 multiplied by the quantity, 2 raised to the power E2, reduced modulo ULONG_MAX+1 if E1 has type unsigned long, UINT_MAX+1 otherwise. (The constants ULONG_MAX and UINT_MAX are defined in the header <limits.h> .) ------8<------ I am not sure if this makes left-shifting into the sign bit legitimate, but the reality is that none of the x86_64 gcc versions I tried (this includes 4.9.4, 5.5.0, 6.4.0, 7.3.0, 8.1.0) reported issues about (1 << 31) when turning UBSAN on with -std=gnu89. This is the main reason why Linux kernel UBSAN won't complain about (1 << 31). On the basis of above experimental results, switching U-Boot build system back to C89 would both keep it aligned to Linux kernel and would consistently silent all the "left-shift 1 into sign bit" UBSAN errors (of which there are potentially hundreds, according to the stats presented in [15]). At the same time, this feels like a step back, so I actually expect people to NAK this change. It still remains the simplest I can think of (hence my favorite). It is also grounded on my belief that the changes in the Kconfig/Kbuild should come top-down to U-Boot from Linux kernel (where the actual development takes place). > Or do we have to ask GCC folks to fix these inconsistencies? While trying to create an account in GCC Bugzilla, I got the message: "User account creation has been restricted" :( Please, feel free to CC any member of GCC community. Any feedback/comments which direction to take would be appreciated. [5] fa893990e9b5 ("Makefile: Ensure we build with -std=gnu11") [6] C11 draft: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf [7] C18 draft: http://www.open-std.org/jtc1/sc22/wg14/www/abq/c17_updated_proposed_fdis.pdf [8] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2148.htm#dr_463 [9] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf [10] http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1457 [11] https://youtu.be/yG1OZ69H_-o?t=1668 [12] https://gcc.gnu.org/onlinedocs/gcc/Standards.html [13] Linux v3.18-rc5 commit 51b97e354ba9 ("kernel: use the gnu89 standard explicitly") [14] https://port70.net/~nsz/c/c89/c89-draft.html#3.3.7 [15] http://patchwork.ozlabs.org/cover/962307/ Best regards, Eugeniu.
Hi Bin,
jFYI, I've created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87392
("UBSAN behavior on left-shifting 1 into the sign bit is dependent on C
standard"), to get some recommendation from GCC guys how to handle
these warnings in U-Boot.
Regards,
Eugeniu.
Hi Eugeniu, On Sun, Sep 23, 2018 at 7:10 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > Hi Bin, > > jFYI, I've created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87392 > ("UBSAN behavior on left-shifting 1 into the sign bit is dependent on C > standard"), to get some recommendation from GCC guys how to handle > these warnings in U-Boot. Thank you very much for following up with the gcc folks! Let's see how they respond. BTW: your bug report is elaborate. Well done on the research! Regards, Bin
Hi Bin, On Tue, Sep 25, 2018 at 10:06:52AM +0800, Bin Meng wrote: > Hi Eugeniu, > > On Sun, Sep 23, 2018 at 7:10 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > > > Hi Bin, > > > > jFYI, I've created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87392 > > ("UBSAN behavior on left-shifting 1 into the sign bit is dependent on C > > standard"), to get some recommendation from GCC guys how to handle > > these warnings in U-Boot. > > Thank you very much for following up with the gcc folks! Let's see how > they respond. > > BTW: your bug report is elaborate. Well done on the research! > > Regards, > Bin I feel like before UBSAN reaches mainline U-Boot, we will make some friends in the compiler communities. I have raised another bug report [1], this time to LLVM folks, since U-Boot simply refuses to boot when built with clang and UBSAN=y. This new issue is related to the implementation of U-Boot linker-generated arrays, as summarized in the cover letter [2] of my series. Somehow, GCC UBSAN cooperates well with the linker-generated arrays, while Clang UBSAN does not. Hopefully this will be clarified in [1] and hopefully no significant changes will be needed in include/linker_lists.h to allow booting -fsanitized clang-built U-Boot. Regarding the GCC discussion [3], it is relatively settled, but not to our advantage. GCC folks first clarified (credits to them for that) how shifting into (not past) the sign bit is defined in the existing C standards. Specifically, C89/C90 considers this "implementation-defined", while more recent C standards (C99, C11, C18) make this "undefined". Since U-Boot is compiled using -std=gnu11, "shifting into the sign bit" errors look legitimate. On the other hand, official GCC documentation says [4]: > As an extension to the C language, GCC does not use the latitude given > in C99 and C11 only to treat certain aspects of signed ‘<<’ as > undefined. The above quote was used by GCC guys to actually support/convey the idea that some aspects of left-shifting (e.g. left-shifting into the sign bit) are still defined in GCC (i.e. they don't lead to UB). If so, then I am really puzzled, since I do not understand the practicality of bothering users with errors which reflect what C standard says on paper instead of how it is implemented in the compiler internals. This is pretty much the most recent status of the discussion and, as you can see, it doesn't shed too much light on how to tackle the left- shifting overflows into the sign bit (fix them, ignore them, roll back the C standard, etc). This is still to be decided by the U-Boot community. [1] https://bugs.llvm.org/show_bug.cgi?id=39219 [2] https://patchwork.ozlabs.org/cover/962307/ [3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87392 [4] https://gcc.gnu.org/onlinedocs/gcc-8.2.0/gcc/Integers-implementation.html#Integers-implementation Best regards, Eugeniu.
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h index 9c1dbe61d596..d8b7b8013c74 100644 --- a/arch/x86/include/asm/msr-index.h +++ b/arch/x86/include/asm/msr-index.h @@ -370,7 +370,7 @@ #define MSR_IA32_APICBASE 0x0000001b #define MSR_IA32_APICBASE_BSP (1<<8) #define MSR_IA32_APICBASE_ENABLE (1<<11) -#define MSR_IA32_APICBASE_BASE (0xfffff<<12) +#define MSR_IA32_APICBASE_BASE (0xfffffUL << 12) #define MSR_IA32_TSCDEADLINE 0x000006e0
Fix the following UBSAN report: ====================================================================== UBSAN: Undefined behaviour in arch/x86/cpu/lapic.c:73:14 left shift of 1048575 by 12 places cannot be represented in type 'int' ====================================================================== Steps to reproduce the above: * echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig * make ARCH=x86 qemu-x86_defconfig all * qemu-system-i386 --version QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31) * qemu-system-i386 --nographic -bios u-boot.rom Fixes: 98568f0fa96b ("x86: Import MSR/MTRR code from Linux") Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> --- Changes in v2: - None. Newly pushed. --- arch/x86/include/asm/msr-index.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)