This is an archive of the discontinued LLVM Phabricator instance.

[BOLT][AArch64] Handle data at the beginning of a function when disassembling and building CFG.
ClosedPublic

Authored by treapster on Jun 6 2022, 6:42 AM.

Details

Summary

This patch adds getFirstInstructionOffset method for BinaryFunction which is used to properly handle cases where data is at zero offset in a function. The main change is that we add basic block at first instruction offset when disassembling, which prevents assertion failures in buildCFG.

Diff Detail

Event Timeline

treapster created this revision.Jun 6 2022, 6:42 AM
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
treapster requested review of this revision.Jun 6 2022, 6:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 6:42 AM
yota9 accepted this revision.Jun 6 2022, 6:50 AM

Overall LGTM

bolt/test/AArch64/data-at-0-offset.c
4

Which warning is supposed to be here? Please update with more specific string

This revision is now accepted and ready to land.Jun 6 2022, 6:50 AM
yota9 added inline comments.Jun 6 2022, 6:52 AM
bolt/test/AArch64/data-at-0-offset.c
2
treapster updated this revision to Diff 434475.Jun 6 2022, 7:50 AM

Seems like clang used on buildkite doesn't mark .space directive as data, changed test to use repeated .byte instead

treapster updated this revision to Diff 434482.Jun 6 2022, 8:12 AM

Add more specific warning

treapster added inline comments.Jun 6 2022, 8:25 AM
bolt/test/AArch64/data-at-0-offset.c
4

Well, without my patch it just fails with assertion llvm-project/bolt/lib/Core/BinaryFunction.cpp:1982: bool llvm::bolt::BinaryFunction::buildCFG(llvm::bolt::MCPlusBuilder::AllocatorIdTy): Assertion 'PrevBB && "no previous basic block for a fall through"' failed. But just in case i now added check for disassembling failure.

yota9 requested changes to this revision.Jun 7 2022, 8:49 AM

Please fix the test

This revision now requires changes to proceed.Jun 7 2022, 8:49 AM
treapster updated this revision to Diff 435140.Jun 8 2022, 7:00 AM
treapster marked an inline comment as done.

(Presumably) Fix test

Please fix the test

Done

yota9 accepted this revision.Jun 8 2022, 9:11 AM

LGTM

This revision is now accepted and ready to land.Jun 8 2022, 9:11 AM
rafauler accepted this revision.Jun 8 2022, 3:05 PM

Thanks for reviewing @yota9 and thanks for your contribution @treapster. Do you need me to commit this?

@rafauler, yes, it'll be cool if you commit. My author string is Denis Revunov <revunov.denis@huawei-partners.com>. Thanks in advance!