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
