<div style="font-family:arial,helvetica,sans-serif"><font>Nice! This looks so much better, thanks!<div><br></div><div><div><div>+/// \brief Evaluate the size and offset of an object ponted by a Value*</div><div>
+/// statically. Fails if size or offset are not known at compile time.</div>
<div>+class ObjectSizeOffsetVisitor</div><div>+: public InstVisitor<ObjectSizeOffsetVisitor, SizeOffsetType> {</div></div><div><br></div><div>Mm, two space before the ":" I think? I'm not sure but it looks wrong with no indentation at all. Same with ObjectSizeOffsetEvaluator too.</div>
<div><br></div><div>+static Function *getCalledFunction(const Value *V, bool LookThroughBitCast) {</div><div>+ if (LookThroughBitCast)</div><div>+ V = V->stripPointerCasts();</div><div>+ const CallInst *CI = dyn_cast<CallInst>(V);</div>
<div> if (!CI)</div><div>- return false;</div><div>+ return 0;</div></div><div><br></div><div>That works for C, but operator new can through exceptions. Please support InvokeInst as well as CallInst. Sorry. Yes, this means that callers have to worry about asking for a malloc-like function potentially throwing exceptions.</div>
<div><br></div><div><div>+static bool hasNoAliasAttr(const Value *V, bool LookThroughBitCast) {</div><div>+ Function *Callee = getCalledFunction(V, LookThroughBitCast);</div><div>+ return Callee && Callee->hasFnAttr(Attribute::NoAlias);</div>
<div>+}</div><div><br></div><div>When V isa CallInst or InvokeInst, the call or invoke may have noalias (and other attributes normally found on function prototypes) at the call-site instead. Yes, llvm supports stating that a function may write memory but at this particular callsite we happen to know that it won't.</div>
<div><br></div><div> CallSite CS(LookThroughBitCast ? V->stripPointerCasts() : V->stripPointerCasts());</div><div> CS.paramHasAttr(~0U, Attribute::NoAlias);</div><div><br></div><div>will test that, but you may want to change CallSite/CallInst/InvokeInst to add CS.hasFnAttr() to make this way cleaner. See the way CallSite::doesNotReturn() is implemented for example.</div>
<div><br></div><div><div>+/// \brief Tests if a value is a call to a library function that allocates</div><div>+/// unitialized memory (such as malloc).</div><div>+bool llvm::isMallocLikeFn(const Value *V, bool LookThroughBitCast) {</div>
</div><div><br></div><div>Typo in comment ("unitialized").</div><div><br></div><div><div>+/// \brief Compute the size of the object pointed by Ptr. Returns true and the</div><div>+/// object size in Size if successfull, and false otherwise.</div>
</div><div><br></div><div>Typo in comment ("successfull").</div><div><br></div><div><div><div><div><div>+ APInt ObjSize = Data.first, Offset = Data.second;</div><div>+ // check for overflow</div><div>+ if (Offset.slt(0) || ObjSize.ult(Offset))</div>
<div>+ Size = 0;</div></div><div><br></div><div>If there's overflow, you don't want to return false?</div></div><div><br></div></div><div>+STATISTIC(ObjectVisitorInterproc,</div><div>+ "ObjectSizeOffsetVisitor: unhandled interproc value");</div>
<div>+STATISTIC(ObjectVisitorLoad,</div><div>+ "ObjectSizeOffsetVisitor: unhandled load instruction");</div></div><div><br></div><div>STATISTIC(ObjectVisitorArgument, "Number of arguments with unsolved size and offset");</div>
<div><div>STATISTIC(ObjectVisitorLoad, "Number of load instructions with unsolved size and offset");</div></div><div><br></div><div><div><div>+ObjectSizeOffsetVisitor::ObjectSizeOffsetVisitor(const TargetData *TD,</div>
</div><div>+ LLVMContext &Context,</div><div>+ bool RoundToAlign)</div><div>+: TD(TD), RoundToAlign(RoundToAlign) {</div>
</div><div><br></div><div><div>I tried to figure out how much the : is supposed to be indented by and found no consensus, except that it's supposed to be indented. :) Can I buy four spaces, maybe?</div></div><div><br>
</div><div><div><div>+ // check cache</div><div>+ CacheMapTy::iterator CacheIt = CacheMap.find(V);</div><div>+ if (CacheIt != CacheMap.end())</div><div>+ return CacheIt->second;</div></div><div>// ... invalidate CacheIt ...</div>
<div>+ CacheMap[V] = Result;</div></div><div><br></div><div>One common trick is to say:</div><div> SizeOffsetEvalType &Result = CacheMap[V];</div><div>but that requires a way to tell the difference between a default-constructed SizeOffsetEvalType from unknown(). If you can't apply this change, please at least add a comment before "CacheMap[V] = Result;" to tell the reader that CacheIt is invalid at this point!</div>
<div><br></div><div><br></div><div>So, one other thing I was thinking. We're potentially visiting the same instruction chain twice. ObjectSizeOffsetVisitor isn't caching, which means that calling the evaluator on a chain of instructions like:</div>
<div>void foo(A) {</div><div> B = A + 1</div><div> C = B + 1</div><div> D = C + 1</div><div> // ...</div><div> Z = Y + 1</div><div>if we call the evaluator on Z we'll end up calling the visitor on Y which will traverse up to A then fail, then the evaluator will walk to Y and call the visitor on X which will do the same traversal (minus one) and fail. Ultimately this is O(n^2).</div>
<div><br></div><div>One fix is to add caching, but can we do better?<br></div><div><br></div><div><br></div><div>I think you can submit the next version of the patch instead of mailing it out for further review.</div><div>
<br></div><div>Nick</div><div><br></div><div class="gmail_quote">On 19 June 2012 16:20, Nuno Lopes <span dir="ltr"><<a href="mailto:nunoplopes@sapo.pt" target="_blank">nunoplopes@sapo.pt</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks Nick and Chandler for the reviews! Since this patch touches a lot of different places, I definitely want to make sure I don't break (too many) things.<br>
I send a new patch in attach, which I believe addresses all concerns raised so far.<div><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I think this should really be two pieces, one which determines the size of<br>
things which can be done without modifying the code and a wrapper around<br>
that which builds up IR when that fails. Here's roughly what I have in mind:<br>
</blockquote>
<br></div>
Well, I gave it a try and I split the code in the new patch.<br>
The only thing that bothers me is that there is a little code duplication. But I agree the code becomes much cleaner.<div><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Index: lib/Analysis/IPA/<u></u>GlobalsModRef.cpp<br>
==============================<u></u>==============================<u></u>=======<br>
--- lib/Analysis/IPA/<u></u>GlobalsModRef.cpp (revision 158666)<br>
+++ lib/Analysis/IPA/<u></u>GlobalsModRef.cpp (working copy)<br>
@@ -329,15 +329,8 @@<br>
// Check the value being stored.<br>
Value *Ptr = GetUnderlyingObject(SI-><u></u>getOperand(0));<br>
<br>
- if (isMalloc(Ptr)) {<br>
- // Okay, easy case.<br>
- } else if (CallInst *CI = dyn_cast<CallInst>(Ptr)) {<br>
- Function *F = CI->getCalledFunction();<br>
- if (!F || !F->isDeclaration()) return false; // Too hard to<br>
analyze.<br>
- if (F->getName() != "calloc") return false; // Not calloc.<br>
- } else {<br>
+ if (!isNoAliasFn(Ptr))<br>
return false; // Too hard to analyze.<br>
- }<br>
<br>
I'm not sure this is right.. There's another assumption that globals<br>
mod-ref is making which isn't in the comments; it's assuming that the<br>
called functions (malloc and calloc) have no side-effects other than the<br>
allocation of memory. In general, the callee needs to mark<br>
"FunctionInfo[Writers[i]].<u></u>GlobalInfo[I] |= Mod" when isNoAlias but can<br>
remove that marking when it really is malloc/calloc.<br>
</blockquote>
<br></div>
You're right. I replaced that call with isAllocLikeFn().<div><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+enum AllocType {<br>
+ MallocLike = 1<<0, // allocates<br>
+ CallocLike = 1<<1, // allocates + bzero<br>
+ ReallocLike = 1<<2, // reallocates<br>
+ StrDupLike = 1<<3,<br>
+ AllocLike = MallocLike | CallocLike | StrDupLike,<br>
+ AnyAlloc = MallocLike | CallocLike | ReallocLike | StrDupLike<br>
+};<br>
<br>
I don't understand why StrDup gets special handling? "fopen" also returns<br>
newly allocated memory. What they have in common is the noalias marker. Are<br>
you using this like calloc to indicate that the memory is initialized to a<br>
copy of other memory? Please add comments.<br>
</blockquote>
<br></div>
strdup() has to be treated separately because the function to compute the object's size (strlen(str)+1) has a different pattern from the others (which is a simple multiplication of up to 2 parameters).<br>
Supporting fopen() here is not very important IMHO, since client code will not dereference a FILE* directly, so the information about its size is probably not very important. For alias analysis purposes, the function should be marked noalias.<br>
<br>
<br>
Thanks,<br>
Nuno<div><div><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Nick<br>
<br>
On 18 June 2012 14:19, Nuno Lopes <<a href="mailto:nunoplopes@sapo.pt" target="_blank">nunoplopes@sapo.pt</a>> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi,<br>
<br>
Right now, there are multiple implementations of analysis of memory<br>
allocation functions across the LLVM codebase, including:<br>
- computing size of pointed objects (dead store elimination, instcombine,<br>
basic alias analysis)<br>
- recognize allocation functions (e.g., to know that the return value<br>
doesn't alias anything else, or to recognize uninitialized allocated memory<br>
for undef purposes).<br>
<br>
Therefore I send in attach a patch to refactor the MemoryBuiltin Analysis<br>
to include all the functionality needed by the other passes just mentioned.<br>
It removes a lot of code duplication, and the proposed implementation is<br>
better than all the previous ones combined. The patch adds support for the<br>
noalias function attribute as well.<br>
This patch also adds support for computing the size of pointed objects at<br>
run-time, which is basically a copy from the bounds checking pass. After<br>
this patch goes in, most of the code in the bounds checking pass can be<br>
removed.<br>
<br>
Any comments/suggestions/etc..?<br>
<br>
Thanks,<br>
Nuno<br>
</blockquote></blockquote>
</div></div></blockquote></div><br></div>
</font></div>