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