Page MenuHomePhabricator

[ELF] Define a reportRangeError() overload for thunks and tidy up recent PPC64 thunk range errors
ClosedPublic

Authored by MaskRay on Thu, Sep 10, 4:45 PM.

Details

Summary

Prefer errorOrWarn to fatal for recoverable errors and graceful degradation
when --noinhibit-exec is specified.

Mention the destination symbol, otherwise the diagnostic is not really actionable.
Two errors are not tested but the patch does not intend to add the coverage.

Diff Detail

Event Timeline

MaskRay created this revision.Thu, Sep 10, 4:45 PM
MaskRay requested review of this revision.Thu, Sep 10, 4:45 PM
MaskRay updated this revision to Diff 291101.Thu, Sep 10, 4:46 PM

Remove an unneeded change

grimar added inline comments.Fri, Sep 11, 1:42 AM
lld/ELF/Relocations.cpp
116

I wonder if it should be a local helper function in Thunks.cpp, as it does not used anywhere else?

lld/ELF/Thunks.cpp
900

Forgot to remove the commented line?

lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
13–16

Perhaps add --noinhibit-exec case, since the logic was changed to support it?

MaskRay updated this revision to Diff 291250.Fri, Sep 11, 9:31 AM
MaskRay marked 2 inline comments as done.

Address comments

lld/ELF/Relocations.cpp
116

getDefinedLocation(sym) is a static function. Defining the helper in Thunks.cpp would replicate getDefinedLocation(sym)

I think it is fine to define the overloads in the same place to keep the style consistent.

grimar accepted this revision.Mon, Sep 14, 1:35 AM

LGTM

This revision is now accepted and ready to land.Mon, Sep 14, 1:35 AM
stefanp added inline comments.Mon, Sep 14, 8:59 AM
lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
16

Question:
What does this do? There is no FileCheck after it and you are sending the output to /dev/null so what output file is retained?

MaskRay added inline comments.Mon, Sep 14, 9:08 AM
lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
16

--noinhibit-exec makes errorOrWarn report a warning, so the output is not inhibited and lld can still return 0.

This revision was landed with ongoing or failed builds.Mon, Sep 14, 9:57 AM
This revision was automatically updated to reflect the committed changes.