This is an archive of the discontinued LLVM Phabricator instance.

[ELF][PPC64] Fix assertion failure for branches to hidden undefined weak for -no-pie
ClosedPublic

Authored by MaskRay on Feb 14 2022, 2:54 PM.

Details

Summary

Reported by Stefan Pintilie in D119773.

For a branch to a hidden undefined weak symbol, there is an
assert(sym->getVA()); failure in PPC64LongBranchTargetSection::writeTo for a
-no-pie link. The root cause is that we unnecessarily produce a dynamic
relocation for a -no-pie link.

Fix this by changing the condition to just s.isUndefined(). See the inline
comment.

Rename ppc64-weak-undef-call.s to ppc64-undefined-weak.s to be consistent with
other architectures.

Diff Detail

Event Timeline

MaskRay created this revision.Feb 14 2022, 2:54 PM
MaskRay requested review of this revision.Feb 14 2022, 2:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2022, 2:54 PM

@stefanp I use this opportunity to clean up some tests, so I have created a new patch.

ppc64 psABI may want to clarify the behavior (aarch64 psABI defines the behavior for a branch to an undefined weak, though I think "branch to the next instruction" is a bad choice).
I have documented what I know in https://maskray.me/blog/2021-04-25-weak-symbol#branch-relocations

I find that GNU ld's powerpc64 port may change the bl instruction to nop in some cases. I think that is not great, either, since the run-time undefined behavior (branch to undefined weak) is not detected.

I will back port whatever fix into release/14.x

MaskRay updated this revision to Diff 408637.Feb 14 2022, 3:06 PM

add a test for undefined non-weak

stefanp accepted this revision as: stefanp.Feb 15 2022, 12:02 PM

I checked and this fixes the issue we were seeing.
Thank you for fixing this!

LGTM

This revision is now accepted and ready to land.Feb 15 2022, 12:02 PM
Miss_Grape added inline comments.
lld/ELF/Arch/PPC64.cpp
1386

Why externally referenced symbols, this is not the type of undefined symbols here?I would like to know what kind of scenario is being dealt with here

Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2022, 7:51 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
MaskRay marked an inline comment as done.Apr 18 2022, 8:24 PM
MaskRay added inline comments.
lld/ELF/Arch/PPC64.cpp
1386

I do not understand the question. "externally referenced symbol" is not a linker term. The symbol type is irrelevant. The condition has been explained in the updated comment.

lld/test/ELF/ppc64-undefined.s