| Message ID | 1447171055-29567-4-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/11/2015 16:57, Stefan Hajnoczi wrote: > clang 3.7.0 on x86_64 warns about the following: > > disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value] > imm |= (-1 << 7); > ~~ ^ > > Note that this patch preserves the tab indent in this source file > because the surrounding code still uses tabs. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> I would like to know a case where (except with ubsan) clang actually uses the optimization. If not, this is just error message theatre (which is not news for clang) and shouldn't have been part of -Wall. Paolo
On 10 November 2015 at 17:33, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 10/11/2015 16:57, Stefan Hajnoczi wrote: >> clang 3.7.0 on x86_64 warns about the following: >> >> disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value] >> imm |= (-1 << 7); >> ~~ ^ >> >> Note that this patch preserves the tab indent in this source file >> because the surrounding code still uses tabs. >> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > I would like to know a case where (except with ubsan) clang actually > uses the optimization. > > If not, this is just error message theatre (which is not news for clang) > and shouldn't have been part of -Wall. It could be they're attempting to warn us now about the possibility that in a future version of clang they will start using this UB to optimize with. http://stackoverflow.com/questions/22883790/left-shift-of-negative-values reports that Intel's ICC will use this in dead-code-elimination optimization. One day clang might do that too. thanks -- PMM
On 10/11/2015 18:48, Peter Maydell wrote: > It could be they're attempting to warn us now about the possibility > that in a future version of clang they will start using this UB > to optimize with. Good luck to them. Paolo
Peter Maydell <peter.maydell@linaro.org> writes: > On 10 November 2015 at 17:33, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 10/11/2015 16:57, Stefan Hajnoczi wrote: >>> clang 3.7.0 on x86_64 warns about the following: >>> >>> disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value] >>> imm |= (-1 << 7); >>> ~~ ^ >>> >>> Note that this patch preserves the tab indent in this source file >>> because the surrounding code still uses tabs. >>> >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >> >> I would like to know a case where (except with ubsan) clang actually >> uses the optimization. >> >> If not, this is just error message theatre (which is not news for clang) >> and shouldn't have been part of -Wall. > > It could be they're attempting to warn us now about the possibility > that in a future version of clang they will start using this UB > to optimize with. > > http://stackoverflow.com/questions/22883790/left-shift-of-negative-values > reports that Intel's ICC will use this in dead-code-elimination > optimization. One day clang might do that too. Nice example of a compiler being gratuitously nasty.
On Tue, Nov 10, 2015 at 10:52 AM, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On 10 November 2015 at 17:33, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> >>> >>> On 10/11/2015 16:57, Stefan Hajnoczi wrote: >>>> clang 3.7.0 on x86_64 warns about the following: >>>> >>>> disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value] >>>> imm |= (-1 << 7); >>>> ~~ ^ >>>> >>>> Note that this patch preserves the tab indent in this source file >>>> because the surrounding code still uses tabs. >>>> >>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>> >>> I would like to know a case where (except with ubsan) clang actually >>> uses the optimization. >>> >>> If not, this is just error message theatre (which is not news for clang) >>> and shouldn't have been part of -Wall. >> >> It could be they're attempting to warn us now about the possibility >> that in a future version of clang they will start using this UB >> to optimize with. >> >> http://stackoverflow.com/questions/22883790/left-shift-of-negative-values >> reports that Intel's ICC will use this in dead-code-elimination >> optimization. One day clang might do that too. > > Nice example of a compiler being gratuitously nasty. > I don't read this warning as "clang will do crazy things with your code eventually". Clang has always been very verbose when it comes to undefined behavior, and I don't think that's really a bad thing to do. Even if clang does emit sane code for it, all bets are off for other compilers -- so it's more of a portability warning. And I know some other compilers *won't* warn before doing crazy things in the name of undefined behavior. The ICC example is a fine one... In my experience fixing the warnings produced by clang has actually eliminated bugs that were present but undiscovered on other platforms.
Steven Noonan <steven@uplinklabs.net> writes: > On Tue, Nov 10, 2015 at 10:52 AM, Markus Armbruster <armbru@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> On 10 November 2015 at 17:33, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> >>>> >>>> On 10/11/2015 16:57, Stefan Hajnoczi wrote: >>>>> clang 3.7.0 on x86_64 warns about the following: >>>>> >>>>> disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value] >>>>> imm |= (-1 << 7); >>>>> ~~ ^ >>>>> >>>>> Note that this patch preserves the tab indent in this source file >>>>> because the surrounding code still uses tabs. >>>>> >>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>>> >>>> I would like to know a case where (except with ubsan) clang actually >>>> uses the optimization. >>>> >>>> If not, this is just error message theatre (which is not news for clang) >>>> and shouldn't have been part of -Wall. >>> >>> It could be they're attempting to warn us now about the possibility >>> that in a future version of clang they will start using this UB >>> to optimize with. >>> >>> http://stackoverflow.com/questions/22883790/left-shift-of-negative-values >>> reports that Intel's ICC will use this in dead-code-elimination >>> optimization. One day clang might do that too. >> >> Nice example of a compiler being gratuitously nasty. >> > > I don't read this warning as "clang will do crazy things with your > code eventually". Clang has always been very verbose when it comes to > undefined behavior, and I don't think that's really a bad thing to do. [...] Misunderstanding? Clang's warning is at worst annoying, but nowhere near nasty. ICC concluding that code executing a left shift of a negative value must be unreachable is gratuitously nasty.
On Tue, Nov 10, 2015 at 12:06 PM, Markus Armbruster <armbru@redhat.com> wrote: > Steven Noonan <steven@uplinklabs.net> writes: > >> On Tue, Nov 10, 2015 at 10:52 AM, Markus Armbruster <armbru@redhat.com> wrote: >>> Peter Maydell <peter.maydell@linaro.org> writes: >>> >>>> On 10 November 2015 at 17:33, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>>> >>>>> >>>>> On 10/11/2015 16:57, Stefan Hajnoczi wrote: >>>>>> clang 3.7.0 on x86_64 warns about the following: >>>>>> >>>>>> disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value] >>>>>> imm |= (-1 << 7); >>>>>> ~~ ^ >>>>>> >>>>>> Note that this patch preserves the tab indent in this source file >>>>>> because the surrounding code still uses tabs. >>>>>> >>>>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>>>> >>>>> I would like to know a case where (except with ubsan) clang actually >>>>> uses the optimization. >>>>> >>>>> If not, this is just error message theatre (which is not news for clang) >>>>> and shouldn't have been part of -Wall. >>>> >>>> It could be they're attempting to warn us now about the possibility >>>> that in a future version of clang they will start using this UB >>>> to optimize with. >>>> >>>> http://stackoverflow.com/questions/22883790/left-shift-of-negative-values >>>> reports that Intel's ICC will use this in dead-code-elimination >>>> optimization. One day clang might do that too. >>> >>> Nice example of a compiler being gratuitously nasty. >>> >> >> I don't read this warning as "clang will do crazy things with your >> code eventually". Clang has always been very verbose when it comes to >> undefined behavior, and I don't think that's really a bad thing to do. > [...] > > Misunderstanding? > > Clang's warning is at worst annoying, but nowhere near nasty. Sure, it's merely annoying if you only compile your code with Clang. But my point is that a lot of Clang's aggressive undefined behavior warnings are there to get people to write portable code so it doesn't break on other compilers where the "undefined behavior" depends on the compiler implementation. > ICC concluding that code executing a left shift of a negative value must > be unreachable is gratuitously nasty. I agree, that's completely insane.
On 10/11/2015 20:51, Steven Noonan wrote: > I don't read this warning as "clang will do crazy things with your > code eventually". Clang has always been very verbose when it comes to > undefined behavior, and I don't think that's really a bad thing to do. Sure, but it doesn't belong in -Wall. It's what -Wextra is for. Paolo
diff --git a/disas/arm.c b/disas/arm.c index 6165246..7a7354b 100644 --- a/disas/arm.c +++ b/disas/arm.c @@ -1779,7 +1779,7 @@ print_insn_coprocessor (bfd_vma pc, struct disassemble_info *info, long given, /* Is ``imm'' a negative number? */ if (imm & 0x40) - imm |= (-1 << 7); + imm |= (~0u << 7); func (stream, "%d", imm); }
clang 3.7.0 on x86_64 warns about the following: disas/arm.c:1782:17: warning: shifting a negative signed value is undefined [-Wshift-negative-value] imm |= (-1 << 7); ~~ ^ Note that this patch preserves the tab indent in this source file because the surrounding code still uses tabs. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- disas/arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)