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

[David Blaikie]

On Wed, Mar 25, 2015 at 6:20 AM, Simon Atanasyan <simon at atanasyan.com>
wrote:

> On Tue, Mar 24, 2015 at 8:57 PM, Simon Atanasyan <simon at atanasyan.com>
> wrote:
> > On Tue, Mar 24, 2015 at 8:51 PM, Shankar Easwaran
> > <shankare at codeaurora.org> wrote:
> >> On 3/24/2015 12:41 PM, Simon Atanasyan wrote:
> >>>
> >>> On Tue, Mar 24, 2015 at 8:35 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>>>
> >>>> On Tue, Mar 24, 2015 at 10:28 AM, Simon Atanasyan <
> simon at atanasyan.com>
> >>>> wrote:
> >>>>>
> >>>>> On Tue, Mar 24, 2015 at 7: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?
> >>>>>
> >>>>> I thought about an alternative solution - fix Elf_Rel_Impl::getType()
> >>>>> method so all its variants have the same return type uint32_t. Any
> >>>>> objections against this idea?
> >>>>
> >>>> It still doesn't seem to improve anything - if getType() still only
> >>>> returns
> >>>> a number 0-255, the shift still is a zero-ing action. Hiding it from
> the
> >>>> compiler doesn't seem to change anything in the semantics of the
> >>>> operation &
> >>>> just obscures things further... seems to me, at least.
> >>>
> >>> The getType() does not always return 0-255. In case of 32-bit variant
> >>> of Elf_Rel_Impl it does. In case of 64-bit variant it returns 32-bit
> >>> integer which can be split up to four parts. In my case I extract
> >>> lower 8 bits and high 24 bits.
> >>
> >> Could you use the r_info (getRInfo) field and extract the needed
> >> information, seems that is what you need ?
> >
> > Nice. I think it is the best solution for the problem.
>
> I hope the problem has been fixed by r233186. Unfortunately using
> getRInfo does not solve the problem because in 32-bit case we extract
> too much data and touch symbol index part of r_info field.
>
> So I just factor out the code that gets the "tag" into the separate
> function. That allows to keep all bit manipulation code into the
> single place and removes the warning.
>

This (to me - though lld isn't my project, etc) just seems to complicate
the code further - what's the point of &ing the 0xFFFFFF00 mask if you're
just shifting down 8 anyway? This hides the issue from the compiler because
the bitwise & does integer promotion (so the char is promoted to int, then
and-ed, then shifted, so the shift is over an int instead of a char) but
produces exactly the same result & now makes the optimizer (& reader) do a
bit more work?

- David


>
> --
> Simon Atanasyan
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150325/f77c89b0/attachment.html>