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

[Richard Smith]

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.
>

In that case, the FixIt can be on a note attached to the diagnostic, rather
than on the diagnostic itself.


> 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/20140827/f3121cd7/attachment.html>