[llvm-commits] [PATCH] Fix undefined behavior in the Mips backend

[Ahmed Charles]

R152390
From: Ahmed Charles
Sent: 3/2/2012 10:31 AM
To: Akira Hatanaka
Cc: llvm-commits at cs.uiuc.edu
Subject: RE: [llvm-commits] [PATCH] Fix undefined behavior in the Mips
backend
Either one is right (I spent some time with different programs trying
out the various combinations), but I think that it is clearer if there
are no conversions and the bits are all laid out in front of you. That
is a matter of opinion though. :)
From: Akira Hatanaka
Sent: 3/1/2012 6:17 PM
To: Ahmed Charles
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] [PATCH] Fix undefined behavior in the Mips
backend
Thank you for the patch.

I don't think you have to replace ~0xffff with 0xffffffffffff0000ULL.
Isn't ~0xffff converted to uint64_t in the following way?

1. Flip bits: 0xffff0000 (which is equal to -0x10000)
2. add one more than the maximum value that can be represented with
uint64_t: -0x10000 + 0x10000000000000000 = 0xffffffffffff0000

Same for -1:
-1 + 0x10000000000000000 = 0xffffffffffffffff

Isn't this what the standard states?

On Wed, Feb 29, 2012 at 2:23 AM, Ahmed Charles <ahmedcharles at gmail.com> wrote:
> Does the attached make sense?
>
> It was a bit tricky getting the integer promotions to do the same
> thing. The biggest one was:
>
> -  GetInstSeqLs((Imm + 0x8000) & ~0xffff, RemSize, SeqLs);
> +  GetInstSeqLs((Imm + 0x8000ULL) & 0xffffffffffff0000ULL, RemSize, SeqLs);
>
> sign extending ~0xffff, when implicitly promoting to an uint64_t seems
> a bit subtle to me, so I changed the literals to be a bit more
> explicit.
>
> On Fri, Feb 24, 2012 at 11:46 AM, Akira Hatanaka <ahatanak at gmail.com> wrote:
>> Also, the type of MaskedImm should be uint64_t in this line:
>>
>> void MipsAnalyzeImmediate::GetInstSeqLs(int64_t Imm, unsigned RemSize,
>>                                         InstSeqLs &SeqLs) {
>>   int64_t MaskedImm = Imm & (((uint64_t)-1) >> (64 - Size));
>>
>> These should remain int64_t:
>>
>> void MipsAnalyzeImmediate::ReplaceADDiuSLLWithLUi(InstSeq &Seq) {
>>  ...
>>  int64_t Imm = SignExtend64<16>(Seq[0].ImmOpnd);
>>  int64_t ShiftedImm = Imm << (Seq[1].ImmOpnd - 16);
>>
>>
>> Thanks for pointing out this problem.
>>
>> On Fri, Feb 24, 2012 at 11:43 AM, Akira Hatanaka <ahatanak at gmail.com> wrote:
>>> I think the possibility of undefined behavior remains even after
>>> applying the patch, since the uint64_t value is converted to int64_t
>>> when it is passed to GetInstSeqLs. Is that right?
>>>
>>> If that is the case, all functions should take "uint64_t Imm" instead
>>> of "int64_t Imm" as you have suggested.
>>>
>>> On Fri, Feb 24, 2012 at 2:30 AM, Ahmed Charles <ahmedcharles at gmail.com> wrote:
>>>> This patch maintains current behavior while avoiding UB. I'd like
>>>> someone with more understanding of the Mips backend to suggest whether
>>>> all of these functions should take a uint64_t or not, since they are
>>>> just representing 'bits', rather than signed values.
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>
>
>
> --
> Ahmed Charles