[PATCH 0/2] Fix invalid left shift of negative value.

[PATCH 0/2] Fix invalid left shift of negative value.

[Dominik Vogt]

The following series of patches fixes all occurences of
left-shifting negative constants in C code which is undefined by
the C standard.  The patches have been tested on s390x, covering
only a small subset of the changes.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

Re: [PATCH 1/2] Fix invalid left shift of negative value.

[Dominik Vogt]

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

On Tue, Nov 10, 2015 at 12:16:38PM +0100, Dominik Vogt wrote:
> The following series of patches fixes all occurences of
> left-shifting negative constants in C code which is undefined by
> the C standard.  The patches have been tested on s390x, covering
> only a small subset of the changes.

Changes in gdb/.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[-- Attachment #2: 0001-gdb-Fix-left-shift-of-negative-value.patch --]
[-- Type: text/x-diff, Size: 2913 bytes --]

From f0480d41f3036d193513fa4dfbba414201b610ec Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Fri, 30 Oct 2015 15:17:22 +0100
Subject: [PATCH 1/2] gdb: Fix left shift of negative value.

This patch fixes all occurences of left-shifting negative constants in C cod
which is undefined by the C standard.

gdb/ChangeLog:

        * hppa-tdep.c (hppa_sign_extend, hppa_low_hppa_sign_extend)
        (prologue_inst_adjust_sp, hppa_frame_cache): Fix left shift of negative
        value.
        * dwarf2read.c (read_subrange_type): Likewise.
---
 gdb/ChangeLog    | 7 +++++++
 gdb/dwarf2read.c | 2 +-
 gdb/hppa-tdep.c  | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 87dc8b4..48921e7 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -15048,7 +15048,7 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
      the bounds as signed, and thus sign-extend their values, when
      the base type is signed.  */
   negative_mask =
-    (LONGEST) -1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1);
+    -((LONGEST) 1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1));
   if (low.kind == PROP_CONST
       && !TYPE_UNSIGNED (base_type) && (low.data.const_val & negative_mask))
     low.data.const_val |= negative_mask;
diff --git a/gdb/hppa-tdep.c b/gdb/hppa-tdep.c
index ba7f946..3206729 100644
--- a/gdb/hppa-tdep.c
+++ b/gdb/hppa-tdep.c
@@ -104,7 +104,7 @@ static const struct objfile_data *hppa_objfile_priv_data = NULL;
 static int
 hppa_sign_extend (unsigned val, unsigned bits)
 {
-  return (int) (val >> (bits - 1) ? (-1 << bits) | val : val);
+  return (int) (val >> (bits - 1) ? (-(1 << bits)) | val : val);
 }
 
 /* For many immediate values the sign bit is the low bit!  */
@@ -112,7 +112,7 @@ hppa_sign_extend (unsigned val, unsigned bits)
 static int
 hppa_low_hppa_sign_extend (unsigned val, unsigned bits)
 {
-  return (int) ((val & 0x1 ? (-1 << (bits - 1)) : 0) | val >> 1);
+  return (int) ((val & 0x1 ? (-(1 << (bits - 1))) : 0) | val >> 1);
 }
 
 /* Extract the bits at positions between FROM and TO, using HP's numbering
@@ -1357,7 +1357,7 @@ prologue_inst_adjust_sp (unsigned long inst)
 
   /* std,ma X,D(sp) */
   if ((inst & 0xffe00008) == 0x73c00008)
-    return (inst & 0x1 ? -1 << 13 : 0) | (((inst >> 4) & 0x3ff) << 3);
+    return (inst & 0x1 ? -(1 << 13) : 0) | (((inst >> 4) & 0x3ff) << 3);
 
   /* addil high21,%r30; ldo low11,(%r1),%r30)
      save high bits in save_high21 for later use.  */
@@ -2066,7 +2066,7 @@ hppa_frame_cache (struct frame_info *this_frame, void **this_cache)
 		CORE_ADDR offset;
 		
 		if ((inst >> 26) == 0x1c)
-		  offset = (inst & 0x1 ? -1 << 13 : 0)
+		  offset = (inst & 0x1 ? -(1 << 13) : 0)
 		    | (((inst >> 4) & 0x3ff) << 3);
 		else if ((inst >> 26) == 0x03)
 		  offset = hppa_low_hppa_sign_extend (inst & 0x1f, 5);
-- 
2.3.0

Re: [PATCH 2/2] Fix invalid left shift of negative value.

[Dominik Vogt]

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

On Tue, Nov 10, 2015 at 12:16:38PM +0100, Dominik Vogt wrote:
> The following series of patches fixes all occurences of
> left-shifting negative constants in C code which is undefined by
> the C standard.  The patches have been tested on s390x, covering
> only a small subset of the changes.

Changes in gdb/testsuite/.  Not sure whether they are good.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[-- Attachment #2: 0002-gdb-testsuite-Fix-left-shift-of-negative-value.patch --]
[-- Type: text/x-diff, Size: 1935 bytes --]

From d1cbc6f371e2720fe4bf4975d8ad9c81a9f01351 Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Fri, 30 Oct 2015 15:18:06 +0100
Subject: [PATCH 2/2] gdb/testsuite: Fix left shift of negative value.

This patch fixes all occurences of left-shifting negative constants in C cod
which is undefined by the C standard.

gdb/testsuite/ChangeLog:

        * lib/dwarf.exp (_note): Fix left shift of negative value.
        * gdb.trace/trace-condition.exp: Likewise.
---
 gdb/testsuite/gdb.trace/trace-condition.exp | 2 +-
 gdb/testsuite/lib/dwarf.exp                 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/trace-condition.exp b/gdb/testsuite/gdb.trace/trace-condition.exp
index aec0401..46fa5ed 100644
--- a/gdb/testsuite/gdb.trace/trace-condition.exp
+++ b/gdb/testsuite/gdb.trace/trace-condition.exp
@@ -152,7 +152,7 @@ foreach trace_command { "trace" "ftrace" } {
     test_tracepoints $trace_command "21 * 2 == 42" 10
     test_tracepoints $trace_command "21 << 1 == 42" 10
     test_tracepoints $trace_command "42 >> 1 == 21" 10
-    test_tracepoints $trace_command "-21 << 1 == -42" 10
+    test_tracepoints $trace_command "-(21 << 1) == -42" 10
     test_tracepoints $trace_command "-42 >> 1 == -21" 10
     test_tracepoints $trace_command "(0xabababab & 0x0000ffff) == 0xabab" 10
     test_tracepoints $trace_command "(0xabababab | 0x0000ffff) == 0xababffff" 10
diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index 9716795..c87da87 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -1289,7 +1289,7 @@ namespace eval Dwarf {
 	_op .ascii [_quote $name]
 	# Alignment.
 	set align 2
-	set total [expr {($namelen + (1 << $align) - 1) & (-1 << $align)}]
+	set total [expr {($namelen + (1 << $align) - 1) & -(1 << $align)}]
 	for {set i $namelen} {$i < $total} {incr i} {
 	    _op .byte 0
 	}
-- 
2.3.0

Re: [PATCH 0/2] Fix invalid left shift of negative value.

[Kevin Buettner]

On Tue, 10 Nov 2015 12:16:38 +0100
Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:

> The following series of patches fixes all occurences of
> left-shifting negative constants in C code which is undefined by
> the C standard.  The patches have been tested on s390x, covering
> only a small subset of the changes.

Looking at one of your changes from part 1/2...

-    (LONGEST) -1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1);
+    -((LONGEST) 1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1));

What aspect of the original expression is not defined by the C standard?

(I'm looking at an old copy of Harbison & Steel; the original expression
looks okay to me, but perhaps I'm missing something...)

FWIW, I have an easier time understanding the intent of the first/original
expression than your proposed replacement.

Kevin

Re: [PATCH 0/2] Fix invalid left shift of negative value.

[Ulrich Weigand]

Kevin Büttner wrote:

> Looking at one of your changes from part 1/2...
> 
> -    (LONGEST) -1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1);
> +    -((LONGEST) 1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1));
> 
> What aspect of the original expression is not defined by the C standard?

The C standard (either C99 or C11) says:

  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.

Note the "otherwise" case includes any E1 of signed type and negative value.

(For >>, the behavior in the latter case is at least implementation-
defined, and not undefined.)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 0/2] Fix invalid left shift of negative value.

[Kevin Buettner]

On Wed, 11 Nov 2015 18:23:27 +0100 (CET)
"Ulrich Weigand" <uweigand@de.ibm.com> wrote:

> Kevin Buettner wrote:
> 
> > Looking at one of your changes from part 1/2...
> > 
> > -    (LONGEST) -1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1);
> > +    -((LONGEST) 1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1));
> > 
> > What aspect of the original expression is not defined by the C standard?
> 
> The C standard (either C99 or C11) says:
> 
>   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.
> 
> Note the "otherwise" case includes any E1 of signed type and negative value.
> 
> (For >>, the behavior in the latter case is at least implementation-
> defined, and not undefined.)

Thank you for providing the relevant text from the standard.

Do you (or anyone else) know the rationale for specifying that the
behavior of << is undefined for (signed) negative values?

My guess is that it's due to the fact that there are several ways
to represent signed numbers and that the standard has to account for
all of them.

If that guess is correct, then it seems to me that using the unary
minus operator to help construct a mask is most likely broken for some
signed number representations.  (I.e. we won't get the mask that
we've come to expect from the two's complement representation.)  If so,
we should consider whether we want to find a more portable way to
construct these masks.

Regardless, I want to have a better understanding of this matter
before approving Dominik's patch.

Kevin

Re: [PATCH 0/2] Fix invalid left shift of negative value.

[Kevin Buettner]

On Wed, 11 Nov 2015 12:27:08 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> On Wed, 11 Nov 2015 18:23:27 +0100 (CET)
> "Ulrich Weigand" <uweigand@de.ibm.com> wrote:
> 
> > Kevin Buettner wrote:
> > 
> > > Looking at one of your changes from part 1/2...
> > > 
> > > -    (LONGEST) -1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1);
> > > +    -((LONGEST) 1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1));
> > > 
> > > What aspect of the original expression is not defined by the C standard?
> > 
> > The C standard (either C99 or C11) says:
> > 
> >   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.
> > 
> > Note the "otherwise" case includes any E1 of signed type and negative value.
> > 
> > (For >>, the behavior in the latter case is at least implementation-
> > defined, and not undefined.)
> 
> Thank you for providing the relevant text from the standard.
> 
> Do you (or anyone else) know the rationale for specifying that the
> behavior of << is undefined for (signed) negative values?
> 
> My guess is that it's due to the fact that there are several ways
> to represent signed numbers and that the standard has to account for
> all of them.
> 
> If that guess is correct, then it seems to me that using the unary
> minus operator to help construct a mask is most likely broken for some
> signed number representations.  (I.e. we won't get the mask that
> we've come to expect from the two's complement representation.)  If so,
> we should consider whether we want to find a more portable way to
> construct these masks.
> 
> Regardless, I want to have a better understanding of this matter
> before approving Dominik's patch.

I've been pondering this some more.  It seems to me that there are
more than a few places in GDB that assume that two's complement
is being used as the representation for signed integers.

I came across this comment in defs.h:

/* Defaults for system-wide constants (if not defined by xm.h, we fake it).
   FIXME: Assumes 2's complement arithmetic.  */

Is this something that we really want to fix?  Can anyone think of a
host which can't run GDB (and upon which we'd like to run GDB) due the
fact that it uses something other than the two's complement
representation for signed integers?

My opinion:  Assumptions about two's complement in GDB should not be
fixed.  I can't think of any architecture that I'd care to use which
uses something other than two's complement.  My limited research on
the matter shows that really archaic machines used one's complement or
signed magnitude representations.

If we all agree that this is something we don't want to fix, then I
think we should remove that FIXME and assert somewhere that GDB is
expected to be hosted on platforms which use two's complement
representation for signed integers.

With that in mind...

I've looked over both of Dominik's patches.  They look okay to me.

Kevin

Re: [PATCH 0/2] Fix invalid left shift of negative value.

[Pedro Alves]

On 11/17/2015 05:09 AM, Kevin Buettner wrote:

> I came across this comment in defs.h:
> 
> /* Defaults for system-wide constants (if not defined by xm.h, we fake it).
>    FIXME: Assumes 2's complement arithmetic.  */
> 

(side note, the xm.h is gone since 2007..)

> Is this something that we really want to fix?  Can anyone think of a
> host which can't run GDB (and upon which we'd like to run GDB) due the
> fact that it uses something other than the two's complement
> representation for signed integers?

Can't think of one.

> My opinion:  Assumptions about two's complement in GDB should not be
> fixed.  I can't think of any architecture that I'd care to use which
> uses something other than two's complement.  My limited research on
> the matter shows that really archaic machines used one's complement or
> signed magnitude representations.
> 
> If we all agree that this is something we don't want to fix, then I
> think we should remove that FIXME and assert somewhere that GDB is
> expected to be hosted on platforms which use two's complement
> representation for signed integers.

Agreed.  If someone wants to port gdb to such a host, then we can
worry about it then.

Thanks,
Pedro Alves

Re: [PATCH 0/2] Fix invalid left shift of negative value.

[Paul_Koning]

> On Nov 17, 2015, at 9:13 AM, Pedro Alves <palves@redhat.com> wrote:
> 
> On 11/17/2015 05:09 AM, Kevin Buettner wrote:
> 
>> ...
>> Is this something that we really want to fix?  Can anyone think of a
>> host which can't run GDB (and upon which we'd like to run GDB) due the
>> fact that it uses something other than the two's complement
>> representation for signed integers?
> 
> Can't think of one.

Agreed.  One's complement is seriously obsolete.  I can't think of any architecture in current use (in actual hardware) that uses it.  You can of course easily find any number of examples supported in simulators like SIMH, but that isn't significant.  None of those are likely to become GDB targets.

>> My opinion:  Assumptions about two's complement in GDB should not be
>> fixed.  I can't think of any architecture that I'd care to use which
>> uses something other than two's complement.  My limited research on
>> the matter shows that really archaic machines used one's complement or
>> signed magnitude representations.
>> 
>> If we all agree that this is something we don't want to fix, then I
>> think we should remove that FIXME and assert somewhere that GDB is
>> expected to be hosted on platforms which use two's complement
>> representation for signed integers.
> 
> Agreed.  If someone wants to port gdb to such a host, then we can
> worry about it then.

I remember reading a claim that C requires two's complement (at least ANSI C), so it's not likely that this will ever happen, and if it does, GDB will be the least of the port developer's worries.

	paul

Re: [PATCH 3/2] Fix invalid left shift of negative value.

[Dominik Vogt]

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

And another one:

On Tue, Nov 10, 2015 at 12:16:38PM +0100, Dominik Vogt wrote:
> The following series of patches fixes all occurences of
> left-shifting negative constants in C code which is undefined by
> the C standard.  The patches have been tested on s390x, covering
> only a small subset of the changes.

Changes in sim/.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[-- Attachment #2: 0003-ChangeLog --]
[-- Type: text/plain, Size: 467 bytes --]

sim/arm/ChangeLog

	* thumbemu.c (handle_T2_insn): Fix left shift of negative value.
	* armemu.c (handle_v6_insn): Likewise.

sim/avr/ChangeLog

	* interp.c (sign_ext): Fix left shift of negative value.

sim/mips/ChangeLog

	* micromips.igen (process_isa_mode): Fix left shift of negative value.

sim/msp430/ChangeLog

	* msp430-sim.c (get_op, put_op): Fix left shift of negative value.

sim/v850/ChangeLog

	* simops.c (v850_bins): Fix left shift of negative value.

[-- Attachment #3: 0003-sim-Fix-left-shift-of-negative-value.patch --]
[-- Type: text/x-diff, Size: 7684 bytes --]

From 5855e92b06a55ec2cba580788361fbbcc0f1c754 Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Fri, 30 Oct 2015 15:19:28 +0100
Subject: [PATCH 3/2] sim: Fix left shift of negative value.

---
 sim/arm/armemu.c        | 40 ++++++++++++++++++++--------------------
 sim/arm/thumbemu.c      | 16 ++++++++--------
 sim/avr/interp.c        |  2 +-
 sim/mips/micromips.igen |  2 +-
 sim/msp430/msp430-sim.c |  4 ++--
 sim/v850/simops.c       |  2 +-
 6 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/sim/arm/armemu.c b/sim/arm/armemu.c
index f2a84eb..3826c78 100644
--- a/sim/arm/armemu.c
+++ b/sim/arm/armemu.c
@@ -351,11 +351,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
 	      {
 		n = (val1 >> i) & 0xFFFF;
 		if (n & 0x8000)
-		  n |= -1 << 16;
+		  n |= -(1 << 16);
 
 		m = (val2 >> i) & 0xFFFF;
 		if (m & 0x8000)
-		  m |= -1 << 16;
+		  m |= -(1 << 16);
 
 		r = n + m;
 
@@ -371,11 +371,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
 	  case 0xF3: /* QASX<c> <Rd>,<Rn>,<Rm>.  */
 	    n = val1 & 0xFFFF;
 	    if (n & 0x8000)
-	      n |= -1 << 16;
+	      n |= -(1 << 16);
 
 	    m = (val2 >> 16) & 0xFFFF;
 	    if (m & 0x8000)
-	      m |= -1 << 16;
+	      m |= -(1 << 16);
 
 	    r = n - m;
 
@@ -388,11 +388,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
 
 	    n = (val1 >> 16) & 0xFFFF;
 	    if (n & 0x8000)
-	      n |= -1 << 16;
+	      n |= -(1 << 16);
 
 	    m = val2 & 0xFFFF;
 	    if (m & 0x8000)
-	      m |= -1 << 16;
+	      m |= -(1 << 16);
 
 	    r = n + m;
 
@@ -407,11 +407,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
 	  case 0xF5: /* QSAX<c> <Rd>,<Rn>,<Rm>.  */
 	    n = val1 & 0xFFFF;
 	    if (n & 0x8000)
-	      n |= -1 << 16;
+	      n |= -(1 << 16);
 
 	    m = (val2 >> 16) & 0xFFFF;
 	    if (m & 0x8000)
-	      m |= -1 << 16;
+	      m |= -(1 << 16);
 
 	    r = n + m;
 
@@ -424,11 +424,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
 
 	    n = (val1 >> 16) & 0xFFFF;
 	    if (n & 0x8000)
-	      n |= -1 << 16;
+	      n |= -(1 << 16);
 
 	    m = val2 & 0xFFFF;
 	    if (m & 0x8000)
-	      m |= -1 << 16;
+	      m |= -(1 << 16);
 
 	    r = n - m;
 
@@ -447,11 +447,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
 	      {
 		n = (val1 >> i) & 0xFFFF;
 		if (n & 0x8000)
-		  n |= -1 << 16;
+		  n |= -(1 << 16);
 
 		m = (val2 >> i) & 0xFFFF;
 		if (m & 0x8000)
-		  m |= -1 << 16;
+		  m |= -(1 << 16);
 
 		r = n - m;
 
@@ -471,11 +471,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
 	      {
 		n = (val1 >> i) & 0xFF;
 		if (n & 0x80)
-		  n |= -1 << 8;
+		  n |= - (1 << 8);
 
 		m = (val2 >> i) & 0xFF;
 		if (m & 0x80)
-		  m |= -1 << 8;
+		  m |= - (1 << 8);
 
 		r = n + m;
 
@@ -495,11 +495,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
 	      {
 		n = (val1 >> i) & 0xFF;
 		if (n & 0x80)
-		  n |= -1 << 8;
+		  n |= - (1 << 8);
 
 		m = (val2 >> i) & 0xFF;
 		if (m & 0x80)
-		  m |= -1 << 8;
+		  m |= - (1 << 8);
 
 		r = n - m;
 
@@ -951,14 +951,14 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
 	    state->Emulate = FALSE;
 	  }
 
-	mask = -1 << lsb;
-	mask &= ~(-1 << (msb + 1));
+	mask = -(1 << lsb);
+	mask &= ~(-(1 << (msb + 1)));
 	state->Reg[Rd] &= ~ mask;
 
 	Rn = BITS (0, 3);
 	if (Rn != 0xF)
 	  {
-	    ARMword val = state->Reg[Rn] & ~(-1 << ((msb + 1) - lsb));
+	    ARMword val = state->Reg[Rn] & ~(-(1 << ((msb + 1) - lsb)));
 	    state->Reg[Rd] |= val << lsb;
 	  }
 	return 1;
@@ -1036,7 +1036,7 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
 
 	val = state->Reg[Rn];
 	val >>= lsb;
-	val &= ~(-1 << (widthm1 + 1));
+	val &= ~(-(1 << (widthm1 + 1)));
 
 	state->Reg[Rd] = val;
 	
diff --git a/sim/arm/thumbemu.c b/sim/arm/thumbemu.c
index 2d26bf6..72929c7 100644
--- a/sim/arm/thumbemu.c
+++ b/sim/arm/thumbemu.c
@@ -204,7 +204,7 @@ handle_T2_insn (ARMul_State * state,
 
 	    simm32 = (J1 << 19) | (J2 << 18) | (imm6 << 12) | (imm11 << 1);
 	    if (S)
-	      simm32 |= (-1 << 20);
+	      simm32 |= -(1 << 20);
 	    break;
 	  }
 
@@ -217,7 +217,7 @@ handle_T2_insn (ARMul_State * state,
 
 	    simm32 = (I1 << 23) | (I2 << 22) | (imm10 << 12) | (imm11 << 1);
 	    if (S)
-	      simm32 |= (-1 << 24);
+	      simm32 |= -(1 << 24);
 	    break;
 	  }
 
@@ -230,7 +230,7 @@ handle_T2_insn (ARMul_State * state,
 
 	    simm32 = (I1 << 23) | (I2 << 22) | (imm10h << 12) | (imm10l << 2);
 	    if (S)
-	      simm32 |= (-1 << 24);
+	      simm32 |= -(1 << 24);
 
 	    CLEART;
 	    state->Reg[14] = (pc + 4) | 1;
@@ -246,7 +246,7 @@ handle_T2_insn (ARMul_State * state,
 
 	    simm32 = (I1 << 23) | (I2 << 22) | (imm10 << 12) | (imm11 << 1);
 	    if (S)
-	      simm32 |= (-1 << 24);
+	      simm32 |= -(1 << 24);
 	    state->Reg[14] = (pc + 4) | 1;
 	    break;
 	  }
@@ -1078,7 +1078,7 @@ handle_T2_insn (ARMul_State * state,
 	ARMword Rn = tBITS (0, 3);
 	ARMword msbit = ntBITS (0, 5);
 	ARMword lsbit = (ntBITS (12, 14) << 2) | ntBITS (6, 7);
-	ARMword mask = -1 << lsbit;
+	ARMword mask = -(1 << lsbit);
 
 	tASSERT (tBIT (4) == 0);
 	tASSERT (ntBIT (15) == 0);
@@ -1489,7 +1489,7 @@ handle_T2_insn (ARMul_State * state,
 	
 	state->Reg[Rt] = ARMul_LoadByte (state, address);
 	if (state->Reg[Rt] & 0x80)
-	  state->Reg[Rt] |= -1 << 8;
+	  state->Reg[Rt] |= -(1 << 8);
 
 	* pvalid = t_resolved;
 	break;
@@ -1542,7 +1542,7 @@ handle_T2_insn (ARMul_State * state,
 
 	state->Reg[Rt] = ARMul_LoadHalfWord (state, address);
 	if (state->Reg[Rt] & 0x8000)
-	  state->Reg[Rt] |= -1 << 16;
+	  state->Reg[Rt] |= -(1 << 16);
 
 	* pvalid = t_branch;
 	break;
@@ -1564,7 +1564,7 @@ handle_T2_insn (ARMul_State * state,
 	    val = state->Reg[Rm];
 	    val = (val >> ror) | (val << (32 - ror));
 	    if (val & 0x8000)
-	      val |= -1 << 16;
+	      val |= -(1 << 16);
 	    state->Reg[Rd] = val;
 	  }
 	else
diff --git a/sim/avr/interp.c b/sim/avr/interp.c
index a6588d3..bdb4e42 100644
--- a/sim/avr/interp.c
+++ b/sim/avr/interp.c
@@ -231,7 +231,7 @@ static byte sram[MAX_AVR_SRAM];
 static int sign_ext (word val, int nb_bits)
 {
   if (val & (1 << (nb_bits - 1)))
-    return val | (-1 << nb_bits);
+    return val | -(1 << nb_bits);
   return val;
 }
 
diff --git a/sim/mips/micromips.igen b/sim/mips/micromips.igen
index 2c62376..f24220e 100644
--- a/sim/mips/micromips.igen
+++ b/sim/mips/micromips.igen
@@ -54,7 +54,7 @@
 :function:::address_word:process_isa_mode:address_word target
 {
   SD->isa_mode = target & 0x1;
-  return (target & (-1 << 1));
+  return (target & (-(1 << 1)));
 }
 
 :function:::address_word:do_micromips_jalr:int rt, int rs, address_word nia, int delayslot_instruction_size
diff --git a/sim/msp430/msp430-sim.c b/sim/msp430/msp430-sim.c
index f32cb69..6a7effa 100644
--- a/sim/msp430/msp430-sim.c
+++ b/sim/msp430/msp430-sim.c
@@ -366,7 +366,7 @@ get_op (SIM_DESC sd, MSP430_Opcode_Decoded *opc, int n)
 
 	  /* Index values are signed.  */
 	  if (addr & (1 << (sign - 1)))
-	    addr |= -1 << sign;
+	    addr |= -(1 << sign);
 
 	  addr += reg;
 
@@ -559,7 +559,7 @@ put_op (SIM_DESC sd, MSP430_Opcode_Decoded *opc, int n, int val)
 
 	  /* Index values are signed.  */
 	  if (addr & (1 << (sign - 1)))
-	    addr |= -1 << sign;
+	    addr |= -(1 << sign);
 
 	  addr += reg;
 
diff --git a/sim/v850/simops.c b/sim/v850/simops.c
index b8b3856..40d578e 100644
--- a/sim/v850/simops.c
+++ b/sim/v850/simops.c
@@ -3317,7 +3317,7 @@ v850_bins (SIM_DESC sd, unsigned int source, unsigned int lsb, unsigned int msb,
   pos = lsb;
   width = (msb - lsb) + 1;
 
-  mask = ~ (-1 << width);
+  mask = ~ (-(1 << width));
   source &= mask;
   mask <<= pos;
   result = (* dest) & ~ mask;
-- 
2.3.0

Re: [PATCH 3/2] Fix invalid left shift of negative value.

[Kevin Buettner]

On Tue, 17 Nov 2015 16:09:57 +0100
Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:

> And another one:
> 
> On Tue, Nov 10, 2015 at 12:16:38PM +0100, Dominik Vogt wrote:
> > The following series of patches fixes all occurences of
> > left-shifting negative constants in C code which is undefined by
> > the C standard.  The patches have been tested on s390x, covering
> > only a small subset of the changes.
> 
> Changes in sim/.

LGTM.

However...

I can't approve changes to sim/.  Please wait for Mike Frysinger (for
whom I've added a CC) to approve this change.

Thanks,

Kevin

Re: [PATCH 3/2] Fix invalid left shift of negative value.

[Dominik Vogt]

On Tue, Nov 17, 2015 at 04:09:57PM +0100, Dominik Vogt wrote:
> And another one:
> 
> On Tue, Nov 10, 2015 at 12:16:38PM +0100, Dominik Vogt wrote:
> > The following series of patches fixes all occurences of
> > left-shifting negative constants in C code which is undefined by
> > the C standard.  The patches have been tested on s390x, covering
> > only a small subset of the changes.
> 
> Changes in sim/.

Ping.

> sim/arm/ChangeLog
> 
> 	* thumbemu.c (handle_T2_insn): Fix left shift of negative value.
> 	* armemu.c (handle_v6_insn): Likewise.
> 
> sim/avr/ChangeLog
> 
> 	* interp.c (sign_ext): Fix left shift of negative value.
> 
> sim/mips/ChangeLog
> 
> 	* micromips.igen (process_isa_mode): Fix left shift of negative value.
> 
> sim/msp430/ChangeLog
> 
> 	* msp430-sim.c (get_op, put_op): Fix left shift of negative value.
> 
> sim/v850/ChangeLog
> 
> 	* simops.c (v850_bins): Fix left shift of negative value.

> >From 5855e92b06a55ec2cba580788361fbbcc0f1c754 Mon Sep 17 00:00:00 2001
> From: Dominik Vogt <vogt@linux.vnet.ibm.com>
> Date: Fri, 30 Oct 2015 15:19:28 +0100
> Subject: [PATCH 3/2] sim: Fix left shift of negative value.
> 
> ---
>  sim/arm/armemu.c        | 40 ++++++++++++++++++++--------------------
>  sim/arm/thumbemu.c      | 16 ++++++++--------
>  sim/avr/interp.c        |  2 +-
>  sim/mips/micromips.igen |  2 +-
>  sim/msp430/msp430-sim.c |  4 ++--
>  sim/v850/simops.c       |  2 +-
>  6 files changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/sim/arm/armemu.c b/sim/arm/armemu.c
> index f2a84eb..3826c78 100644
> --- a/sim/arm/armemu.c
> +++ b/sim/arm/armemu.c
> @@ -351,11 +351,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
>  	      {
>  		n = (val1 >> i) & 0xFFFF;
>  		if (n & 0x8000)
> -		  n |= -1 << 16;
> +		  n |= -(1 << 16);
>  
>  		m = (val2 >> i) & 0xFFFF;
>  		if (m & 0x8000)
> -		  m |= -1 << 16;
> +		  m |= -(1 << 16);
>  
>  		r = n + m;
>  
> @@ -371,11 +371,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
>  	  case 0xF3: /* QASX<c> <Rd>,<Rn>,<Rm>.  */
>  	    n = val1 & 0xFFFF;
>  	    if (n & 0x8000)
> -	      n |= -1 << 16;
> +	      n |= -(1 << 16);
>  
>  	    m = (val2 >> 16) & 0xFFFF;
>  	    if (m & 0x8000)
> -	      m |= -1 << 16;
> +	      m |= -(1 << 16);
>  
>  	    r = n - m;
>  
> @@ -388,11 +388,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
>  
>  	    n = (val1 >> 16) & 0xFFFF;
>  	    if (n & 0x8000)
> -	      n |= -1 << 16;
> +	      n |= -(1 << 16);
>  
>  	    m = val2 & 0xFFFF;
>  	    if (m & 0x8000)
> -	      m |= -1 << 16;
> +	      m |= -(1 << 16);
>  
>  	    r = n + m;
>  
> @@ -407,11 +407,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
>  	  case 0xF5: /* QSAX<c> <Rd>,<Rn>,<Rm>.  */
>  	    n = val1 & 0xFFFF;
>  	    if (n & 0x8000)
> -	      n |= -1 << 16;
> +	      n |= -(1 << 16);
>  
>  	    m = (val2 >> 16) & 0xFFFF;
>  	    if (m & 0x8000)
> -	      m |= -1 << 16;
> +	      m |= -(1 << 16);
>  
>  	    r = n + m;
>  
> @@ -424,11 +424,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
>  
>  	    n = (val1 >> 16) & 0xFFFF;
>  	    if (n & 0x8000)
> -	      n |= -1 << 16;
> +	      n |= -(1 << 16);
>  
>  	    m = val2 & 0xFFFF;
>  	    if (m & 0x8000)
> -	      m |= -1 << 16;
> +	      m |= -(1 << 16);
>  
>  	    r = n - m;
>  
> @@ -447,11 +447,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
>  	      {
>  		n = (val1 >> i) & 0xFFFF;
>  		if (n & 0x8000)
> -		  n |= -1 << 16;
> +		  n |= -(1 << 16);
>  
>  		m = (val2 >> i) & 0xFFFF;
>  		if (m & 0x8000)
> -		  m |= -1 << 16;
> +		  m |= -(1 << 16);
>  
>  		r = n - m;
>  
> @@ -471,11 +471,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
>  	      {
>  		n = (val1 >> i) & 0xFF;
>  		if (n & 0x80)
> -		  n |= -1 << 8;
> +		  n |= - (1 << 8);
>  
>  		m = (val2 >> i) & 0xFF;
>  		if (m & 0x80)
> -		  m |= -1 << 8;
> +		  m |= - (1 << 8);
>  
>  		r = n + m;
>  
> @@ -495,11 +495,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
>  	      {
>  		n = (val1 >> i) & 0xFF;
>  		if (n & 0x80)
> -		  n |= -1 << 8;
> +		  n |= - (1 << 8);
>  
>  		m = (val2 >> i) & 0xFF;
>  		if (m & 0x80)
> -		  m |= -1 << 8;
> +		  m |= - (1 << 8);
>  
>  		r = n - m;
>  
> @@ -951,14 +951,14 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
>  	    state->Emulate = FALSE;
>  	  }
>  
> -	mask = -1 << lsb;
> -	mask &= ~(-1 << (msb + 1));
> +	mask = -(1 << lsb);
> +	mask &= ~(-(1 << (msb + 1)));
>  	state->Reg[Rd] &= ~ mask;
>  
>  	Rn = BITS (0, 3);
>  	if (Rn != 0xF)
>  	  {
> -	    ARMword val = state->Reg[Rn] & ~(-1 << ((msb + 1) - lsb));
> +	    ARMword val = state->Reg[Rn] & ~(-(1 << ((msb + 1) - lsb)));
>  	    state->Reg[Rd] |= val << lsb;
>  	  }
>  	return 1;
> @@ -1036,7 +1036,7 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
>  
>  	val = state->Reg[Rn];
>  	val >>= lsb;
> -	val &= ~(-1 << (widthm1 + 1));
> +	val &= ~(-(1 << (widthm1 + 1)));
>  
>  	state->Reg[Rd] = val;
>  	
> diff --git a/sim/arm/thumbemu.c b/sim/arm/thumbemu.c
> index 2d26bf6..72929c7 100644
> --- a/sim/arm/thumbemu.c
> +++ b/sim/arm/thumbemu.c
> @@ -204,7 +204,7 @@ handle_T2_insn (ARMul_State * state,
>  
>  	    simm32 = (J1 << 19) | (J2 << 18) | (imm6 << 12) | (imm11 << 1);
>  	    if (S)
> -	      simm32 |= (-1 << 20);
> +	      simm32 |= -(1 << 20);
>  	    break;
>  	  }
>  
> @@ -217,7 +217,7 @@ handle_T2_insn (ARMul_State * state,
>  
>  	    simm32 = (I1 << 23) | (I2 << 22) | (imm10 << 12) | (imm11 << 1);
>  	    if (S)
> -	      simm32 |= (-1 << 24);
> +	      simm32 |= -(1 << 24);
>  	    break;
>  	  }
>  
> @@ -230,7 +230,7 @@ handle_T2_insn (ARMul_State * state,
>  
>  	    simm32 = (I1 << 23) | (I2 << 22) | (imm10h << 12) | (imm10l << 2);
>  	    if (S)
> -	      simm32 |= (-1 << 24);
> +	      simm32 |= -(1 << 24);
>  
>  	    CLEART;
>  	    state->Reg[14] = (pc + 4) | 1;
> @@ -246,7 +246,7 @@ handle_T2_insn (ARMul_State * state,
>  
>  	    simm32 = (I1 << 23) | (I2 << 22) | (imm10 << 12) | (imm11 << 1);
>  	    if (S)
> -	      simm32 |= (-1 << 24);
> +	      simm32 |= -(1 << 24);
>  	    state->Reg[14] = (pc + 4) | 1;
>  	    break;
>  	  }
> @@ -1078,7 +1078,7 @@ handle_T2_insn (ARMul_State * state,
>  	ARMword Rn = tBITS (0, 3);
>  	ARMword msbit = ntBITS (0, 5);
>  	ARMword lsbit = (ntBITS (12, 14) << 2) | ntBITS (6, 7);
> -	ARMword mask = -1 << lsbit;
> +	ARMword mask = -(1 << lsbit);
>  
>  	tASSERT (tBIT (4) == 0);
>  	tASSERT (ntBIT (15) == 0);
> @@ -1489,7 +1489,7 @@ handle_T2_insn (ARMul_State * state,
>  	
>  	state->Reg[Rt] = ARMul_LoadByte (state, address);
>  	if (state->Reg[Rt] & 0x80)
> -	  state->Reg[Rt] |= -1 << 8;
> +	  state->Reg[Rt] |= -(1 << 8);
>  
>  	* pvalid = t_resolved;
>  	break;
> @@ -1542,7 +1542,7 @@ handle_T2_insn (ARMul_State * state,
>  
>  	state->Reg[Rt] = ARMul_LoadHalfWord (state, address);
>  	if (state->Reg[Rt] & 0x8000)
> -	  state->Reg[Rt] |= -1 << 16;
> +	  state->Reg[Rt] |= -(1 << 16);
>  
>  	* pvalid = t_branch;
>  	break;
> @@ -1564,7 +1564,7 @@ handle_T2_insn (ARMul_State * state,
>  	    val = state->Reg[Rm];
>  	    val = (val >> ror) | (val << (32 - ror));
>  	    if (val & 0x8000)
> -	      val |= -1 << 16;
> +	      val |= -(1 << 16);
>  	    state->Reg[Rd] = val;
>  	  }
>  	else
> diff --git a/sim/avr/interp.c b/sim/avr/interp.c
> index a6588d3..bdb4e42 100644
> --- a/sim/avr/interp.c
> +++ b/sim/avr/interp.c
> @@ -231,7 +231,7 @@ static byte sram[MAX_AVR_SRAM];
>  static int sign_ext (word val, int nb_bits)
>  {
>    if (val & (1 << (nb_bits - 1)))
> -    return val | (-1 << nb_bits);
> +    return val | -(1 << nb_bits);
>    return val;
>  }
>  
> diff --git a/sim/mips/micromips.igen b/sim/mips/micromips.igen
> index 2c62376..f24220e 100644
> --- a/sim/mips/micromips.igen
> +++ b/sim/mips/micromips.igen
> @@ -54,7 +54,7 @@
>  :function:::address_word:process_isa_mode:address_word target
>  {
>    SD->isa_mode = target & 0x1;
> -  return (target & (-1 << 1));
> +  return (target & (-(1 << 1)));
>  }
>  
>  :function:::address_word:do_micromips_jalr:int rt, int rs, address_word nia, int delayslot_instruction_size
> diff --git a/sim/msp430/msp430-sim.c b/sim/msp430/msp430-sim.c
> index f32cb69..6a7effa 100644
> --- a/sim/msp430/msp430-sim.c
> +++ b/sim/msp430/msp430-sim.c
> @@ -366,7 +366,7 @@ get_op (SIM_DESC sd, MSP430_Opcode_Decoded *opc, int n)
>  
>  	  /* Index values are signed.  */
>  	  if (addr & (1 << (sign - 1)))
> -	    addr |= -1 << sign;
> +	    addr |= -(1 << sign);
>  
>  	  addr += reg;
>  
> @@ -559,7 +559,7 @@ put_op (SIM_DESC sd, MSP430_Opcode_Decoded *opc, int n, int val)
>  
>  	  /* Index values are signed.  */
>  	  if (addr & (1 << (sign - 1)))
> -	    addr |= -1 << sign;
> +	    addr |= -(1 << sign);
>  
>  	  addr += reg;
>  
> diff --git a/sim/v850/simops.c b/sim/v850/simops.c
> index b8b3856..40d578e 100644
> --- a/sim/v850/simops.c
> +++ b/sim/v850/simops.c
> @@ -3317,7 +3317,7 @@ v850_bins (SIM_DESC sd, unsigned int source, unsigned int lsb, unsigned int msb,
>    pos = lsb;
>    width = (msb - lsb) + 1;
>  
> -  mask = ~ (-1 << width);
> +  mask = ~ (-(1 << width));
>    source &= mask;
>    mask <<= pos;
>    result = (* dest) & ~ mask;
> -- 
> 2.3.0

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[SIM patch] Re: [PATCH 3/2] Fix invalid left shift of negative value.

[Joel Brobecker]

> On Tue, Nov 17, 2015 at 04:09:57PM +0100, Dominik Vogt wrote:
> > And another one:
> > 
> > On Tue, Nov 10, 2015 at 12:16:38PM +0100, Dominik Vogt wrote:
> > > The following series of patches fixes all occurences of
> > > left-shifting negative constants in C code which is undefined by
> > > the C standard.  The patches have been tested on s390x, covering
> > > only a small subset of the changes.
> > 
> > Changes in sim/.
> 
> Ping.

Sorry about the delay in answering this. I see Mike is in Cc, and
he usually reviews sim patches very quickly. It looks good to me,
but normally, Mike does the approving. Nevertheless, in the interest
of moving forward, if Mike isn't available or hasn't answered by
next Sunday, please feel free to push your patch.



> 
> > sim/arm/ChangeLog
> > 
> > 	* thumbemu.c (handle_T2_insn): Fix left shift of negative value.
> > 	* armemu.c (handle_v6_insn): Likewise.
> > 
> > sim/avr/ChangeLog
> > 
> > 	* interp.c (sign_ext): Fix left shift of negative value.
> > 
> > sim/mips/ChangeLog
> > 
> > 	* micromips.igen (process_isa_mode): Fix left shift of negative value.
> > 
> > sim/msp430/ChangeLog
> > 
> > 	* msp430-sim.c (get_op, put_op): Fix left shift of negative value.
> > 
> > sim/v850/ChangeLog
> > 
> > 	* simops.c (v850_bins): Fix left shift of negative value.
> 
> > >From 5855e92b06a55ec2cba580788361fbbcc0f1c754 Mon Sep 17 00:00:00 2001
> > From: Dominik Vogt <vogt@linux.vnet.ibm.com>
> > Date: Fri, 30 Oct 2015 15:19:28 +0100
> > Subject: [PATCH 3/2] sim: Fix left shift of negative value.
> > 
> > ---
> >  sim/arm/armemu.c        | 40 ++++++++++++++++++++--------------------
> >  sim/arm/thumbemu.c      | 16 ++++++++--------
> >  sim/avr/interp.c        |  2 +-
> >  sim/mips/micromips.igen |  2 +-
> >  sim/msp430/msp430-sim.c |  4 ++--
> >  sim/v850/simops.c       |  2 +-
> >  6 files changed, 33 insertions(+), 33 deletions(-)
> > 
> > diff --git a/sim/arm/armemu.c b/sim/arm/armemu.c
> > index f2a84eb..3826c78 100644
> > --- a/sim/arm/armemu.c
> > +++ b/sim/arm/armemu.c
> > @@ -351,11 +351,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
> >  	      {
> >  		n = (val1 >> i) & 0xFFFF;
> >  		if (n & 0x8000)
> > -		  n |= -1 << 16;
> > +		  n |= -(1 << 16);
> >  
> >  		m = (val2 >> i) & 0xFFFF;
> >  		if (m & 0x8000)
> > -		  m |= -1 << 16;
> > +		  m |= -(1 << 16);
> >  
> >  		r = n + m;
> >  
> > @@ -371,11 +371,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
> >  	  case 0xF3: /* QASX<c> <Rd>,<Rn>,<Rm>.  */
> >  	    n = val1 & 0xFFFF;
> >  	    if (n & 0x8000)
> > -	      n |= -1 << 16;
> > +	      n |= -(1 << 16);
> >  
> >  	    m = (val2 >> 16) & 0xFFFF;
> >  	    if (m & 0x8000)
> > -	      m |= -1 << 16;
> > +	      m |= -(1 << 16);
> >  
> >  	    r = n - m;
> >  
> > @@ -388,11 +388,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
> >  
> >  	    n = (val1 >> 16) & 0xFFFF;
> >  	    if (n & 0x8000)
> > -	      n |= -1 << 16;
> > +	      n |= -(1 << 16);
> >  
> >  	    m = val2 & 0xFFFF;
> >  	    if (m & 0x8000)
> > -	      m |= -1 << 16;
> > +	      m |= -(1 << 16);
> >  
> >  	    r = n + m;
> >  
> > @@ -407,11 +407,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
> >  	  case 0xF5: /* QSAX<c> <Rd>,<Rn>,<Rm>.  */
> >  	    n = val1 & 0xFFFF;
> >  	    if (n & 0x8000)
> > -	      n |= -1 << 16;
> > +	      n |= -(1 << 16);
> >  
> >  	    m = (val2 >> 16) & 0xFFFF;
> >  	    if (m & 0x8000)
> > -	      m |= -1 << 16;
> > +	      m |= -(1 << 16);
> >  
> >  	    r = n + m;
> >  
> > @@ -424,11 +424,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
> >  
> >  	    n = (val1 >> 16) & 0xFFFF;
> >  	    if (n & 0x8000)
> > -	      n |= -1 << 16;
> > +	      n |= -(1 << 16);
> >  
> >  	    m = val2 & 0xFFFF;
> >  	    if (m & 0x8000)
> > -	      m |= -1 << 16;
> > +	      m |= -(1 << 16);
> >  
> >  	    r = n - m;
> >  
> > @@ -447,11 +447,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
> >  	      {
> >  		n = (val1 >> i) & 0xFFFF;
> >  		if (n & 0x8000)
> > -		  n |= -1 << 16;
> > +		  n |= -(1 << 16);
> >  
> >  		m = (val2 >> i) & 0xFFFF;
> >  		if (m & 0x8000)
> > -		  m |= -1 << 16;
> > +		  m |= -(1 << 16);
> >  
> >  		r = n - m;
> >  
> > @@ -471,11 +471,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
> >  	      {
> >  		n = (val1 >> i) & 0xFF;
> >  		if (n & 0x80)
> > -		  n |= -1 << 8;
> > +		  n |= - (1 << 8);
> >  
> >  		m = (val2 >> i) & 0xFF;
> >  		if (m & 0x80)
> > -		  m |= -1 << 8;
> > +		  m |= - (1 << 8);
> >  
> >  		r = n + m;
> >  
> > @@ -495,11 +495,11 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
> >  	      {
> >  		n = (val1 >> i) & 0xFF;
> >  		if (n & 0x80)
> > -		  n |= -1 << 8;
> > +		  n |= - (1 << 8);
> >  
> >  		m = (val2 >> i) & 0xFF;
> >  		if (m & 0x80)
> > -		  m |= -1 << 8;
> > +		  m |= - (1 << 8);
> >  
> >  		r = n - m;
> >  
> > @@ -951,14 +951,14 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
> >  	    state->Emulate = FALSE;
> >  	  }
> >  
> > -	mask = -1 << lsb;
> > -	mask &= ~(-1 << (msb + 1));
> > +	mask = -(1 << lsb);
> > +	mask &= ~(-(1 << (msb + 1)));
> >  	state->Reg[Rd] &= ~ mask;
> >  
> >  	Rn = BITS (0, 3);
> >  	if (Rn != 0xF)
> >  	  {
> > -	    ARMword val = state->Reg[Rn] & ~(-1 << ((msb + 1) - lsb));
> > +	    ARMword val = state->Reg[Rn] & ~(-(1 << ((msb + 1) - lsb)));
> >  	    state->Reg[Rd] |= val << lsb;
> >  	  }
> >  	return 1;
> > @@ -1036,7 +1036,7 @@ handle_v6_insn (ARMul_State * state, ARMword instr)
> >  
> >  	val = state->Reg[Rn];
> >  	val >>= lsb;
> > -	val &= ~(-1 << (widthm1 + 1));
> > +	val &= ~(-(1 << (widthm1 + 1)));
> >  
> >  	state->Reg[Rd] = val;
> >  	
> > diff --git a/sim/arm/thumbemu.c b/sim/arm/thumbemu.c
> > index 2d26bf6..72929c7 100644
> > --- a/sim/arm/thumbemu.c
> > +++ b/sim/arm/thumbemu.c
> > @@ -204,7 +204,7 @@ handle_T2_insn (ARMul_State * state,
> >  
> >  	    simm32 = (J1 << 19) | (J2 << 18) | (imm6 << 12) | (imm11 << 1);
> >  	    if (S)
> > -	      simm32 |= (-1 << 20);
> > +	      simm32 |= -(1 << 20);
> >  	    break;
> >  	  }
> >  
> > @@ -217,7 +217,7 @@ handle_T2_insn (ARMul_State * state,
> >  
> >  	    simm32 = (I1 << 23) | (I2 << 22) | (imm10 << 12) | (imm11 << 1);
> >  	    if (S)
> > -	      simm32 |= (-1 << 24);
> > +	      simm32 |= -(1 << 24);
> >  	    break;
> >  	  }
> >  
> > @@ -230,7 +230,7 @@ handle_T2_insn (ARMul_State * state,
> >  
> >  	    simm32 = (I1 << 23) | (I2 << 22) | (imm10h << 12) | (imm10l << 2);
> >  	    if (S)
> > -	      simm32 |= (-1 << 24);
> > +	      simm32 |= -(1 << 24);
> >  
> >  	    CLEART;
> >  	    state->Reg[14] = (pc + 4) | 1;
> > @@ -246,7 +246,7 @@ handle_T2_insn (ARMul_State * state,
> >  
> >  	    simm32 = (I1 << 23) | (I2 << 22) | (imm10 << 12) | (imm11 << 1);
> >  	    if (S)
> > -	      simm32 |= (-1 << 24);
> > +	      simm32 |= -(1 << 24);
> >  	    state->Reg[14] = (pc + 4) | 1;
> >  	    break;
> >  	  }
> > @@ -1078,7 +1078,7 @@ handle_T2_insn (ARMul_State * state,
> >  	ARMword Rn = tBITS (0, 3);
> >  	ARMword msbit = ntBITS (0, 5);
> >  	ARMword lsbit = (ntBITS (12, 14) << 2) | ntBITS (6, 7);
> > -	ARMword mask = -1 << lsbit;
> > +	ARMword mask = -(1 << lsbit);
> >  
> >  	tASSERT (tBIT (4) == 0);
> >  	tASSERT (ntBIT (15) == 0);
> > @@ -1489,7 +1489,7 @@ handle_T2_insn (ARMul_State * state,
> >  	
> >  	state->Reg[Rt] = ARMul_LoadByte (state, address);
> >  	if (state->Reg[Rt] & 0x80)
> > -	  state->Reg[Rt] |= -1 << 8;
> > +	  state->Reg[Rt] |= -(1 << 8);
> >  
> >  	* pvalid = t_resolved;
> >  	break;
> > @@ -1542,7 +1542,7 @@ handle_T2_insn (ARMul_State * state,
> >  
> >  	state->Reg[Rt] = ARMul_LoadHalfWord (state, address);
> >  	if (state->Reg[Rt] & 0x8000)
> > -	  state->Reg[Rt] |= -1 << 16;
> > +	  state->Reg[Rt] |= -(1 << 16);
> >  
> >  	* pvalid = t_branch;
> >  	break;
> > @@ -1564,7 +1564,7 @@ handle_T2_insn (ARMul_State * state,
> >  	    val = state->Reg[Rm];
> >  	    val = (val >> ror) | (val << (32 - ror));
> >  	    if (val & 0x8000)
> > -	      val |= -1 << 16;
> > +	      val |= -(1 << 16);
> >  	    state->Reg[Rd] = val;
> >  	  }
> >  	else
> > diff --git a/sim/avr/interp.c b/sim/avr/interp.c
> > index a6588d3..bdb4e42 100644
> > --- a/sim/avr/interp.c
> > +++ b/sim/avr/interp.c
> > @@ -231,7 +231,7 @@ static byte sram[MAX_AVR_SRAM];
> >  static int sign_ext (word val, int nb_bits)
> >  {
> >    if (val & (1 << (nb_bits - 1)))
> > -    return val | (-1 << nb_bits);
> > +    return val | -(1 << nb_bits);
> >    return val;
> >  }
> >  
> > diff --git a/sim/mips/micromips.igen b/sim/mips/micromips.igen
> > index 2c62376..f24220e 100644
> > --- a/sim/mips/micromips.igen
> > +++ b/sim/mips/micromips.igen
> > @@ -54,7 +54,7 @@
> >  :function:::address_word:process_isa_mode:address_word target
> >  {
> >    SD->isa_mode = target & 0x1;
> > -  return (target & (-1 << 1));
> > +  return (target & (-(1 << 1)));
> >  }
> >  
> >  :function:::address_word:do_micromips_jalr:int rt, int rs, address_word nia, int delayslot_instruction_size
> > diff --git a/sim/msp430/msp430-sim.c b/sim/msp430/msp430-sim.c
> > index f32cb69..6a7effa 100644
> > --- a/sim/msp430/msp430-sim.c
> > +++ b/sim/msp430/msp430-sim.c
> > @@ -366,7 +366,7 @@ get_op (SIM_DESC sd, MSP430_Opcode_Decoded *opc, int n)
> >  
> >  	  /* Index values are signed.  */
> >  	  if (addr & (1 << (sign - 1)))
> > -	    addr |= -1 << sign;
> > +	    addr |= -(1 << sign);
> >  
> >  	  addr += reg;
> >  
> > @@ -559,7 +559,7 @@ put_op (SIM_DESC sd, MSP430_Opcode_Decoded *opc, int n, int val)
> >  
> >  	  /* Index values are signed.  */
> >  	  if (addr & (1 << (sign - 1)))
> > -	    addr |= -1 << sign;
> > +	    addr |= -(1 << sign);
> >  
> >  	  addr += reg;
> >  
> > diff --git a/sim/v850/simops.c b/sim/v850/simops.c
> > index b8b3856..40d578e 100644
> > --- a/sim/v850/simops.c
> > +++ b/sim/v850/simops.c
> > @@ -3317,7 +3317,7 @@ v850_bins (SIM_DESC sd, unsigned int source, unsigned int lsb, unsigned int msb,
> >    pos = lsb;
> >    width = (msb - lsb) + 1;
> >  
> > -  mask = ~ (-1 << width);
> > +  mask = ~ (-(1 << width));
> >    source &= mask;
> >    mask <<= pos;
> >    result = (* dest) & ~ mask;
> > -- 
> > 2.3.0

-- 
Joel

Re: [SIM patch] Re: [PATCH 3/2] Fix invalid left shift of negative value.

[Andreas Arnez]

On Sun, Dec 06 2015, Joel Brobecker wrote:

>> On Tue, Nov 17, 2015 at 04:09:57PM +0100, Dominik Vogt wrote:
>> > And another one:
>> > 
>> > On Tue, Nov 10, 2015 at 12:16:38PM +0100, Dominik Vogt wrote:
>> > > The following series of patches fixes all occurences of
>> > > left-shifting negative constants in C code which is undefined by
>> > > the C standard.  The patches have been tested on s390x, covering
>> > > only a small subset of the changes.
>> > 
>> > Changes in sim/.
>> 
>> Ping.
>
> Sorry about the delay in answering this. I see Mike is in Cc, and
> he usually reviews sim patches very quickly. It looks good to me,
> but normally, Mike does the approving. Nevertheless, in the interest
> of moving forward, if Mike isn't available or hasn't answered by
> next Sunday, please feel free to push your patch.

As suggested, I've pushed this now.