[llvm-commits] [llvm] r149457 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineCalls.cpp test/Transforms/InstCombine/2008-01-06-BitCastAttributes.ll test/Transforms/InstCombine/call.ll

[Evan Cheng]

Hi Jim,

I chatted with Eli about this. I share his concern that your fix is not quite right.  Please chat with Eli to get this sorted out.

Thanks,

Evan

On Jan 31, 2012, at 4:32 PM, Eli Friedman wrote:

> On Tue, Jan 31, 2012 at 4:08 PM, Jim Grosbach <grosbach at apple.com> wrote:
>> Author: grosbach
>> Date: Tue Jan 31 18:08:17 2012
>> New Revision: 149457
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=149457&view=rev
>> Log:
>> Disable InstCombine unsafe folding bitcasts of calls w/ varargs.
>> 
>> Changing arguments from being passed as fixed to varargs is unsafe, as
>> the ABI may require they be handled differently (stack vs. register, for
>> example).
>> 
>> Remove two tests which rely on the bitcast being folded into the direct
>> call, which is exactly the transformation that's unsafe.
>> 
>> Removed:
>>    llvm/trunk/test/Transforms/InstCombine/2008-01-06-BitCastAttributes.ll
>> Modified:
>>    llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
>>    llvm/trunk/test/Transforms/InstCombine/call.ll
>> 
>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=149457&r1=149456&r2=149457&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp (original)
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Tue Jan 31 18:08:17 2012
>> @@ -1105,21 +1105,12 @@
>>     if (FT->isVarArg()!=cast<FunctionType>(APTy->getElementType())->isVarArg())
>>       return false;
>>   }
>> -
>> -  if (FT->getNumParams() < NumActualArgs && FT->isVarArg() &&
>> -      !CallerPAL.isEmpty())
>> -    // In this case we have more arguments than the new function type, but we
>> -    // won't be dropping them.  Check that these extra arguments have attributes
>> -    // that are compatible with being a vararg call argument.
>> -    for (unsigned i = CallerPAL.getNumSlots(); i; --i) {
>> -      if (CallerPAL.getSlot(i - 1).Index <= FT->getNumParams())
>> -        break;
>> -      Attributes PAttrs = CallerPAL.getSlot(i - 1).Attrs;
>> -      if (PAttrs & Attribute::VarArgsIncompatible)
>> -        return false;
>> -    }
>> 
>> -
>> +  // If we're casting varargs to non-varargs, that may not be allowable
>> +  // under the ABI, so conservatively don't do anything.
>> +  if (FT->getNumParams() < NumActualArgs && FT->isVarArg())
>> +    return false;
> 
> This comment doesn't make sense given the code above this: if we can't
> see the definition, we won't reach this check, and if we can see the
> definition we're casting we're casting varargs to non-varargs, we
> can't possibly be introducing undefined behavior.
> 
> -Eli
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits