Page MenuHomePhabricator

[MSAN] Reintroduce libatomic load/store instrumentation
ClosedPublic

Authored by guiand on Aug 7 2020, 2:33 PM.

Details

Summary
Have the front-end use the `nounwind` attribute on atomic libcalls.
This prevents us from seeing `invoke __atomic_load` in MSAN, which
is problematic as it has no successor for instrumentation to be added.

Diff Detail

Event Timeline

guiand created this revision.Aug 7 2020, 2:33 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 7 2020, 2:33 PM
Herald added subscribers: llvm-commits, Restricted Project, jfb, hiraditya. · View Herald Transcript
guiand requested review of this revision.Aug 7 2020, 2:33 PM
guiand added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3518

TODO: Remove

__libatomic_load might come at the end of the function, with no succeeding BB

Not exactly. It may come at the end of a BB.

We should probably fix it in clang by adding nounwind to libatomic calls, and ignore invokes here.

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

Check if the function already exists.

3510

addVisibility(GlobalValue::HiddenVisibility);

guiand updated this revision to Diff 284076.Aug 7 2020, 3:50 PM
guiand edited the summary of this revision. (Show Details)
guiand added a reviewer: rsmith.

Simplified by returning to the old implementation, but having libatomic calls made nounwind (so we never see them as invokes).

Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2020, 3:50 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
guiand updated this revision to Diff 284079.Aug 7 2020, 3:51 PM
guiand marked 2 inline comments as done.

Rebased on master

guiand updated this revision to Diff 284080.Aug 7 2020, 3:54 PM

Rebased on master (again)

eugenis added inline comments.Aug 7 2020, 4:03 PM
clang/lib/CodeGen/CGAtomic.cpp
315 ↗(On Diff #284080)

This needs a clang test, and better move it to a separate change.

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

Better explicitly check that this an invoke and not call. Call always has a next instruction.

And move this check to the parent function to apply regular call instrumentation to the unrecognized call.

3511

It has a successor. Two successors, actually.
I'd rather say something like "invoke of __atomic_load".

guiand updated this revision to Diff 284102.Aug 7 2020, 5:26 PM
guiand removed a reviewer: rsmith.

Separated out the frontend change. Addressed other comments.

guiand marked 2 inline comments as done.
guiand added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3513

TODO: Remove this redudant call

eugenis added inline comments.Aug 13 2020, 3:53 PM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3513

you've already checked it with assert(isa<CallInst>) - which could be changed to isTerminator(), and this could be simplified to NextIRB(CB.getNextNode()).

guiand updated this revision to Diff 285572.Aug 14 2020, 12:52 AM

Addressed comments

guiand marked an inline comment as done.Aug 14 2020, 12:53 AM
eugenis accepted this revision.Aug 14 2020, 12:47 PM

LGTM with 1 comment

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

Probably need the same check for atomic_store.

This revision is now accepted and ready to land.Aug 14 2020, 12:47 PM
guiand added inline comments.Aug 14 2020, 12:56 PM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3573

Although it would be good for consistency I don't think it's strictly needed since with atomic_store we don't need to use getNextNode()

eugenis added inline comments.Aug 14 2020, 1:03 PM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3573

ah, good point. LGTM