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 <[email protected]> wrote: > > > On Tue, Sep 16, 2014 at 5:53 PM, Akira Hatanaka <[email protected]> > wrote: > >> On Tue, Sep 16, 2014 at 5:12 PM, Eric Christopher <[email protected]> >> wrote: >> >>> >>> >>> On Tue, Sep 16, 2014 at 4:00 PM, Akira Hatanaka <[email protected]> >>> wrote: >>> >>>> On Tue, Sep 16, 2014 at 1:06 PM, Eric Christopher <[email protected]> >>>> 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 <[email protected]> >>>>> 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 <[email protected]> >>>>>> 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 <[email protected]> >>>>>>> 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 < >>>>>>>> [email protected]> 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 <[email protected]> >>>>>>>>> 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 < >>>>>>>>>> [email protected]> 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 < >>>>>>>>>>> [email protected]> 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 >>>>>>>>>>> [email protected] >>>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> _______________________________________________ >>>>>> cfe-commits mailing list >>>>>> [email protected] >>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
