[llvm-commits] [PATCH] MemorySanitizer instrumentation pass.
Chandler Carruth
chandlerc at gmail.com
Mon Nov 19 01:57:22 PST 2012
Thanks for the ping. I've been working on these comments far too slowly. These are largely just style and aesthetic comments.
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:254-255
@@ +253,4 @@
+ // which is not yet implemented.
+ WarningFn = M.getOrInsertFunction(ClKeepGoing ? "__msan_warning" :
+ "__msan_warning_noreturn", IRB.getVoidTy(), NULL);
+
----------------
I would create a variable to hoist out the conditional expression:
lang=cpp
StringRef WarningFnName = ClKeepGoing ? "__msan_warning"
: "__msan_warning_noreturn";
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1233
@@ +1232,3 @@
+
+struct MemorySanitizerVarArgHelper_X86_64: public MemorySanitizerVarArgHelper {
+ // An unfortunate workaround for asymmetric lowering of va_arg stuff.
----------------
Evgeniy Stepanov wrote:
> Chandler Carruth wrote:
> > I think a helper *class* is overkill. Especially a dynamic class with virtual dispatch.
> >
> > Why not just a few helper functions? I was mostly looking for you to add a 'visitVarArgCallSite' or some such above.
> >
> > In particular, I think pulling all of this out here makes the above visitor a bit harder to read as it has an odd forwarding layer.
> >
> > That said, I do like the factoring you have here. I would just fold all of this into the above visitor class.
> That will get messy again if we add support for VarArg on other platforms, like x86_32. With the helper class we would just add a new implementation. With helper method we'd need a bunch of if(s) all around.
>
> If virtual dispatch overhead is what you are worried about, we can avoid that with some template magic, though I doubt it matters.
>
>
Ok, I'll take a look at the details of this abstraction then...
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:367-368
@@ +366,4 @@
+ getCleanShadow(ConvertedShadow), "_mscmp");
+ Instruction *CheckTerm = SplitBlockAndInsertIfThen(cast<Instruction>(Cmp),
+ /* Unreachable */ !ClKeepGoing, MS.ColdCallWeights);
+
----------------
I think this indenting doesn't work too well. How about:
Instruction *CheckTerm
= SplitBlockAndInsertIfThen(cast<Instruction>(Cmp),
/* Unreachable */ !ClKeepGoing,
MS.ColdCallWeights);
================
Comment at: llvm/include/llvm/IRBuilder.h:270-273
@@ -268,2 +269,6 @@
+ IntegerType* getIntPtrTy(DataLayout *DL, unsigned AddrSpace = 0) {
+ return Type::getIntNTy(Context, DL->getPointerSizeInBits(AddrSpace));
+ }
+
//===--------------------------------------------------------------------===//
----------------
Has this gone in as a separate change? If not, please just submit it, consider it reviewed. =]
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:473-474
@@ +472,4 @@
+ ///
+ /// OriginAddr = (ShadowAddr + OriginOffset) & ~3ULL
+ /// = Addr & (~ShadowAddr & ~3ULL) + OriginOffset
+ Value *getOriginPtr(Value *Addr, IRBuilder<> &IRB) {
----------------
BTW, this is a great comment. =] It helps a lot for the reader.
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:297
@@ +296,3 @@
+
+struct MemorySanitizerVarArgHelper {
+ /// \brief Visit a CallSite.
----------------
You should give a nice summary of the purpose of the abstraction here, especially focused on what the next subclass would likely look like.
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:316-317
@@ +315,4 @@
+
+MemorySanitizerVarArgHelper* CreateVarArgHelper(Function &Func, MemorySanitizer &Msan,
+ MemorySanitizerVisitor &Visitor);
+
----------------
80-columns, etc.
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:319-324
@@ +318,8 @@
+
+// This class does all the work for a given function. Store and Load
+// instructions store and load corresponding shadow and origin
+// values. Most instructions propagate shadow from arguments to their
+// return values. Certain instructions (most importantly, BranchInst)
+// test their shadow and print reports (with a runtime call) if it's
+// non-zero.
+struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
----------------
This should be a doxygen.
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:353-354
@@ +352,4 @@
+ DEBUG(if (!InsertChecks)
+ dbgs() << "MemorySanitizer is not inserting checks into '"
+ << F.getName() << "'\n");
+ }
----------------
I think this needs another 2-spaces of indent...
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:207
@@ +206,3 @@
+static GlobalVariable *createPrivateNonConstGlobalForString(Module &M,
+ StringRef Str) {
+ Constant *StrConst = ConstantDataArray::getString(M.getContext(), Str);
----------------
Indent seems off on this parameter. Please line up parameters after the (
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:348
@@ +347,3 @@
+
+ MemorySanitizerVisitor(Function &Func, MemorySanitizer &Msan)
+ : F(Func), MS(Msan),
----------------
In general, you should feel free to shadow the member names with the parameter names in constructors. It is guaranteed to do the right thing. For example:
MemorySanitizerVisitor(Function &F, MemorySanitizer &MS)
: F(F), MS(MS),
Should "just work". =]
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1226
@@ +1225,3 @@
+
+struct MemorySanitizerVarArgHelper_X86_64: public MemorySanitizerVarArgHelper {
+ // An unfortunate workaround for asymmetric lowering of va_arg stuff.
----------------
space before the ':' here
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1241
@@ +1240,3 @@
+ MemorySanitizerVarArgHelper_X86_64(Function &Func, MemorySanitizer &Msan,
+ MemorySanitizerVisitor &Visitor)
+ : F(Func), MS(Msan), MSV(Visitor), VAArgTLSCopy(0), VAArgOverflowSize(0) { }
----------------
See my comments about indenting elsewhere: generally, line up the parameters rather than just using a 4-space indent.
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1242
@@ +1241,3 @@
+ MemorySanitizerVisitor &Visitor)
+ : F(Func), MS(Msan), MSV(Visitor), VAArgTLSCopy(0), VAArgOverflowSize(0) { }
+
----------------
And here is another good place to use intentional shadowing of the member names.=
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1226
@@ +1225,3 @@
+
+struct MemorySanitizerVarArgHelper_X86_64: public MemorySanitizerVarArgHelper {
+ // An unfortunate workaround for asymmetric lowering of va_arg stuff.
----------------
Chandler Carruth wrote:
> space before the ':' here
Underscore-separated names are strongly avoided in LLVM's codebase.
A couple of observations at this point: these classes are file-local, so the "MemorySanitizer" bit in the name doesn't seem terribly important as long as they're in an anonymous namespace.
I would suggest "VarArgHelper" and "VarArgAMD64Helper". Especially as you cite the AMD64 ABI for many aspects of this, that seems more clear.
A doxygen comment of course will help clarify the role of this further.
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1244
@@ +1243,3 @@
+
+ enum ArgClass { ARG_GP, ARG_FP, ARG_MEMORY };
+
----------------
The coding conventions indicate this should be ArgKind, and the names should be AK_GeneralPurpose, AK_FloatingPoint, and AK_Memory (if I've decoded the names correctly).
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1295-1297
@@ +1294,5 @@
+ }
+ IRB.CreateStore(ConstantInt::get(MS.VAArgOverflowSizeTLS->getType()->
+ getElementType(), OverflowOffset - AMD64FpEndOffset),
+ MS.VAArgOverflowSizeTLS);
+ }
----------------
This line wrapping is very hard to understand.
Please use local variables and other techniques to fit lines in 80-columns, and find more natural ways to wrap lines.
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1290-1291
@@ +1289,4 @@
+ Base = getShadowPtrForVAArgument(A, IRB, OverflowOffset);
+ OverflowOffset += DataLayout::RoundUpAlignment(MS.TD->getTypeAllocSize(
+ A->getType()), 8);
+ }
----------------
Another place where 4-space indent makes the flow of this harder to read. Pulling the size out into a local variable removes the need for line wrapping I suspect.
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1301-1302
@@ +1300,4 @@
+ /// \brief Compute the shadow address for a given va_arg.
+ Value *getShadowPtrForVAArgument(Value *A, IRBuilder<> &IRB,
+ int ArgOffset) {
+ Value *Base = IRB.CreatePointerCast(MS.VAArgTLS, MS.IntptrTy);
----------------
Line up 'ArgOffset' with the open '(' please.
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1317-1318
@@ +1316,4 @@
+ // FIXME: magic ABI constants.
+ IRB.CreateMemSet(ShadowPtr, Constant::getNullValue(IRB.getInt8Ty()),
+ /* size */24, /* alignment */16, false);
+ }
----------------
Another place where arguments need to be lined up with the '('. I'll stop commenting on each of these, please audit them as this appears to be a common style problem.
http://llvm-reviews.chandlerc.com/D56
More information about the llvm-commits
mailing list