* [PATCH 0/2] Fix invalid left shift of negative value. @ 2015-11-10 11:16 Dominik Vogt 2015-11-10 11:17 ` [PATCH 1/2] " Dominik Vogt ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Dominik Vogt @ 2015-11-10 11:16 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] Fix invalid left shift of negative value. 2015-11-10 11:16 [PATCH 0/2] Fix invalid left shift of negative value Dominik Vogt @ 2015-11-10 11:17 ` Dominik Vogt 2015-11-10 11:18 ` [PATCH 2/2] " Dominik Vogt ` (2 subsequent siblings) 3 siblings, 0 replies; 14+ messages in thread From: Dominik Vogt @ 2015-11-10 11:17 UTC (permalink / raw) To: gdb-patches [-- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] Fix invalid left shift of negative value. 2015-11-10 11:16 [PATCH 0/2] Fix invalid left shift of negative value Dominik Vogt 2015-11-10 11:17 ` [PATCH 1/2] " Dominik Vogt @ 2015-11-10 11:18 ` Dominik Vogt 2015-11-10 22:42 ` [PATCH 0/2] " Kevin Buettner 2015-11-17 15:10 ` [PATCH 3/2] " Dominik Vogt 3 siblings, 0 replies; 14+ messages in thread From: Dominik Vogt @ 2015-11-10 11:18 UTC (permalink / raw) To: gdb-patches [-- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Fix invalid left shift of negative value. 2015-11-10 11:16 [PATCH 0/2] Fix invalid left shift of negative value Dominik Vogt 2015-11-10 11:17 ` [PATCH 1/2] " Dominik Vogt 2015-11-10 11:18 ` [PATCH 2/2] " Dominik Vogt @ 2015-11-10 22:42 ` Kevin Buettner 2015-11-11 17:23 ` Ulrich Weigand 2015-11-17 15:10 ` [PATCH 3/2] " Dominik Vogt 3 siblings, 1 reply; 14+ messages in thread From: Kevin Buettner @ 2015-11-10 22:42 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Fix invalid left shift of negative value. 2015-11-10 22:42 ` [PATCH 0/2] " Kevin Buettner @ 2015-11-11 17:23 ` Ulrich Weigand 2015-11-11 19:27 ` Kevin Buettner 0 siblings, 1 reply; 14+ messages in thread From: Ulrich Weigand @ 2015-11-11 17:23 UTC (permalink / raw) To: Kevin Buettner; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Fix invalid left shift of negative value. 2015-11-11 17:23 ` Ulrich Weigand @ 2015-11-11 19:27 ` Kevin Buettner 2015-11-17 5:09 ` Kevin Buettner 0 siblings, 1 reply; 14+ messages in thread From: Kevin Buettner @ 2015-11-11 19:27 UTC (permalink / raw) To: Ulrich Weigand; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Fix invalid left shift of negative value. 2015-11-11 19:27 ` Kevin Buettner @ 2015-11-17 5:09 ` Kevin Buettner 2015-11-17 14:13 ` Pedro Alves 0 siblings, 1 reply; 14+ messages in thread From: Kevin Buettner @ 2015-11-17 5:09 UTC (permalink / raw) To: gdb-patches; +Cc: Dominik Vogt 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Fix invalid left shift of negative value. 2015-11-17 5:09 ` Kevin Buettner @ 2015-11-17 14:13 ` Pedro Alves 2015-11-17 17:33 ` Paul_Koning 0 siblings, 1 reply; 14+ messages in thread From: Pedro Alves @ 2015-11-17 14:13 UTC (permalink / raw) To: Kevin Buettner, gdb-patches; +Cc: Dominik Vogt 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Fix invalid left shift of negative value. 2015-11-17 14:13 ` Pedro Alves @ 2015-11-17 17:33 ` Paul_Koning 0 siblings, 0 replies; 14+ messages in thread From: Paul_Koning @ 2015-11-17 17:33 UTC (permalink / raw) To: palves; +Cc: kevinb, gdb-patches, vogt > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/2] Fix invalid left shift of negative value. 2015-11-10 11:16 [PATCH 0/2] Fix invalid left shift of negative value Dominik Vogt ` (2 preceding siblings ...) 2015-11-10 22:42 ` [PATCH 0/2] " Kevin Buettner @ 2015-11-17 15:10 ` Dominik Vogt 2015-11-17 17:49 ` Kevin Buettner 2015-11-30 8:54 ` Dominik Vogt 3 siblings, 2 replies; 14+ messages in thread From: Dominik Vogt @ 2015-11-17 15:10 UTC (permalink / raw) To: gdb-patches [-- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/2] Fix invalid left shift of negative value. 2015-11-17 15:10 ` [PATCH 3/2] " Dominik Vogt @ 2015-11-17 17:49 ` Kevin Buettner 2015-11-30 8:54 ` Dominik Vogt 1 sibling, 0 replies; 14+ messages in thread From: Kevin Buettner @ 2015-11-17 17:49 UTC (permalink / raw) To: gdb-patches; +Cc: Mike Frysinger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/2] Fix invalid left shift of negative value. 2015-11-17 15:10 ` [PATCH 3/2] " Dominik Vogt 2015-11-17 17:49 ` Kevin Buettner @ 2015-11-30 8:54 ` Dominik Vogt 2015-12-06 14:17 ` [SIM patch] " Joel Brobecker 1 sibling, 1 reply; 14+ messages in thread From: Dominik Vogt @ 2015-11-30 8:54 UTC (permalink / raw) To: gdb-patches; +Cc: Mike Frysinger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [SIM patch] Re: [PATCH 3/2] Fix invalid left shift of negative value. 2015-11-30 8:54 ` Dominik Vogt @ 2015-12-06 14:17 ` Joel Brobecker 2015-12-15 13:15 ` Andreas Arnez 0 siblings, 1 reply; 14+ messages in thread From: Joel Brobecker @ 2015-12-06 14:17 UTC (permalink / raw) To: Dominik Vogt; +Cc: gdb-patches, Mike Frysinger > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [SIM patch] Re: [PATCH 3/2] Fix invalid left shift of negative value. 2015-12-06 14:17 ` [SIM patch] " Joel Brobecker @ 2015-12-15 13:15 ` Andreas Arnez 0 siblings, 0 replies; 14+ messages in thread From: Andreas Arnez @ 2015-12-15 13:15 UTC (permalink / raw) To: Joel Brobecker; +Cc: Dominik Vogt, gdb-patches, Mike Frysinger 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-12-15 13:15 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-10 11:16 [PATCH 0/2] Fix invalid left shift of negative value Dominik Vogt 2015-11-10 11:17 ` [PATCH 1/2] " Dominik Vogt 2015-11-10 11:18 ` [PATCH 2/2] " Dominik Vogt 2015-11-10 22:42 ` [PATCH 0/2] " Kevin Buettner 2015-11-11 17:23 ` Ulrich Weigand 2015-11-11 19:27 ` Kevin Buettner 2015-11-17 5:09 ` Kevin Buettner 2015-11-17 14:13 ` Pedro Alves 2015-11-17 17:33 ` Paul_Koning 2015-11-17 15:10 ` [PATCH 3/2] " Dominik Vogt 2015-11-17 17:49 ` Kevin Buettner 2015-11-30 8:54 ` Dominik Vogt 2015-12-06 14:17 ` [SIM patch] " Joel Brobecker 2015-12-15 13:15 ` Andreas Arnez
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).