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

[Aaron Ballman]

On Wed, Mar 25, 2015 at 9: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.

I can confirm that MSVC is much happier now. Thank you!

~Aaron