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

[Aaron Ballman]

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.

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