This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Fix state of MCSymbols in lowering pass
ClosedPublic

Authored by rafauler on Nov 14 2022, 2:17 PM.

Details

Reviewers
Amir
maksfb
Group Reviewers
Restricted Project
Commits
rG62a2feff5784: [BOLT] Fix state of MCSymbols in lowering pass
Summary

We have mostly harmless data races when running
BinaryContext::calculateEmittedSize() in parallel, while performing
split function pass. However, it is possible to end up in a state
where some MCSymbols are still registered and our clean up
failed. This happens rarely but it does happen, and when it happens,
it is a difficult to diagnose heisenbug. To avoid this, add a new
clean pass to perform a last check on MCSymbols, before they
undergo our final emission pass, to verify that they are in a sane
state. If we fail to do this, we might resolve some symbols to zero
and crash the output binary.

Diff Detail

Event Timeline

rafauler created this revision.Nov 14 2022, 2:17 PM
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
rafauler requested review of this revision.Nov 14 2022, 2:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2022, 2:17 PM
Amir accepted this revision.Nov 14 2022, 3:16 PM

Thank you for fixing this non-determinism!
LGTM sans clang-format issue. I wish there was a way to clear symbol state without doing the series of dyn_casts.

This revision is now accepted and ready to land.Nov 14 2022, 3:16 PM
rafauler updated this revision to Diff 476536.Nov 18 2022, 11:28 AM

Fix clang format

tschuett added inline comments.
bolt/lib/Passes/BinaryPasses.cpp
599

static

rafauler added inline comments.Nov 18 2022, 11:50 AM
bolt/lib/Passes/BinaryPasses.cpp
599

this is under an anonymous namespace

tschuett added inline comments.Nov 18 2022, 12:01 PM
bolt/lib/Passes/BinaryPasses.cpp
599

Exactly. You are supposed to use static instead of anonymous namespaces. It is about readability. If I look at resetMCSymbolState, then I cannot see that it has internal linkage. static makes the job simpler.

rafauler added inline comments.Nov 18 2022, 1:02 PM
bolt/lib/Passes/BinaryPasses.cpp
599

Oh, I didn't know/remember this rule changed in LLVM coding standards. Sounds good, thanks.

Ref for other readers: https://llvm.org/docs/CodingStandards.html#id58

rafauler updated this revision to Diff 476579.Nov 18 2022, 1:04 PM

Convert anonymous namespace -> static

rafauler updated this revision to Diff 476631.Nov 18 2022, 4:23 PM

Use a simpler strategy suggested by @maksfb, iterating over MCContext
symbol table.

rafauler edited the summary of this revision. (Show Details)Nov 18 2022, 4:24 PM

This new pass typically runs in the order of less than 100ms for very large binaries, so it does not introduce any important overhead to our pipeline.

This revision was automatically updated to reflect the committed changes.