This is an archive of the discontinued LLVM Phabricator instance.

[NewPM] Port Msan: Second alternative approach without any module level initalization
ClosedPublic

Authored by philip.pfaffe on Dec 13 2018, 3:18 AM.

Details

Summary

Keeping msan a function pass requires replacing the module level initialization:
That means, don't define a ctor function which calls __msan_init, instead just
declare the init function at the first access, and add that to the global ctors
list.

Changes:

  • Pull sanitizer and wrapper pass apart
  • Add a newpm msan
  • Update tests

Caveats:

  • There is one test that I dropped, because it specifically tested the definition of the ctor.

Event Timeline

philip.pfaffe created this revision.Dec 13 2018, 3:18 AM
vitalybuka added inline comments.Dec 13 2018, 4:44 PM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
859

Does this make Module Pass instead of Function Pass a more appropriate choice?
Another reason do to so is to make it consistent with AddressSanitizer.

philip.pfaffe marked an inline comment as done.Dec 14 2018, 2:11 AM
philip.pfaffe added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
859

No it doesn't, function passes may declare other functions, but they aren't allowed to define them.

The whole point of this approach is to not make it a module pass, to retain the locality of function pipelines.

fedor.sergeev accepted this revision.Dec 21 2018, 4:59 AM

I'm not very faimiliar with details of llvm.global_ctors, but solution makes sense.
Code looks good bar minor access nits.
Thus, LGTM.

llvm/include/llvm/Transforms/Instrumentation.h
243–245

Do you need these public? I guess not.
Similar for Legacy version.

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

It seems that you dont need LegacyPass to be a friend.

This revision is now accepted and ready to land.Dec 21 2018, 4:59 AM
chandlerc requested changes to this revision.Dec 23 2018, 1:35 AM

FWIW, I really like the design. Mostly commenting about the code and structure of everything here, but I do think this needs to be updated before it goes in...

llvm/include/llvm/Transforms/Instrumentation.h
235

As I had commented in another version of this, I'd rather a header specifically for the MSan pass rather than adding more and more code to this header.

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
449–453

This still needs a major documentation update to match the new semantics here.

711

Shouldn't this be added to GlobalVariable or some other common place so other sanitizers can use a similar approach?

And it should probably be spelled something like getOrInsert... to match what we do for functions.

855

This functionality is likely to be common to all the sanitizers. Maybe this should either replace or go next to the common checkSanitizerInterfaceFunction bits?

965

Rather than a unique pointer, how about just an optional? that way we don't need separate storage.

This revision now requires changes to proceed.Dec 23 2018, 1:35 AM
philip.pfaffe marked 7 inline comments as done.

Address comments:

  • Move init creator function next to the ctor function
  • Use Module::getOrInsertGlobal instead of my own version, but modify it to fit sanitizer neads

Should the getOrInsertGlobal be its own patch?

chandlerc requested changes to this revision.Dec 26 2018, 6:14 PM

Nice! Only important comment is really about the changes to the module API...

Should the getOrInsertGlobal be its own patch?

Yeah, that makes a lot of sense I think (also we can continue discussing the right API there as I have a question about it below that might get lost....

And once that is good and lands, I think the rest of this patch looks good (w/ comments below addressed). Marking as "needing changes" mostly due to this splitting.

llvm/include/llvm/IR/Module.h
410 ↗(On Diff #179465)

This sentence would be easier for me to read as: "Note that the optional arguments are not considered for matching the global and only have effect when this function needs to add a declaration."

llvm/include/llvm/Transforms/Instrumentation.h
156–159

Move this into the new msan header too?

llvm/include/llvm/Transforms/Instrumentation/MemorySanitizer.h
20–22 ↗(On Diff #179465)

Can you document the design pattern we end up following? No need to be super detailed, just mention that this function pass may insert globals into the module the first time they are required, and otherwise will use the globals already present.

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

Should be the first header?

455

"alread" -> "already"

"In that case" -> a bit ambiguous what case is referred to here. Maybe can just state this more simply: "Also ensures the __msan_init function is in the list of global constructors for the module." or some such.

This revision now requires changes to proceed.Dec 26 2018, 6:14 PM
philip.pfaffe added inline comments.Dec 27 2018, 5:53 AM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
965

I'm in favor, but MSan is currently not movable thanks to MapParams. So I can implement the move, or keep the extra storage (in which I should also explicitly delete the move ctor). Any preference? Keeping the unique_ptr seemed easier and less error prone, and the indirection isn't really hot (taken once per function), so I went that way.

philip.pfaffe marked 5 inline comments as done.

Addressed comments and rebased on top of rL350219.

chandlerc accepted this revision.Jan 2 2019, 7:00 PM

Wheeee! I think this is good to go! I really like the end state here.

I've checked with Vitaly as well and I think you're good-to-go here. I've left some further comments below, but I'm happy for you to land with those changes or send them as follow-ups. I'd like to start helping out here w/ other sanitizers now that we (hopefully) have a design worked out that looks like it will more reliably work.

Thanks so much for sticking through the process to find a good way to handle this level of complex pass!!!

llvm/lib/Passes/PassRegistry.def
233

Happy for it to be a follow-up commit, but we should probably support options for the sanitizers -- these are pretty common for folks to want to parameterize.

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

I think it would be a touch cleaner to make this a helper for actually creating the global, and just use it in the (or as) the lambda. I suspect you could even make it a lambda inside the module initialization routine.

Anyways, this refactoring can be a follow-up, or I can even send you a patch afterward so its easier to see what I'm thinking.

965

I think you can use the emplace method to deal with this and use Optional? But either way is fine to start with.

This revision is now accepted and ready to land.Jan 2 2019, 7:00 PM
This revision was automatically updated to reflect the committed changes.
llvm/test/Instrumentation/MemorySanitizer/check_access_address.ll