[llvm] r183584 - Fix an assertion in MemCpyOpt pass.

[Duncan Sands]

Hi Shuxin,

On 08/06/13 02:51, Shuxin Yang wrote:
> On 6/7/13 4:39 PM, Eli Friedman wrote:
>> On Fri, Jun 7, 2013 at 3:45 PM, Shuxin Yang <shuxin.llvm at gmail.com
>> <mailto:shuxin.llvm at gmail.com>> wrote:
>>
>>     Author: shuxin_yang
>>     Date: Fri Jun  7 17:45:21 2013
>>     New Revision: 183584
>>
>>     URL: http://llvm.org/viewvc/llvm-project?rev=183584&view=rev
>>     Log:
>>       Fix an assertion in MemCpyOpt pass.
>>
>>       The MemCpyOpt pass is capable of optimizing:
>>           callee(&S); copy N bytes from S to D.
>>         into:
>>           callee(&D);
>>     subject to some legality constraints.
>>
>>       Assertion is triggered when the compiler tries to evalute
>>     "sizeof(typeof(D))",
>>     while D is an opaque-typed, 'sret' formal argument of function being compiled.
>>     i.e. the signature of the func being compiled is something like this:
>>       T caller(...,%opaque* noalias nocapture sret %D, ...)
>>
>>       The fix is that when come across such situation, instead of calling some
>>     utility functions to get the size of D's type (which will crash), we simply
>>     assume D has at least N bytes as implified by the copy-instruction.
>>
>>     rdar://14073661
>>
>>     Modified:
>>         llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>>         llvm/trunk/test/Transforms/MemCpyOpt/memcpy.ll
>>
>>     Modified: llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>>     URL:
>>     http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp?rev=183584&r1=183583&r2=183584&view=diff
>>     ==============================================================================
>>     --- llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp (original)
>>     +++ llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp Fri Jun  7
>>     17:45:21 2013
>>     @@ -626,8 +626,10 @@ bool MemCpyOpt::performCallSlotOptzn(Ins
>>            return false;
>>
>>          Type *StructTy = cast<PointerType>(A->getType())->getElementType();
>>     -    uint64_t destSize = TD->getTypeAllocSize(StructTy);
>>     -
>>     +    // If StructTy is an opaque type, it should have at least <cpyLen> bytes,
>>     +    // as implified by the copy-instruction.
>>     +    uint64_t destSize = StructTy->isSized() ?
>>     +                        TD->getTypeAllocSize(StructTy) : cpyLen;
>>          if (destSize < srcSize)
>>            return false;
>>
>>
>> Why is it safe to assume StructTy is at least cpyLen bytes?
> If not, why is it possible copying cpyLen byte from (part of) src to Dest?

unfortunately you can't assume that the original memcpy is executed, because the
call might not return:

   call @func(..., src, ...)
^ might throw an exception or exit
   memcpy(dest, src, ...)

Thus you can't assume that writing cpyLen bytes to dest is automatically safe.

   memcpy(dest, src, ...)
^ moving the memcpy before the call requires knowing the write to dest is safe
   call @func(..., dest, ...)

I think you should just bail out if !StructTy->isSized().

Ciao, Duncan.

>
>> And if it is safe, why is this check necessary at all?
>>
> I guess you confuse cpyLen with SrcLen.
> I asked the same question when I read the code. After the 2nd thought, I think
> it is necessary.
> You never know how the callee access the Src, it could access the last byte (the
> SrcSize-th byte) of the object
> corresponding to the Src, and cpyLen < SrcSize.
>
>
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>