[PATCH] D131631: [MSAN] Separate id ptr from constant string for variable names used in track origins.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/msan/msan.cpp:315-318
+  if (id_ptr == nullptr) {
+    id_ptr = (u32 *)descr;
+    description = descr + 4;
+  }
----------------
we don't want a branch on fast path when we can do +4 in the caller


================
Comment at: compiler-rt/lib/msan/msan.cpp:615
 void __msan_set_alloca_origin(void *a, uptr size, char *descr) {
-  SetAllocaOrigin(a, size, descr,
+  SetAllocaOrigin(a, size, nullptr, descr,
                   StackTrace::GetPreviousInstructionPc(GET_CALLER_PC()));
----------------



================
Comment at: compiler-rt/lib/msan/msan.cpp:623
   // older clang.
-  SetAllocaOrigin(a, size, descr,
+  SetAllocaOrigin(a, size, nullptr, descr,
+                  StackTrace::GetPreviousInstructionPc(GET_CALLER_PC()));
----------------
I guess existing code should work just fine


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:239
        cl::Hidden, cl::init(0xff));
 
 static cl::opt<bool> ClPoisonUndef("msan-poison-undef",
----------------
kda wrote:
> vitalybuka wrote:
> > ```
> > static cl::opt<bool> ClPrintStacklNames("msan-print-stack-names",
> >        cl::desc("Print name of local stack variable"),
> >        cl::Hidden, cl::init(true));
> > ```
> What is this meant to do?
maybe don't add ClPrintStacklNames here
a follow up patch would be better


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:3904
+      Value *Descr = getLocalVarDescription(I);
       IRB.CreateCall(MS.MsanSetAllocaOriginFn,
                      {IRB.CreatePointerCast(&I, IRB.getInt8PtrTy()), Len,
----------------
kda wrote:
> vitalybuka wrote:
> > ```
> > if (ClPrintLocalVNames) {
> >   IRB.CreateCall(MS.MsanSetAllocaOriginNameFn, ...
> > } else {
> >   IRB.CreateCall(MS.MsanSetAllocaOriginFn, ...
> > }
> > ```
> What is the goal here?
> Is this to allow the variable names to be dropped from the binary to save even more size?
yes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131631/new/

https://reviews.llvm.org/D131631