This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kda on Aug 10 2022, 4:30 PM.

Details

Summary

The goal is to reduce the size of the MSAN with track origins binary, by making
the variable name locations constant which will allow the linker to compress
them.

Follows: https://reviews.llvm.org/D131415

Diff Detail

Event Timeline

kda created this revision.Aug 10 2022, 4:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 4:30 PM
kda requested review of this revision.Aug 10 2022, 4:30 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 10 2022, 4:30 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
vitalybuka added inline comments.Aug 10 2022, 5:18 PM
compiler-rt/lib/msan/msan_interface_internal.h
111–114

we should not change interface of existing function and just add:
void __msan_set_alloca_origin_with_name(void *a, uptr size, u32 *id_ptr, char *descr);

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
239
static cl::opt<bool> ClPrintStacklNames("msan-print-stack-names",
       cl::desc("Print name of local stack variable"),
       cl::Hidden, cl::init(true));
248
static cl::opt<bool> ClPrintLocalNames("msan-print-local-names",
       cl::desc("Print name of local stack variable"),
       cl::Hidden, cl::init(true));
688–689

commit base is wrong?

3898
if (ClPrintLocalVNames) {
  IRB.CreateCall(MS.MsanSetAllocaOriginNameFn, ...
} else {
  IRB.CreateCall(MS.MsanSetAllocaOriginFn, ...
}

Please re-base, I made some changes to fix PPC64.

kda updated this revision to Diff 451751.Aug 11 2022, 12:23 AM
kda marked 2 inline comments as done.

rework from scratch

kda added inline comments.Aug 11 2022, 12:24 AM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
239

What is this meant to do?

3898

What is the goal here?
Is this to allow the variable names to be dropped from the binary to save even more size?

vitalybuka added inline comments.Aug 11 2022, 9:28 AM
compiler-rt/lib/msan/msan.cpp
317–320

we don't want a branch on fast path when we can do +4 in the caller

612
620–626

I guess existing code should work just fine

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
239

maybe don't add ClPrintStacklNames here
a follow up patch would be better

3898

yes

vitalybuka added inline comments.Aug 11 2022, 9:29 AM
compiler-rt/lib/msan/msan_interface_internal.h
111–114

weird naming, only now I realized what if 4 stands for :)
I guess it's fine for internal call

kda updated this revision to Diff 451961.Aug 11 2022, 1:22 PM
kda marked 6 inline comments as done.

removed branch and improved comments

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3898

I'm going to do this in a follow up CL, because I think it requires adding another function that has the Idptr parameter, but not the Descr, as well as having to modify msan_report to handle nullptr for description.

vitalybuka added inline comments.Aug 11 2022, 2:12 PM
compiler-rt/lib/msan/msan_interface_internal.h
113

we don't need pc in this function

llvm/test/Instrumentation/MemorySanitizer/alloca.ll
26

looks like we don't test last id and descr at all
Let's land D131721 and then you can test a new code same way

kda updated this revision to Diff 451999.Aug 11 2022, 3:17 PM

testing update with squash and rebase

kda updated this revision to Diff 452026.Aug 11 2022, 4:11 PM
kda marked an inline comment as done.

rebase

kda updated this revision to Diff 452053.Aug 11 2022, 7:07 PM

fix IR test

vitalybuka added inline comments.Aug 11 2022, 8:23 PM
llvm/test/Instrumentation/MemorySanitizer/alloca.ll
16–17

we need another line for the int32

kda updated this revision to Diff 452067.Aug 11 2022, 8:29 PM
kda marked an inline comment as done.

add check for IDPTR in IR test

This revision is now accepted and ready to land.Aug 11 2022, 9:13 PM
This revision was landed with ongoing or failed builds.Aug 12 2022, 8:47 AM
This revision was automatically updated to reflect the committed changes.