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