<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 24, 2015 at 9:28 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Mar 24, 2015 at 12:24 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Tue, Mar 24, 2015 at 9:22 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> On Tue, Mar 24, 2015 at 12:16 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> wrote:<br>
>> ><br>
>> ><br>
>> > On Tue, Mar 24, 2015 at 8:49 AM, Simon Atanasyan <<a href="mailto:simon@atanasyan.com">simon@atanasyan.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> Author: atanasyan<br>
>> >> Date: Tue Mar 24 10:49:59 2015<br>
>> >> New Revision: 233088<br>
>> >><br>
>> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=233088&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=233088&view=rev</a><br>
>> >> Log:<br>
>> >> [Mips] Suppress "right shift by too large amount" warning<br>
>> >><br>
>> >> Visual C++ shows the "right shift by too large amount" warning if<br>
>> >> `MipsELFReference` is instantiated for 32-bit target and<br>
>> >> `Elf_Rel_Impl::getType`<br>
>> >> method has `unsigned char` return type. We can freely suppress the<br>
>> >> warning<br>
>> >> in<br>
>> >> that case because MIPS 32-bit ABI does not pack multiple relocation<br>
>> >> types<br>
>> >> into<br>
>> >> the single field `r_type` and the `MipsELFReference::_tag` should be<br>
>> >> always<br>
>> >> zero in that case.<br>
>> ><br>
>> ><br>
>> > Can we just disable the MSVC warning if it's wrong, then?<br>
>><br>
>> IMO, the MSVC warning was correct (the original code was right<br>
>> shifting all significant bits away) and should not be disabled.<br>
><br>
><br>
> The original code is in a template - it seems reasonable that some<br>
> instantiations of the template might legitimately shift all the bits away<br>
> (indeed the code change is just suppressing that behavior - the end result<br>
> will be the same (if you only have 8 bits, promoting to a 32 bit still<br>
> leaves the other bits zero, so shifting down 8 still leaves your result zero<br>
> as it would've been with the original code) - so it doesn't seem like it<br>
> caught a bug here, or at least this code change didn't fix any bugs) while<br>
> other instantiations may not.<br>
<br>
</div></div>Agreed that it did not catch a bug here, but that warning should<br>
trigger a red flag when it appears, as you may not intend to shift<br>
away all significant bits. This isn't likely to be a chatty<br>
diagnostic, and silencing it adds some code clarity IMO. I certainly<br>
questioned the behavior of the code when I ran into it.<br></blockquote><div><br>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.<br><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> - David<br>
><br>
>><br>
>><br>
>> ~Aaron<br>
>><br>
>> ><br>
>> >><br>
>> >><br>
>> >> No functional changes.<br>
>> >><br>
>> >> Modified:<br>
>> >> lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h<br>
>> >><br>
>> >> Modified: lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h?rev=233088&r1=233087&r2=233088&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h?rev=233088&r1=233087&r2=233088&view=diff</a><br>
>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h (original)<br>
>> >> +++ lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h Tue Mar 24<br>
>> >> 10:49:59<br>
>> >> 2015<br>
>> >> @@ -101,13 +101,13 @@ public:<br>
>> >> : ELFReference<ELFT>(<br>
>> >> &rel, rel.r_offset - symValue, Reference::KindArch::Mips,<br>
>> >> rel.getType(_isMips64EL) & 0xff,<br>
>> >> rel.getSymbol(_isMips64EL)),<br>
>> >> - _tag(rel.getType(_isMips64EL) >> 8) {}<br>
>> >> + _tag(uint32_t(rel.getType(_isMips64EL)) >> 8) {}<br>
>> >><br>
>> >> MipsELFReference(uint64_t symValue, const Elf_Rel &rel)<br>
>> >> : ELFReference<ELFT>(rel.r_offset - symValue,<br>
>> >> Reference::KindArch::Mips,<br>
>> >> rel.getType(_isMips64EL) & 0xff,<br>
>> >> rel.getSymbol(_isMips64EL)),<br>
>> >> - _tag(rel.getType(_isMips64EL) >> 8) {}<br>
>> >> + _tag(uint32_t(rel.getType(_isMips64EL)) >> 8) {}<br>
>> >><br>
>> >> uint32_t tag() const override { return _tag; }<br>
>> >> void setTag(uint32_t tag) { _tag = tag; }<br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> llvm-commits mailing list<br>
>> >> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>> ><br>
>> ><br>
>> ><br>
>> > _______________________________________________<br>
>> > llvm-commits mailing list<br>
>> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>