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

[Reid Kleckner]

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.


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/llvm-commits/attachments/20140827/dbb253a6/attachment.html>