This is an archive of the discontinued LLVM Phabricator instance.

Fixed PPC32 local symbol relocation
ClosedPublic

Authored by vit9696 on Oct 24 2017, 3:47 AM.

Details

Summary

This fixes compatibility with llvm after r273499 and r273595, when LLVM started to emit PLT relocations even in static mode causing a regression.

Diff Detail

Event Timeline

vit9696 created this revision.Oct 24 2017, 3:47 AM
hfinkel edited edge metadata.Oct 24 2017, 8:03 AM

If we're going to specifically comment on LLVM's behavior here, I think it is worth mentioning that GNU ld does the same thing. The comments should explain what the linker is doing, and why (e.g., support directly resolving PLT-relative relocations when statically linking).

Also, this will need a test case.

vit9696 updated this revision to Diff 120099.Oct 24 2017, 10:49 AM

Thanks, updated the patch.

ruiu added inline comments.Oct 24 2017, 11:07 AM
ELF/Arch/PPC.cpp
36

Does this say that this is not exactly what we are supposed to do to relocations of this type? If so, please state that first and what we should actually do (what type should actually be returned from this function?).

vit9696 updated this revision to Diff 120104.Oct 24 2017, 11:15 AM

@ruiu, alright, rewrote it more explicitly.

vit9696 marked an inline comment as done.Oct 24 2017, 11:15 AM
ruiu edited edge metadata.Oct 24 2017, 11:18 AM

Thanks! The new comment is much much better. With that, I can perfectly understand the situation.

Hal, are you okay with this change? I'll let you sign off.

hfinkel accepted this revision.Oct 26 2017, 7:21 PM
In D39226#905510, @ruiu wrote:

Thanks! The new comment is much much better. With that, I can perfectly understand the situation.

Hal, are you okay with this change? I'll let you sign off.

Yep. LGTM.

This revision is now accepted and ready to land.Oct 26 2017, 7:21 PM

Sorry, why are you pinging the review? It is approved so you can commit it. Is the issue that you don't have commit access and need someone to commit it? If so, you should state that rather than just pinging the patch.

Yes, I actually meant to ask someone to commit this for me. Given the timespan I thought it was evident :)

vit9696 closed this revision.Dec 10 2017, 9:00 AM

This got merged, thx to tnorthover.