This is an archive of the discontinued LLVM Phabricator instance.

RuntimeDyldELF: Don't abort on R_AARCH64_NONE relocation
ClosedPublic

Authored by yota9 on Jan 11 2022, 4:43 PM.

Details

Summary

Do nothing on R_AARCH64_NONE relocation. The relocation is used by BOLT when re-linking the final binary. It is used as a dummy relocation hack in order to stop the RuntimeDyld to skip the allocation of the section.

Diff Detail

Event Timeline

yota9 created this revision.Jan 11 2022, 4:43 PM
yota9 requested review of this revision.Jan 11 2022, 4:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2022, 4:43 PM

How does BOLT use the relocation type?

(BOLT is in the monorepo now) If you give instructions, reviewers may have a better idea.
While I do not know how ExecutionEngine handles the situation, in other ELF components I review/contribute we really need a test for every relocation type.
This is important to not regress while refactoring code.

yota9 added a comment.EditedJan 11 2022, 5:34 PM

Hello @MaskRay . It is used in DwarfLineTable::emitCU as a dummy relocation hack in order to stop the RuntimeDyld to skip the allocation of the section. Also I would like to point that all the other targets have the proper R_*_NONE handling :)

yota9 updated this revision to Diff 399286.Jan 12 2022, 4:03 AM

Add test

yota9 edited the summary of this revision. (Show Details)Jan 12 2022, 4:07 AM
yota9 removed a reviewer: rafaelauler.
yota9 added subscribers: rafaelauler, maksfb.
lhames accepted this revision.Jan 12 2022, 12:59 PM

LGTM.

This revision is now accepted and ready to land.Jan 12 2022, 12:59 PM
MaskRay added inline comments.Jan 12 2022, 1:14 PM
llvm/test/ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_none.yaml
4 ↗(On Diff #399286)

. after a complete sentence.

In other ELF components we prefer ## for non-RUN non-CHECK comments. I haven't checked whether test/ExecutionEngine wants to follow this convention.

24 ↗(On Diff #399286)

remove

R_AARCH64_NONE doesn't need a symbol

MaskRay accepted this revision.Jan 12 2022, 1:15 PM

I'd suggest AARCH64_NONE instead of ARM64_none

yota9 added a comment.Jan 12 2022, 1:32 PM

@MaskRay I don't mind to rename it, I've named it with ARM64 because most of the tests in this directory are named with arm64.

OK. I don't mind that much. Just noticed that the directory name is AArch64/. In ELF, various AArch64/ARM64 related constants are called AARCH64 instead of ARM64.

yota9 marked 2 inline comments as done.Jan 13 2022, 12:34 AM

Done

This revision was landed with ongoing or failed builds.Jan 13 2022, 12:56 AM
This revision was automatically updated to reflect the committed changes.