This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Allow function fragments to point to one jump table
ClosedPublic

Authored by nhuhuan on Jun 15 2022, 5:23 PM.

Details

Summary

Resolve a crash related to split functions

Due to split function optimization, a function can be divided to two

fragments, and both fragments can access same jump table. This
violates 
the assumption that a jump table can only have one parent
function, 
which causes a crash during instrumentation.

We want to support the case: different functions cannot access same
jump tables, but different fragments of same function can!

As all fragments are from same function, we point JT::Parent to one
specific fragment. Right now it is the first disassembled fragment, but
we can point it to the function's main fragment later.

Functions are disassembled sequentially. Previously, at the end of
processing a function, JT::OffsetEntries is cleared, so other fragment
can no longer reuse JT::OffsetEntries. To extend the support for split
function, we only clear JT::OffsetEntries after all functions are
disassembled.

Let say A.hot and A.cold access JT of three targets {X, Y, Z}, where
X and Y are in A.hot, and Z is in A.cold. Suppose that A.hot is
disassembled first, JT::OffsetEntries = {X',Y',INVALID_OFFSET}. When
A.cold is disassembled, it cannot reuse JT::OffsetEntries above due to
different fragment start. A simple solution:
A.hot = {X',Y',INVALID_OFFSET}
A.cold = {INVALID_OFFSET, INVALID_OFFSET, INVALID_OFFSET}

We update the assertion to allow different fragments of same function
to get the same JumpTable object.

Potential improvements:
A.hot = {X',Y',INVALID_OFFSET}
A.cold = {INVALID_OFFSET, INVALID_OFFSET, Z'}
The main issue is A.hot and A.cold have separate CFGs, thus jump table
targets are still constrained within fragment bounds.

Future improvements:
A.hot = {X, Y, Z}
A.cold = {X, Y, Z}

Diff Detail

Event Timeline

nhuhuan created this revision.Jun 15 2022, 5:23 PM
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: ayermolo. · View Herald Transcript
nhuhuan requested review of this revision.Jun 15 2022, 5:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 5:23 PM
nhuhuan edited the summary of this revision. (Show Details)Jun 15 2022, 5:27 PM
nhuhuan edited the summary of this revision. (Show Details)
nhuhuan updated this revision to Diff 437416.Jun 15 2022, 6:03 PM

Include all the changes

Amir added a comment.Jun 15 2022, 7:14 PM

Please retitle the diff to state what is changed - e.g. "allow function fragments point to one jump table".

I like the summary, it explains the issue in a clear manner. Some nits:

  • no need to explicitly mention python,
  • I would rephrase 'next goal' and 'stretch goal' as potential or future improvements.
  • please run a spell checker.
bolt/test/X86/split-func-jump-table-fragment-bidirection.s
5

Please updated the message

Amir added inline comments.Jun 15 2022, 7:27 PM
bolt/test/X86/split-func-jump-table-fragment-bidirection.s
12
nhuhuan retitled this revision from [BOLT] Resolve crash when optimizing Python to [BOLT] Allow function fragments to point to one jump table.Jun 16 2022, 10:22 AM
nhuhuan edited the summary of this revision. (Show Details)
nhuhuan updated this revision to Diff 437614.Jun 16 2022, 11:13 AM

Update manual test

This revision is now accepted and ready to land.Jun 16 2022, 1:26 PM
nhuhuan updated this revision to Diff 438013.Jun 17 2022, 12:45 PM
nhuhuan marked 2 inline comments as done.

clang-formatted changes

This revision was automatically updated to reflect the committed changes.