This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Verify externally referenced blocks against jump table targets
ClosedPublic

Authored by Amir on Aug 23 2022, 1:13 PM.

Details

Summary

For functions with references to internal offsets from data, verify externally
referenced blocks against the set of jump table targets. Mark the function
as non-simple if there are any unclaimed data to code references.

Diff Detail

Event Timeline

Amir created this revision.Aug 23 2022, 1:13 PM
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
Amir requested review of this revision.Aug 23 2022, 1:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2022, 1:13 PM
Amir updated this revision to Diff 454978.Aug 23 2022, 3:01 PM

Verify the set of internal references against jump table targets.

Amir retitled this revision from [BOLT] Improve recognition of unknown control flow to [BOLT] Verify externally referenced blocks against jump table targets.Aug 23 2022, 3:03 PM
Amir edited the summary of this revision. (Show Details)
rafauler added inline comments.Aug 23 2022, 3:04 PM
bolt/lib/Core/BinaryFunction.cpp
1861

What happens if we do have JumpTables and yet we have internal references that do not appear in any jump table?

1862

From reading this code I got the impression that UnknownControlFlow is only used in strict mode?

I guess I reviewed an older version of the diff

Amir planned changes to this revision.Aug 23 2022, 6:41 PM
Amir updated this revision to Diff 455257.Aug 24 2022, 9:43 AM

HasUnknownControlFlow -> !Strict

Amir planned changes to this revision.Aug 24 2022, 9:44 AM
Amir updated this revision to Diff 455335.Aug 24 2022, 12:56 PM

Move checks past HasFixedIndirectBranch

Amir updated this revision to Diff 455341.Aug 24 2022, 1:14 PM

clang-format

Amir marked 2 inline comments as done.Aug 25 2022, 1:46 PM
Amir updated this revision to Diff 455731.Aug 25 2022, 3:06 PM

Update warning message to make it clear for end user what's going on

bolt/lib/Core/BinaryFunction.cpp
1887

@ayermolo: hope the warning makes sense now

Amir updated this revision to Diff 455740.Aug 25 2022, 3:42 PM

Add extra check

Amir updated this revision to Diff 455794.Aug 25 2022, 10:02 PM

Mark as non-simple

Amir edited the summary of this revision. (Show Details)Aug 25 2022, 10:05 PM
Amir updated this revision to Diff 456007.Aug 26 2022, 1:21 PM

Added assembly test

Amir updated this revision to Diff 456051.Aug 26 2022, 4:08 PM

Fix shrinkwrapping-restore-position test

Amir updated this revision to Diff 456833.Aug 30 2022, 6:32 PM

Add runtime test checking if the moved code with external data references to it
works correctly. If there's a reference to offset which is not a registered BB
or label, ignore the function.

Amir updated this revision to Diff 456837.Aug 30 2022, 6:44 PM

clang-format

Amir updated this revision to Diff 456839.Aug 30 2022, 6:58 PM

Check for constant islands

maksfb added inline comments.Sep 1 2022, 11:05 AM
bolt/lib/Core/BinaryFunction.cpp
1875

Let's refactor the code into a function since it's quite large.

1880
1895
1897
1903
1904
Amir updated this revision to Diff 457745.Sep 2 2022, 6:41 PM

Move checks into BF::validateExternalReferences

Amir updated this revision to Diff 457746.Sep 2 2022, 6:47 PM

Address nits

Amir marked 6 inline comments as done.Sep 2 2022, 6:48 PM
Amir updated this revision to Diff 458612.Sep 7 2022, 5:21 PM

rebase

maksfb added inline comments.Sep 15 2022, 5:34 PM
bolt/include/bolt/Core/BinaryFunction.h
2108 ↗(On Diff #458612)

It's reasonable to expect for validate...() function to return true if all checks pass. Please invert the return code.

bolt/lib/Core/BinaryFunction.cpp
1763

Is there a reason to keep Found in a variable?

1775

If you don't expect this warning to be firing frequently, let's print it at default verbosity level.

Amir updated this revision to Diff 460575.Sep 15 2022, 5:46 PM

Address Maksim's comments

Amir marked 3 inline comments as done.Sep 15 2022, 5:46 PM
maksfb accepted this revision.Sep 15 2022, 6:04 PM

LGTM, but please rename the method before the commit.

bolt/lib/Core/BinaryFunction.cpp
1875–1876

There's an apparent confusion with "internal" vs "external" reference in this context. Let's rename the method to validateExternallyReferencedOffsets() or validateReferencedOffsets() to avoid it.

This revision is now accepted and ready to land.Sep 15 2022, 6:04 PM
Amir updated this revision to Diff 460793.Sep 16 2022, 9:09 AM

Rename method to validateExternallyReferencedOffsets

Amir marked an inline comment as done.Sep 16 2022, 9:10 AM
Amir updated this revision to Diff 460808.Sep 16 2022, 9:55 AM

clang-format