Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
lld/ELF/Arch/PPC64.cpp | ||
---|---|---|
1054 | misaligned | |
lld/ELF/Target.h | ||
216 | Make it imperative, i.e. // Write a prefixed instruction, which is a 4-byte prefix followed by a 4-byte instruction (regardless of endianness). "As a result, we need to shift the pieces around on little endian machines." is not needed. | |
lld/test/ELF/ppc64-pcrel-call-to-toc.s | ||
19 | ## | |
23 | -NEXT: |
lld/ELF/Thunks.cpp | ||
---|---|---|
283 | minor nit: // When the caller does not use TOC and does not preserve R2 makes a local call to a callee that requires a TOC pointer ... --> // When a caller that does not maintain a toc-pointer performs a local call to a callee which requires a toc-pointer ... | |
852 | fatal error if the offset is to large to encode in the instruction. | |
856 | Comment is wrong. destination.getVA() - getThunkTargetSym()->getVA() is the pc-relative offset to the function (func@pcrel), not the offset to the functions plt entry. | |
862 | Good name. We could shorten to '__gep_setup' as I think anyone looking at a PPC64 binary is likely to understand gep is short for global entry point. |
lld/ELF/Thunks.cpp | ||
---|---|---|
856 | Sorry, I missed that the comment also says the instruction is a pld which would be correct if we are loading the symbols address from the plt, but this thunk type is for functions not in the plt. Do you intend to load the address of the function out of a table? or do you meant to calculate the address relative to the program counter? |
Thanks Sean and MaskRay for the review!
- Check if the offset computed in PPC64R12SetupStub::writeTo fits in 34 bits and report fatal error if it is too large.
- Use __gep_setup in the name for r12 set up stub and update test case accordingly.
- Address nits in the comments.
lld/ELF/Thunks.cpp | ||
---|---|---|
856 | No worries. Right, should load the symbol address from the plt instead so that we can use pld to load the callee's global entry point to r12. Will update it accordingly. |
Seems fine to me, but of course, wait to hear from Sean/MaskRay.
lld/ELF/Thunks.cpp | ||
---|---|---|
855 | Would anyone object to naming these in an enum? enum PPCInstrMasks : uint64_t { PLD_NO_DISP = 0x04100000E5800000, MTCTR_R12 = 0x7D8903A6, ... }; in llvm/include/llvm/Object/ELF.h? | |
864 | I do not object to the name, but if there is precedent in the GNU linker, we should match it for consistency and familiarity. |
- Added a fix to use paddi instead of pld to put the callee global entry point to r12
- Added enum for PPCInstrMasks in llvm/include/llvm/Object/ELF.h e.g. PADDI_R12_NO_DISP and MTCTR_R12
- Updated the offset computation for r12 set up stub and test case.
lld/ELF/Thunks.cpp | ||
---|---|---|
855 | Thanks for the advice. enum added in ELF.h | |
864 | GNU linker ld uses long_branch.callee as the name, which also used for r2 save stub as mentioned in https://reviews.llvm.org/D82950 . I would go with __gep_steup_ as it represents the thunk type. |
I only had a couple of comments.
Overall I think this looks good.
lld/ELF/Arch/PPC64.cpp | ||
---|---|---|
113 | nit: Why was this moved up and the comment deleted? | |
lld/ELF/Thunks.cpp | ||
873 | I feel like there may be an easier way to do this without all of the shifts: uint64_t paddi = PADDI_R12_NO_DISP | (((offset >> 16) & 0x3ffff) << 32) | (offset & 0xffff); writePrefixedInstruction(buf + 0, paddi); // paddi r12, 0, func@pcrel, 1 That way you won't have to break the instruction apart and then put it back together. |
Thanks Stefan.
- Modified paddi hex computation with less shifts.
- Fixed nits for clang-format.
lld/test/ELF/ppc64-pcrel-call-to-toc.s | ||
---|---|---|
3 | Do you mind adopting https://reviews.llvm.org/D83834#change-qrppZgZj6Chf ? |
I only had one question.
Otherwise, LGTM.
However, please wait for approval from sfertile and/or MaskRay before committing.
lld/ELF/Arch/PPC64.cpp | ||
---|---|---|
113 | Ok, see that the comment was moved. |
lld/ELF/Arch/PPC64.cpp | ||
---|---|---|
113 | I moved it up to group the functions declared in lld/ELF/Target.h (following declaration order) Also separated from the static functions below. |
lld/test/ELF/ppc64-pcrel-call-to-toc.s | ||
---|---|---|
3 | Thanks MaskRay for the input! The new style looks better. Howvever, seems your patch has not landed it yet. : ) |
nit: Why was this moved up and the comment deleted?
I assume it was something that the rebase did...