[PATCH] x86 inline-asm: error-out on a 64-bit variable bound to a single register in 32-bit mode

[Akira Hatanaka]

Does the latest patch look fine? I am working on another patch which fixes
a similar bug and I need to commit this patch first.


On Thu, Aug 28, 2014 at 10:08 AM, Akira Hatanaka <ahatanak at gmail.com> wrote:

> Latest version of the patch is attached which fixes a couple of
> oversights. I had to add a line which checks whether Ty is a dependent type
> before getTypeSize is called. Also, in the test case, "=" was missing
> before constraint "a", so fixed that too.
>
> On Wed, Aug 27, 2014 at 3:22 PM, Reid Kleckner <rnk at google.com> wrote:
>
>> New patch looks good to me.
>>
>> It sounds like we have two cases of size mismatch:
>> - The output operand lvalue is smaller than the constraint, meaning the
>> store will write out of bounds. Your patch adds this.
>> - The output operand lvalue is bigger than the constraint, meaning the
>> whole value won't be initialized. We currently warn here via
>> validateConstraintModifier.
>>
>> This code probably deserves some cleanup, but your patch is consistent
>> with what we do for input operands, so let's go with that.
>>
>>
> The reason llvm is crashing in the backend is that it's trying to use a
> 64-bit register in 32-bit mode. It's not because a store is writing out of
> bounds or there is a value left uninitialized. In the test case, if we
> declare the variable bound to constraint "=a" to be a unit32_t or an
> integer type that is smaller than 32-bit, clang compiles the program fine.
>
>
>>
>> On Wed, Aug 27, 2014 at 12:35 PM, Akira Hatanaka <ahatanak at gmail.com>
>> wrote:
>>
>>> The commit log in r166737 doesn't say much about why this is a warning
>>> instead of an error, but I know there are cases where warnings are needed.
>>> For example, clang has to issue warnings instead of errors for the
>>> inline-asm statements in the test case committed in r216260. If it's not
>>> desirable to change validateConstraintModifier, we can add a function which
>>> checks the output size that is similar to validateInputSize in r167717 (see
>>> attached patch), which was suggested in the post-commit review.
>>>
>>>
>>> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20121112/067945.html
>>>
>>> I am not sure whether we can use fixit in this case. Fixit hints should
>>> be used only if we know the user's intent and it's very clear that applying
>>> the fixit hint is the right thing to do. Changing the type of variable "r"
>>> to a 32-bit int will avoid crashing, but it doesn't look like that's what
>>> the user wants.
>>>
>>> On Mon, Aug 25, 2014 at 6:20 PM, Reid Kleckner <rnk at google.com> wrote:
>>>
>>>> Can you investigate why we are warning in the first place? I think we
>>>> should either only warn or only error. Currently we have a warning with a
>>>> fixit but we don't recover as though we had applied the fixit. If we did
>>>> that, we would not crash.
>>>>
>>>> In addition to the Clang-side changes, LLVM should probably be
>>>> returning an error or reporting a fatal error instead of hitting
>>>> unreachable.
>>>>
>>>>
>>>>  On Mon, Aug 25, 2014 at 2:10 PM, Akira Hatanaka <ahatanak at gmail.com>
>>>> wrote:
>>>>
>>>>>  Rebased patches attached.
>>>>>
>>>>> I also made changes to the clang patch so that clang can error-out
>>>>> after a size mismatch is found as soon as
>>>>> possible.TargetInfo::validateConstraintModifier has an extra parameter
>>>>> IsError, which is set when it decides there is no point in continuing
>>>>> compilation and it should stop compilation immediately. The error message
>>>>> clang prints looks better than lllvm's message, but if it isn't right to
>>>>> change the warning to an error, then I guess we have to detect the error
>>>>> later just before isel, as is done in the llvm patch.
>>>>>
>>>>> On Wed, Jul 23, 2014 at 3:56 PM, Akira Hatanaka <ahatanak at gmail.com>
>>>>> wrote:
>>>>>
>>>>>> llvm should error-out when a 64-bit variable is bound to a single
>>>>>> register in x86 32-bit mode, but ToT clang/llvm fails to detect this error
>>>>>> and continues compilation until it crashes in type-legalization:
>>>>>>
>>>>>> $ llc test/CodeGen/X86/inline-asm-regsize.ll  -O3
>>>>>> -mtriple=i386-apple-darwin -o -
>>>>>>
>>>>>> inline-asm-regsize.ll  -O3 -mtriple=i386-apple-darwin -o -
>>>>>>
>>>>>> .section __TEXT,__text,regular,pure_instructions
>>>>>>
>>>>>> ExpandIntegerResult #0: 0x7fa2d1041728: i64 = Register %RCX [ID=0]
>>>>>>
>>>>>>
>>>>>> Do not know how to expand the result of this operator!
>>>>>>
>>>>>> UNREACHABLE executed at
>>>>>> /Users/ahatanaka/projects/llvm/git/llvm3/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:1116!
>>>>>>
>>>>>> The attached patch fixes llvm to error-out and print this error
>>>>>> message:
>>>>>>
>>>>>> error: Cannot bind a variable larger than 32-bit to a single register
>>>>>> in 32-bit mode
>>>>>>
>>>>>>
>>>>>> My initial solution was to have clang detect this error in
>>>>>> TargetInfo::validateConstraintModifier. However, the code in
>>>>>> SemaStmtAsm.cpp has to be changed to error-out instead of issuing a
>>>>>> warning, which I wasn't sure was the right thing to do. I am attaching this
>>>>>> patch too in case someone has a suggestion or an opinion on it.
>>>>>>
>>>>>> <rdar://problem/17476970>
>>>>>>
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>
>>>
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140829/f09015d0/attachment.html>