[llvm-commits] [PATCH] MemorySanitizer instrumentation pass.

[Chandler Carruth]

  I think this can be submitted after fixing the style issues i've noted. I'm anxious to move into smaller incremental patches as well. =]

  BTW, Sorry for being so strict on style, but it's very important to the community and the codebase. I know it can be pedantic and annoying, but I and others greatly appreciate your work to find consistent and clean patterns for the code that match those in the rest of the project. I await the day that clang-format will automate all of this for us...


================
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);
+
----------------
Evgeniy Stepanov wrote:
> Chandler Carruth wrote:
> > I think this indenting doesn't work too well. How about:
> > 
> >         Instruction *CheckTerm
> >           = SplitBlockAndInsertIfThen(cast<Instruction>(Cmp),
> >                                       /* Unreachable */ !ClKeepGoing,
> >                                       MS.ColdCallWeights);
> Are you sure that "=" goes to the new line?
either way...

================
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) {
----------------
Evgeniy Stepanov wrote:
> Chandler Carruth wrote:
> > BTW, this is a great comment. =] It helps a lot for the reader.
> And it's also incorrect :)
> I'm not sure what else to say here. Do you mean it's too verbose and I should remove it?
No no! I just wanted to give some positive feedback.

I often feel like in code reviews, all I'm doing is complaining. Usually, there is a lot of internal monologue along the lines of "Oh wow, yea, that looks *soo* nice now! I'm so glad that change worked out!" etc. It's a personal goal of mine to try to surface at least one or two of these as actual code review comments that are *positive* comments on how good the patch is looking.

Hopefully it helps a tiny bit with the long slog of big initial patches, and lots of stylistic comments.

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1401
@@ +1400,3 @@
+VarArgHelper* CreateVarArgHelper(Function &Func, MemorySanitizer &Msan,
+    MemorySanitizerVisitor &Visitor) {
+  return new VarArgAMD64Helper(Func, Msan, Visitor);
----------------
Still some places at least where things haven't been lined up with the opening '('.

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:630
@@ +629,3 @@
+            setOrigin(A, EntryIRB.CreateLoad(
+              getOriginPtrForArgument(AI, EntryIRB, ArgOffset)));
+        }
----------------
I think a local variable is more clear here than trying to tell (as a reader) whether this line is an argument to setOrigin or CreateLoad....

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1415
@@ +1414,3 @@
+  F.removeAttribute(AttrListPtr::FunctionIndex,
+    Attributes::get(F.getContext(), B));
+
----------------
another odd indent after (


http://llvm-reviews.chandlerc.com/D56