Page MenuHomePhabricator

[NewPM][TSan] Reiterate the TSan port
ClosedPublic

Authored by philip.pfaffe on Jan 10 2019, 4:15 AM.

Details

Summary

Second iteration of D56433 which got reverted in rL350719. The problem
in the previous version was that we dropped the thunk calling the tsan init
function. The new version keeps the thunk which should appease dyld, but is not
actually OK wrt. the current semantics of function passes.

Diff Detail

Repository
rC Clang

Event Timeline

philip.pfaffe created this revision.Jan 10 2019, 4:15 AM

Add missing license to TSan header.

Just a side note - seeing a problem with dyld wrt original tsan_init solution, do we have the same problem with msan_init, which was similarly solved in D55647?

vitalybuka added inline comments.Jan 10 2019, 5:15 PM
llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
91 ↗(On Diff #181026)

msan -> tsan runtime

llvm/test/Instrumentation/ThreadSanitizer/tsan_basic.ll
2 ↗(On Diff #181026)

what is about other tests?

Fix a comment.

philip.pfaffe marked 2 inline comments as done.Jan 11 2019, 2:08 AM
philip.pfaffe added inline comments.
llvm/test/Instrumentation/ThreadSanitizer/tsan_basic.ll
2 ↗(On Diff #181026)

I don't need to test tsan, just the pass pipeline specifics. The other tests appeared to mainly test tsan specifics, so I didn't touch them.

I can absolutely port the others, but that will (in theory at least) double the runtime. Do you have suggestions which other tests are meaningful to the pipeline?

Insert the constructor and init functions only once per module.

Do you think the current check on the ctor function is sufficient, or should I
also check whether it actually contains the init call as well?

chandlerc accepted this revision.Jan 15 2019, 3:40 AM

OK, this makes sense to me now. Minor comment below, but LGTM with that.

llvm/lib/Transforms/Utils/ModuleUtils.cpp
183–184 ↗(On Diff #181237)

Can you add a FIXME to sink the get-or-insert style logic into the module API much like we have for globals?

That will (eventually) make this much easier to move into a concurrent model.

This revision is now accepted and ready to land.Jan 15 2019, 3:40 AM

Execute appendtoGlobalCtors lazily as well. Previously the inserted ctor was
still added multiple times to the global ctors list. Doing this lazily requires
knowing when the ctor is actually created, so I added a callback to handle the
insertion.

philip.pfaffe marked an inline comment as done.Jan 15 2019, 10:52 AM
chandlerc accepted this revision.Jan 16 2019, 1:22 AM

Still LGTM.

Oh, and a nit. Minor though.

llvm/include/llvm/Transforms/Utils/ModuleUtils.h
71 ↗(On Diff #181821)

Use a llvm::function_ref here? Should be much cheaper.

Closed by commit rC351314: [NewPM][TSan] Reiterate the TSan port (authored by pfaffe, committed by ). · Explain WhyJan 16 2019, 1:31 AM
This revision was automatically updated to reflect the committed changes.