Page MenuHomePhabricator

[PPC64] Sort .toc sections accessed with small code model relocs close to the .got part 2
ClosedPublic

Authored by sfertile on Jan 25 2019, 9:21 AM.

Details

Summary

Follow on patch which addresses the other small code model relocation types. For lld we should ignore the small code model relocations that represent offsets to entries in the .got for sorting purpose. Since these relocations don't access the .toc sections they should have no bearing on how we sort the .toc sections, and since we don't sort got entries (they are allocated on demand in order) we don't need to track which objects contain these relocation types.

Diff Detail

Repository
rL LLVM

Event Timeline

sfertile created this revision.Jan 25 2019, 9:21 AM
ruiu added inline comments.Jan 28 2019, 3:09 PM
ELF/Arch/PPC64.cpp
101 ↗(On Diff #183555)

Could you update the comment a bit so that it gives the big picture about what this function does? Looks like this comment is slightly too detailed and might not be understood if you already know what this is for.

sfertile updated this revision to Diff 184330.Jan 30 2019, 10:41 AM

Updated /moved a few comments.

sfertile marked an inline comment as done.Jan 30 2019, 10:50 AM
sfertile added inline comments.
ELF/Arch/PPC64.cpp
101 ↗(On Diff #183555)

The previous comment was a bit out of place here. It had a lot of PPC specific details so I thought I should bury it in the PPC target. I've removed the specific relocations and moved the comment into scanRelocs. I think it makes sense there now as it explains why we only need to flag certain small code model relocations for the sorting (as opposed to what bfd and gold do).

MaskRay accepted this revision.Feb 6 2019, 3:12 AM
MaskRay added inline comments.
ELF/Relocations.cpp
1005 ↗(On Diff #184330)

The description of GOT is verbose.. and I don't get why it is not necessary for small code model sorting.

(Glad to know that we don't need a ton of GOT relocations used in binuitls-gdb: R_PPC64_GOT_TLSLD16 R_PPC64_GOT_TPREL16_DS R_PPC64_GOT16 ...)

Than change LG.

This revision is now accepted and ready to land.Feb 6 2019, 3:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 3:12 AM
sfertile added inline comments.Feb 6 2019, 12:00 PM
ELF/Relocations.cpp
1005 ↗(On Diff #184330)

Ok, let me try to explain it in a different way. let me know if this is better:

We can separate the small code model relocations into 2 categories:
1) Those that access the compiler generated .toc  sections.
2) Those that access the linker allocated got entries.
lld allocates got entries to symbols on demand. Since we don't try to sort the got entries in any way we don't have to track which objects have got-based small code model relocs. The .toc sections get placed after the end of the linker allocated .got section and we do sort those so sections addressed with small code model relocations come first.

I don't know what ld.bfd and gold do, but from the snippet you posted on the previous review I would guess they do sort the .got entries. I think llds current behavior is reasonable though. Since the compilers emit .toc entries for most/all of the indirection the size of the actual linker allocated portion of the .got tend to be quite small. The majority of the .gots size comes from the merged in .toc sections which we do sort and layout to maximize addressability. Having a .got thats large enough to overflow a small code model relocation on its own is pretty unlikely.

If any of the compilers switched to use the got-indirections described in the ABI rather then using the .toc section for indirections then I think we should reconsider sorting the .got entries based on small code model addressability.

ruiu accepted this revision.Feb 6 2019, 2:17 PM

LGTM

MaskRay added inline comments.Feb 6 2019, 6:52 PM
ELF/Relocations.cpp
1005 ↗(On Diff #184330)

Thanks for the explanation. I've read some documentation and agree that got-indirect relocation types have a less chance to get overflow. Sorting GOT entries does not change the computed values of R_PPC64_TOC16 R_PPC64_TOC16_DS. Handling just R_PPC64_TOC16 and R_PPC64_TOC16_DS should provide a good coverage.

// libgcc.a
__fixunssfti:

0:       00 00 4c 3c     addis 2, 12, 0
         0000000000000000:  R_PPC64_REL16_HA     .TOC.
4:       00 00 42 38     addi 2, 2, 0
         0000000000000004:  R_PPC64_REL16_LO     .TOC.+4
8:       00 00 c2 e8     ld 6, 0(2)
         0000000000000008:  R_PPC64_TOC16_DS     .toc       ######### this can go overflow
This revision was automatically updated to reflect the committed changes.