[llvm-commits] [llvm] r157261 - in /llvm/trunk: include/llvm/InitializePasses.h include/llvm/LinkAllPasses.h include/llvm/Transforms/Scalar.h include/llvm/Transforms/Utils/Local.h lib/Transforms/InstCombine/InstCombine.h lib/Transforms/InstCombine/InstCombineAddSub.cpp lib/Transforms/InstCombine/InstructionCombining.cpp lib/Transforms/Scalar/BoundsChecking.cpp lib/Transforms/Scalar/Scalar.cpp

[John Criswell]

On 5/22/12 5:19 PM, Nuno Lopes wrote:
> Hi John,
>
> Thanks for the throughout review!
>
> Some replies inline:
>
>
> [snip]
>
>
>>> +ConstTriState BoundsChecking::computeAllocSize(Value *Alloc, 
>>> uint64_t&Size,
>>> +                                     Value*&SizeValue) {
>>> +  if (GlobalVariable *GV = dyn_cast<GlobalVariable>(Alloc)) {
>>> +    if (GV->hasDefinitiveInitializer()) {
>>> +      Constant *C = GV->getInitializer();
>>> +      Size = TD->getTypeAllocSize(C->getType());
>>> +      return Const;
>>> +    }
>>> +    return Dunno;
>>
>> You can find the size of a global even if it doesn't have an 
>> initializer; the trick is to make sure that the global is not 
>> declared in another compilation unit.
>
> uhm, if the global doesn't have a definitive initializer, how can you 
> be sure that it won't be replaced in, e.g., link time?  Unless I'm 
> missing something, I think this is correct.

As far as the global being replaced, I do not think that should be 
possible for at least some of the linkage types (like internal and 
private).  It certainly is possible for other linkage types (like 
weak).  The tricky ones would be the types in which the global is 
defined but externally visible.  I'd have to check whether the size can 
change if the global has two definitions in different compilation units 
of different type, and, if so, which type gets used.

>
>
>>> +  // Get to the real allocated thing and offset as fast as possible.
>>> +  Ptr = Ptr->stripPointerCasts();
>>> +  GEPOperator *GEP;
>>> +
>>> +  if ((GEP = dyn_cast<GEPOperator>(Ptr))) {
>>> +    // check if we will be able to get the offset
>>> +    if (!GEP->hasAllConstantIndices()&&  Penalty<  2) {
>>> +      ++ChecksUnable;
>>> +      return false;
>>> +    }
>>> +    Ptr = GEP->getPointerOperand()->stripPointerCasts();
>>> +  }
>>
>> If I understand this code correctly, you're just checking to see if 
>> the dereferenced pointer (Ptr) is a GEP and, if so, generating code 
>> to calculate it's offset.  What if Ptr is a PHI node derived from a 
>> GEP?  I imagine you'd see that in loops a lot.  Does it handle 
>> non-pointer casts (like inttoptr and ptrtoint, which I think some of 
>> the loop transforms add when modifying GEPs)?  Unless I'm not seeing 
>> something, your code will miss a lot of trivial cases.  Is this 
>> intentional or an oversight?
>
> Right now the code doesn't support PHIs nor selects, but support will 
> come soon.

SAFECode's exactcheck optimization supports most of these; the PHI node 
is the only one that is missing AFAIK, and we'll have that once we 
refactor the optimization to be more efficient.

I'm probably beating a dead horse, but it looks to me like you're 
reimplementing, in a slightly different way, some core SAFECode 
components (namely the load/store instrumentation pass and the fast 
check optimization).

> About inttoptr, actually instcombine tries to fold those instructions 
> back to GEPs whenever possible.

Glad to hear that instcombine does that.

-- John T.

>
> Thanks,
> Nuno