Page MenuHomePhabricator

[ELF][Hexagon] Replace R_HEXAGON_GOT with R_GOTPLT
ClosedPublic

Authored by MaskRay on Aug 14 2019, 9:21 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Aug 14 2019, 9:21 PM
sidneym marked an inline comment as done.Aug 15 2019, 9:23 AM
sidneym added inline comments.
ELF/InputSection.cpp
676 ↗(On Diff #215318)

Removing this will produce unexpected output. GOT is the address in the GOT (G). GOTREL is the difference between the location and table (S+A - GOT)

sidneym added inline comments.Aug 15 2019, 3:28 PM
ELF/InputSection.cpp
676 ↗(On Diff #215318)

Ah, looking at the code more carefully I think this ok. I was reading this as GOTPLTREL not GOTPLT. Let me try to look back and see why this wasn't done in the first place.

sidneym added inline comments.Aug 15 2019, 3:40 PM
ELF/InputSection.cpp
676 ↗(On Diff #215318)

R_HEXAGON_GOT was added before R_GOTPLT. I think it's ok to use R_GOTPLT.

sidneym accepted this revision.Aug 16 2019, 9:35 AM
This revision is now accepted and ready to land.Aug 16 2019, 9:35 AM
This revision was automatically updated to reflect the committed changes.
ruiu added a comment.Aug 19 2019, 12:03 AM

Nice! Thank you for doing this.

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

ruiu added a comment.Aug 19 2019, 12:58 AM

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

It shouldn't be too hard to extend oneOf so that the function uses two words instead of one word as a bitmap, but yeah, we should first try to reduce the number of relocation types if possible.

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

It shouldn't be too hard to extend oneOf so that the function uses two words instead of one word as a bitmap, but yeah, we should first try to reduce the number of relocation types if possible.

I had to do this for the CHERI fork of LLD since we added a few new entries. We currently always use a 128-bit mask (https://github.com/CTSRD-CHERI/llvm-project/blob/master/lld/ELF/Relocations.cpp#L139) but I guess we could reorder the enum to have a fast path for commonly checked values and do the second compare only for less common entries. I can submit a patch if you would like to use the 128-bit variant.

ruiu added a comment.Aug 19 2019, 2:38 AM

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

It shouldn't be too hard to extend oneOf so that the function uses two words instead of one word as a bitmap, but yeah, we should first try to reduce the number of relocation types if possible.

I had to do this for the CHERI fork of LLD since we added a few new entries. We currently always use a 128-bit mask (https://github.com/CTSRD-CHERI/llvm-project/blob/master/lld/ELF/Relocations.cpp#L139) but I guess we could reorder the enum to have a fast path for commonly checked values and do the second compare only for less common entries. I can submit a patch if you would like to use the 128-bit variant.

Nice. We probably need your code in a near future. I'm not too worried to always use a 128-bit mask -- I don't think that's a large value, and we can optimize it after it raises in a profiling.

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

At the moment no. I hope to add TLS relocations shortly and I don't think they will require new target-specific enums.