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

[Ahmed Charles]

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