[lld] r233088 - [Mips] Suppress "right shift by too large amount" warning

[David Blaikie]

On Tue, Mar 24, 2015 at 9:28 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> On Tue, Mar 24, 2015 at 12:24 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >
> >
> > On Tue, Mar 24, 2015 at 9:22 AM, Aaron Ballman <aaron at aaronballman.com>
> > wrote:
> >>
> >> On Tue, Mar 24, 2015 at 12:16 PM, David Blaikie <dblaikie at gmail.com>
> >> wrote:
> >> >
> >> >
> >> > On Tue, Mar 24, 2015 at 8:49 AM, Simon Atanasyan <simon at atanasyan.com
> >
> >> > wrote:
> >> >>
> >> >> Author: atanasyan
> >> >> Date: Tue Mar 24 10:49:59 2015
> >> >> New Revision: 233088
> >> >>
> >> >> URL: http://llvm.org/viewvc/llvm-project?rev=233088&view=rev
> >> >> Log:
> >> >> [Mips] Suppress "right shift by too large amount" warning
> >> >>
> >> >> Visual C++ shows the "right shift by too large amount" warning if
> >> >> `MipsELFReference` is instantiated for 32-bit target and
> >> >> `Elf_Rel_Impl::getType`
> >> >> method has `unsigned char` return type. We can freely suppress the
> >> >> warning
> >> >> in
> >> >> that case because MIPS 32-bit ABI does not pack multiple relocation
> >> >> types
> >> >> into
> >> >> the single field `r_type` and the `MipsELFReference::_tag` should be
> >> >> always
> >> >> zero in that case.
> >> >
> >> >
> >> > Can we just disable the MSVC warning if it's wrong, then?
> >>
> >> IMO, the MSVC warning was correct (the original code was right
> >> shifting all significant bits away) and should not be disabled.
> >
> >
> > The original code is in a template - it seems reasonable that some
> > instantiations of the template might legitimately shift all the bits away
> > (indeed the code change is just suppressing that behavior - the end
> result
> > will be the same (if you only have 8 bits, promoting to a 32 bit still
> > leaves the other bits zero, so shifting down 8 still leaves your result
> zero
> > as it would've been with the original code) - so it doesn't seem like it
> > caught a bug here, or at least this code change didn't fix any bugs)
> while
> > other instantiations may not.
>
> Agreed that it did not catch a bug here, but that warning should
> trigger a red flag when it appears, as you may not intend to shift
> away all significant bits. This isn't likely to be a chatty
> diagnostic, and silencing it adds some code clarity IMO. I certainly
> questioned the behavior of the code when I ran into it.
>

Could you suggest a better way to write it? The current code change doesn't
seem to make it any more legible, I don't think - purely suppressing a
warning.

I still don't think, even if this is hard to read, it's worth warning about
given that it is correct here & doesn't seem to have improved due to this
warning diagnosing it.


>
> ~Aaron
>
> >
> > - David
> >
> >>
> >>
> >> ~Aaron
> >>
> >> >
> >> >>
> >> >>
> >> >> No functional changes.
> >> >>
> >> >> Modified:
> >> >>     lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h
> >> >>
> >> >> Modified: lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h
> >> >> URL:
> >> >>
> >> >>
> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h?rev=233088&r1=233087&r2=233088&view=diff
> >> >>
> >> >>
> >> >>
> ==============================================================================
> >> >> --- lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h (original)
> >> >> +++ lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h Tue Mar 24
> >> >> 10:49:59
> >> >> 2015
> >> >> @@ -101,13 +101,13 @@ public:
> >> >>        : ELFReference<ELFT>(
> >> >>              &rel, rel.r_offset - symValue,
> Reference::KindArch::Mips,
> >> >>              rel.getType(_isMips64EL) & 0xff,
> >> >> rel.getSymbol(_isMips64EL)),
> >> >> -        _tag(rel.getType(_isMips64EL) >> 8) {}
> >> >> +        _tag(uint32_t(rel.getType(_isMips64EL)) >> 8) {}
> >> >>
> >> >>    MipsELFReference(uint64_t symValue, const Elf_Rel &rel)
> >> >>        : ELFReference<ELFT>(rel.r_offset - symValue,
> >> >> Reference::KindArch::Mips,
> >> >>                             rel.getType(_isMips64EL) & 0xff,
> >> >>                             rel.getSymbol(_isMips64EL)),
> >> >> -        _tag(rel.getType(_isMips64EL) >> 8) {}
> >> >> +        _tag(uint32_t(rel.getType(_isMips64EL)) >> 8) {}
> >> >>
> >> >>    uint32_t tag() const override { return _tag; }
> >> >>    void setTag(uint32_t tag) { _tag = tag; }
> >> >>
> >> >>
> >> >> _______________________________________________
> >> >> llvm-commits mailing list
> >> >> llvm-commits at cs.uiuc.edu
> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >> >
> >> >
> >> >
> >> > _______________________________________________
> >> > llvm-commits mailing list
> >> > llvm-commits at cs.uiuc.edu
> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150324/27c3a16e/attachment.html>