[PATCH] sim: warnings: disable -Wshift-negative-value

[PATCH] sim: warnings: disable -Wshift-negative-value

[Mike Frysinger]

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(+)

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
-- 
2.43.0

Re: [PATCH] sim: warnings: disable -Wshift-negative-value

[Joseph Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] sim: warnings: disable -Wshift-negative-value

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1947 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] sim: cgen: rework DI macros to avoid signed left shifts

[Mike Frysinger]

The cgen code uses DI as int64_t and UDI as uint64_t.  The DI macros
are used to construct 64-bit values from 32-bit values (for the low
and high parts).  The MAKEDI macro casts the high 32-bit value to a
signed 32-bit value before shifting.  If this created a negative
value, this would be undefined behavior according to the C standard.
All we care about is shifting the 32-bits as they are to the high
32-bits, not caring about sign extension (since there's nothing left
to shift into), and the low 32-bits being empty.  This is what we
get from shifting an unsigned value, so cast it to unsigned 32-bit
to avoid undefined behavior.

While we're here, change the SETLODI macro to truncate the lower
value to 32-bits before we set it.  If it was passing in a 64-bit
value, those high bits would get included too, and that's not what
we want.

Similarly, tweak the SETHIDI macro to cast the value to an unsigned
64-bit instead of a signed 64-bit.  If the value was only 32-bits,
the behavior would be the same.  If it happened to be signed 64-bit,
it would trigger the undefined behavior too.
---
 sim/common/cgen-types.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sim/common/cgen-types.h b/sim/common/cgen-types.h
index 01a3ee9be584..24c2b89216fc 100644
--- a/sim/common/cgen-types.h
+++ b/sim/common/cgen-types.h
@@ -72,9 +72,9 @@ typedef int64_t DI;
 typedef uint64_t UDI;
 #define GETLODI(di) ((SI) (di))
 #define GETHIDI(di) ((SI) ((UDI) (di) >> 32))
-#define SETLODI(di, val) ((di) = (((di) & 0xffffffff00000000LL) | (val)))
-#define SETHIDI(di, val) ((di) = (((di) & 0xffffffffLL) | (((DI) (val)) << 32)))
-#define MAKEDI(hi, lo) ((((DI) (SI) (hi)) << 32) | ((UDI) (USI) (lo)))
+#define SETLODI(di, val) ((di) = (((di) & 0xffffffff00000000LL) | (USI) (val)))
+#define SETHIDI(di, val) ((di) = (((di) & 0xffffffffLL) | (((UDI) (val)) << 32)))
+#define MAKEDI(hi, lo) ((DI) (((UDI) (hi) << 32) | (UDI) (USI) (lo)))
 
 /* These are used to record extracted raw data from an instruction, among other
    things.  It must be a host data type, and not a target one.  */
-- 
2.43.0