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

Eric Christopher echristo at gmail.com
Tue Sep 16 17:12:55 PDT 2014 

On Tue, Sep 16, 2014 at 4:00 PM, Akira Hatanaka <ahatanak at gmail.com> wrote:

> On Tue, Sep 16, 2014 at 1:06 PM, Eric Christopher <echristo at gmail.com>
> wrote:
>
>> You'll want to split out the new contraints for input size into a
>> separate patch. (And just commit it).
>> A small comment of why we're ignoring dependent types would be good.
>>
>> One question: Why not just add all of the contraints first rather than
>> piecemeal as you get testcases? (Related to the comment above).
>>
>>
> Just to make sure I'm not misunderstanding your question, are you
> suggesting I use "=abcdSD" instead of "=a" in the test case and do the
> check in one line?
>
> uint64_t val;
>
> __asm__ volatile("addl %1, %0" : "=abcdSD" (val) : "a" (msr)); //
> expected-error {{invalid output size for constraint '=abcdSD'}}
>
>
> Are you also suggesting that we should have clang print just the
> constraints that are invalid in the error message? For example, if we added
> "A" and use "=abcdSDA" instead, clang would remove "A", since it can be
> bound to a 64-bit variable, and print  "=abcdSD" or "abcdSD" instead?
>
>
>
No, I'm curious why you're adding S and D now, but not any other constraint
that has a size associated with the register.

-eric


>
> Thanks!
>>
>> -eric
>>
>> On Fri Aug 29 2014 at 4:46:37 PM Akira Hatanaka <ahatanak at gmail.com>
>> wrote:
>>
>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>> _______________________________________________
>>> 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/20140916/156d21eb/attachment.html>

More information about the cfe-commits mailing list