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

Akira Hatanaka ahatanak at gmail.com
Thu Sep 18 11:47:16 PDT 2014


Thanks for the review. Committed in r218064.

On Thu, Sep 18, 2014 at 11:22 AM, Eric Christopher <echristo at gmail.com>
wrote:

> LGTM. Thanks!
>
> -eric
>
> On Thu, Sep 18, 2014 at 11:17 AM, Akira Hatanaka <ahatanak at gmail.com>
> wrote:
>
>> The attached patch is a follow-up to r217994. I defined a new function
>> validateOperandSize, which is used to check both input and output sizes.
>>
>>
>> On Tue, Sep 16, 2014 at 6:07 PM, Eric Christopher <echristo at gmail.com>
>> wrote:
>>
>>> Cool, thanks.
>>>
>>> -eric
>>>
>>> On Tue, Sep 16, 2014 at 6:02 PM, Akira Hatanaka <ahatanak at gmail.com>
>>> wrote:
>>>
>>>> OK, I'll check in a patch that fixes
>>>> X86_32TargetInfo::validateInputSize first then.
>>>>
>>>> On Tue, Sep 16, 2014 at 5:55 PM, Eric Christopher <echristo at gmail.com>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Tue, Sep 16, 2014 at 5:53 PM, Akira Hatanaka <ahatanak at gmail.com>
>>>>> wrote:
>>>>>
>>>>>> On Tue, Sep 16, 2014 at 5:12 PM, Eric Christopher <echristo at gmail.com
>>>>>> > wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>>
>>>>>> OK, I see. I just felt that S and D should be added too, since they
>>>>>> are single register constraints that have to be bound to variables smaller
>>>>>> than 64-bit, as constraints a-d are.
>>>>>>
>>>>>> I can probably add R, q, Q, to the switch-case statement too. Also,
>>>>>> in my next patch, I was going to add checks for constraints x and y.
>>>>>>
>>>>>> Should I add the all the constraints I mentioned above to
>>>>>> X86_32TargetInfo::validateInputSize or X86TargetInfo::validateInputSize
>>>>>> first and then add the checks for output constraints?
>>>>>>
>>>>>
>>>>> Seems like a reasonable way to go yes?
>>>>>
>>>>> -eric
>>>>>
>>>>>
>>>>>>
>>>>>> -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/20140918/6eaed453/attachment.html>


More information about the cfe-commits mailing list