Patch Review for PR17158

[Bill Wendling]

Hi Chandler,

I agree with what you're saying here. I like David's match much better. Mine was fixing a symptom, while his prevents the bug from ever occurring.

David,

I added at "LGTM" to the bug. I'll add the assert later on. Thanks for fixing this! :-)

-bw

On Nov 7, 2013, at 1:46 AM, Chandler Carruth <chandlerc at google.com> wrote:

> So, I thought your patch would be the right approach originally Bill. However, Nick argued (I think persuasively) that ConstantExpr's are special in one way: analyses and transforms can rely on them being *canonical* for correctness. It is as-if the verifier asserted canonical form for constant expressions. His rationale is that this should be true because the only thing which can form them *always* does full canonicalization.
> 
> I think that, if we grant this as true, your patch should really just reduce to an assert that this situation never arises. Then the PR becomes a crash-on-valid rather than a miscompile. Finally, David's patch which attempts to correct the canonicalization fixes the PR fully by never forming this pattern.
> 
> If you agree, then cool. Let's get David's patch in and a variant of yours that just assert This Is How The IR Shall Look.
> 
> If not, I don't see this is a set-in-stone issue, but I think I'll agree with Nick here, and I think I would want you to convince him otherwise before moving in the direction of making an analysis safe against non-canonical constant expressions.
> 
> -Chandler
> 
> 
> On Wed, Nov 6, 2013 at 11:07 PM, Bill Wendling <isanbard at gmail.com> wrote:
> It does look like we're trying to solve the same problem. Yours might be a more general solution than mine. I commented on your patch. I still want to consult with Chandler to make sure that it's the correct decision. :-)
> 
> -bw
> 
> On Nov 6, 2013, at 4:36 PM, David Majnemer <david.majnemer at gmail.com> wrote:
> 
>> This looks awfully familiar to a bug that my patch fixes.  I tried a similar approach earlier in http://llvm-reviews.chandlerc.com/D2060 but instead settled on http://llvm-reviews.chandlerc.com/D2093
>> 
>> I have a strong feeling that my second patch will fix your bug (and potentially others).
>> 
>>  -- 
>> David Majnemer
>> 
>> 
>> On Wed, Nov 6, 2013 at 4:07 PM, Bill Wendling <isanbard at gmail.com> wrote:
>> Hi Chandler et al,
>> 
>> This is a potential patch for PR17158. What seems to be happening is that GVN gets ahold of the file, it tries to determine if the initializer for x7 can be folded into the use. Because of SROA, it's doing some weird pointer things involving the structure padding. So for instance, the structure is this:
>> 
>>         { { i32, i8, [3 x i8] }, i8, [3 x i8] }
>> 
>> It copies over the 'i32' and 'i8' from the inner structure. SROA then generates a memcpy from the [3 x i8] in the inner structure to the end of the outer structure, which sounds okay. However, GVN looks at the access to the outer 'i8' element, but because of SROA, that element is generated as the element of the inner [3 x i8] at offset 3. This is of course outside of the array. Because it's padding, the value it returns is undef. Thus it folds the value of '0' into the call to 'printf'.
>> 
>> Got that so far? ;-)
>> 
>> Here's a patch I have. It basically makes sure that, if the constant it's looking at is an array, the index into that array does not go outside of that array. If that happens, then there's something complex going on and the constant folding isn't safe anymore.
>> 
>> What do you think? Is this the correct solution?
>> 
>> -bw
>> 
>> Run the attached t.ll with "opt -sroa -instcombine -gvn", generate the .s file, and run it. It should print '1', but instead prints '0'.
>> 
>> diff --git a/lib/Analysis/ConstantFolding.cpp b/lib/Analysis/ConstantFolding.cpp
>> index e093631..527fa80 100644
>> --- a/lib/Analysis/ConstantFolding.cpp
>> +++ b/lib/Analysis/ConstantFolding.cpp
>> @@ -1145,7 +1145,11 @@ Constant *llvm::ConstantFoldLoadThroughGEPConstantExpr(Constant *C,
>>    // Loop over all of the operands, tracking down which value we are
>>    // addressing.
>>    for (unsigned i = 2, e = CE->getNumOperands(); i != e; ++i) {
>> -    C = C->getAggregateElement(CE->getOperand(i));
>> +    Constant *Offset = CE->getOperand(i);
>> +    if (const ArrayType *AT = dyn_cast<ArrayType>(C->getType()))
>> +      if (AT->getNumElements() <= cast<ConstantInt>(Offset)->getZExtValue())
>> +        return 0;
>> +    C = C->getAggregateElement(Offset);
>>      if (C == 0)
>>        return 0;
>>    }
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131107/8c4e76f1/attachment.html>