TLSDESC Relax Relocation Clarification #475
Conversation
These relaxations are designed so the sequence is not necessarily adjacent. In this case, each of the relocations needs to be marked as relaxable, as each instruction is separately modified/removed. This also removes some spurious `(symbol)` annotations on `R_RISCV_RELAX` - the relaxation relocation does not point to the same symbol as the underlying relocation.
kito-cheng
left a comment
kito-cheng
left a comment
There was a problem hiding this comment.
Oh...I found binutils already did that way, and I think adding R_RISCV_RELAX for each is the right way to do.
kito-cheng
left a comment
kito-cheng
left a comment
There was a problem hiding this comment.
Could you split the part of dropping symbol from R_RISCV_RELAX to a separate PR, I believe this part could be fast approved since it's apparently document fix, and for the TLSDESC part I would like moving forward after discussion within psABI meeting (although I think this should be no controversial)
|
Split out the document fix into #476 |
|
There's no need for additional RELAX relocations. I object to such a change. The R_RISCV_TLSDESC_HI20, R_RISCV_TLSDESC_LOAD_LO12, R_RISCV_TLSDESC_ADD_LO12, and R_RISCV_TLSDESC_CALL relocations form a compact, finite state machine, where each relocation follows the previous one, potentially interspersed with unrelated relocations. A new R_RISCV_TLSDESC_HI20 can only start after an R_RISCV_TLSDESC_CALL has been processed. The optional R_RISCV_RELAX following R_RISCV_TLSDESC_HI20 determines the relaxability of the entire sequence. It's undefined if a non-empty proper subset of instructions within the code sequence (but not the entire sequence) are relaxed. |
|
I'm not in favour of having a completely different relaxation scheme for one set of relocations. All other relaxations in the ABI have all relocations marked with relax. I wouldn't mind a rule that says if one of the four has a relax relocation, they all must, but I don't like the "look at something else to find out if this is relaxable". |
|
For TLS relocations it’s indeed a bit special, unlike other reloc relaxations, they’ll still get relaxed even if linker relaxation is turned off. But I still prefer tagging any instructions that might be changed or removed with |
kito-cheng
left a comment
kito-cheng
left a comment
There was a problem hiding this comment.
Give my explicit LGTM here
|
Oh, I did some digging and found that there was originally an R_RISCV_RELAX, but it was eventually removed with the agreement of the original TLSDESC proposers (@ishitatsuyuki and @rui314), and I also agreed with the proposal at that time... |
|
Ok, I see. We need to write down that this works differently, rather than having it implicit in an example. I will update this PR. |
lenary
changed the title
3 participants
Unlike all other relaxation sequences, TLSDESC relaxations only require a R_RISCV_RELAX on their first relocation. Clarify this fact in the text.