sim: warnings: disable -Wshift-negative-value

Message ID 20231224082639.18038-1-vapier@gentoo.org
State New
Headers
Series sim: warnings: disable -Wshift-negative-value |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Mike Frysinger Dec. 24, 2023, 8:26 a.m. UTC
  The sim expects left shift operations on negative values to have two's
compliment behavior, and right shift operations to sign extend.  In C89,
this was not explicitly mentioned.  In C90, this was changed to undefined
behavior.  In C23, this was settled as the behavior we want in N2412 [1].
One C23 proposal documented that GCC, LLVM, and MSVC already behaved this
way [2], as did every known hardware, and GCC guarantees it behaves this
way in all C standards as an extension.  So disable the warning in case a
compiler automatically turns it on when running in C11 mode (which is our
required minimum version).

From the GCC manual (4.5 Integers):
> https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html
> The results of some bitwise operations on signed integers (C90 6.3, C99 and C11 6.5).
>
> Bitwise operators act on the representation of the value including
> both the sign and value bits, where the sign bit is considered
> immediately above the highest-value value bit. Signed ‘>>’ acts on
> negative numbers by sign extension.
>
> 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.
> However, -fsanitize=shift (and -fsanitize=undefined) will diagnose such
> cases. They are also diagnosed where constant expressions are required.

[1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2412.pdf
[2] https://wg21.link/P0907R4 (not adopted, but has relevant research)
---
 sim/configure                    | 1 +
 sim/m4/sim_ac_option_warnings.m4 | 4 ++++
 2 files changed, 5 insertions(+)
  

Comments

Joseph Myers Dec. 29, 2023, 12:52 a.m. UTC | #1
On Sun, 24 Dec 2023, Mike Frysinger wrote:

> The sim expects left shift operations on negative values to have two's
> compliment behavior, and right shift operations to sign extend.  In C89,
> this was not explicitly mentioned.  In C90, this was changed to undefined
> behavior.  In C23, this was settled as the behavior we want in N2412 [1].

No, there has been no change in C23 to the rules for which shifts are 
undefined.  The change made was to make *representations* of integer types 
defined as two's complement; there were no changes to semantics of 
*operations* on such types.
  
Mike Frysinger Jan. 5, 2024, 7:40 a.m. UTC | #2
On 29 Dec 2023 00:52, Joseph Myers wrote:
> On Sun, 24 Dec 2023, Mike Frysinger wrote:
> > The sim expects left shift operations on negative values to have two's
> > compliment behavior, and right shift operations to sign extend.  In C89,
> > this was not explicitly mentioned.  In C90, this was changed to undefined
> > behavior.  In C23, this was settled as the behavior we want in N2412 [1].
> 
> No, there has been no change in C23 to the rules for which shifts are 
> undefined.  The change made was to make *representations* of integer types 
> defined as two's complement; there were no changes to semantics of 
> *operations* on such types.

thanks, i thought the meat of P0907 was adopted, albeit via a diff proposal.
not that the final thing adopted was only about twos compliment representation.

looking closer at the source of the warnings, it seems to all come from the
macro that creates a 64-bit value from 2 32-bit values, and specifically
when shifting a negative constant into the upper 32-bits.  but we don't need
to make the upper 32-bits signed before shifting since we're going to throw
away all of the initial upper 32-bits, so we can prob fix this with:
-#define MAKEDI(hi, lo) ((((int64_t)  (int32_t) (hi)) << 32) | ((uint64_t) (uint32_t) (lo)))
+#define MAKEDI(hi, lo) ((((int64_t) (uint32_t) (hi)) << 32) | ((uint64_t) (uint32_t) (lo)))

for clarity, i'm kind of inclined to do the operations only as unsigned,
rely on the types to do the appropriate truncation/extension, and then
convert to signed at the end.
#define MAKEDI(hi, lo) ( \
	(int64_t)( \
		((uint64_t)(hi) << 32)) | (uint32_t) (lo) \
	) \
)

there's prob more left shifting of negative values lurking in the tree, but
none that trigger compile-time warnings, so i'm inclined to not audit.  the
harder part would be implementing sign extension with right shifts, so
hopefully we never have to cross that bridge.
-mike
  

Patch

diff --git a/sim/m4/sim_ac_option_warnings.m4 b/sim/m4/sim_ac_option_warnings.m4
index 8a54e563bad2..41a7c5c8c0d6 100644
--- a/sim/m4/sim_ac_option_warnings.m4
+++ b/sim/m4/sim_ac_option_warnings.m4
@@ -74,6 +74,10 @@  build_warnings="$build_warnings
 dnl The cgen virtual insn logic involves enum conversions.
 dnl Disable until we can figure out how to make this work.
 -Wno-enum-conversion
+dnl The sim expects left shift operations on negative values to have two's
+dnl compliment behavior, and right shift operations to sign extend.  In
+dnl practice, all compilers do this, and C23 settled it, so disable the warning.
+-Wno-shift-negative-value
 "
 
 case "${host}" in