Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify R_<CLS>_RELATIVE optimization wording #242

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

smeenai
Copy link
Contributor

@smeenai smeenai commented Jan 31, 2024

The optimization is applied by the static linker, so we should reword
the sentence from that perspective. Thanks to @smithp35 for suggesting
the wording.

@MaskRay
Copy link
Contributor

MaskRay commented Jan 31, 2024

I think "resolves to the current dynamic shared object" conveys the same meaning: rtld binds the symbol reference to the current DSO.

@smeenai
Copy link
Contributor Author

smeenai commented Jan 31, 2024

Hmm. I was thinking about this from the perspective of the static linker, since it'll be the one emitting RELATIVE vs. GLOB_DAT as an optimization (the context of the sentence). I guess a better wording for the entire sentence from that perspective would be:

This relocation represents an optimization; it can be used to replace R_<CLS>_GLOB_DAT when the symbol resolves to is defined in the current dynamic shared object and is not pre-emptable.

You're right from the rtld perspective, but I don't understand the optimization discussion from that perspective. The static linker is the one deciding what relocation type to emit, and rtld is just acting on it, right?

Copy link
Contributor

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for patch.

I've made an alternative suggestion as I think the text should be more specific about static linking and less specific about the current shared object.

@@ -1738,7 +1738,7 @@ The need for copy relocations can be avoided if a compiler generates all code re

- Because the initial value of the place is not related to the ultimate target of a ``R_<CLS>_JUMP_SLOT`` relocation the addend ``A`` of such a REL-type relocation shall be zero rather than the initial content of the place. A platform ABI shall prescribe whether or not the ``r_addend`` field of such a RELA-type relocation is honored. (There may be security-related reasons not to do so).

``R_<CLS>_RELATIVE`` represents a relative adjustment to the place based on the load address of the object relative to its original link address. All symbols defined in the same segment will have the same relative adjustment. If ``S`` is the null symbol (ELF symbol index 0) then the adjustment is based on the segment defining the place. On systems where all segments are mapped contiguously the adjustment will be the same for each reloction, thus adjustment never needs to resolve the symbol. This relocation represents an optimization; it can be used to replace ``R_<CLS>_GLOB_DAT`` when the symbol resolves to the current dynamic shared object.
``R_<CLS>_RELATIVE`` represents a relative adjustment to the place based on the load address of the object relative to its original link address. All symbols defined in the same segment will have the same relative adjustment. If ``S`` is the null symbol (ELF symbol index 0) then the adjustment is based on the segment defining the place. On systems where all segments are mapped contiguously the adjustment will be the same for each reloction, thus adjustment never needs to resolve the symbol. This relocation represents an optimization; it can be used to replace ``R_<CLS>_GLOB_DAT`` when the symbol resolves to the current dynamic shared object and is not pre-emptable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I interpreted resolves to the current dynamic shared object as not including the cases when the symbol could be pre-empted. However it maybe one of those cases where knowing the answer makes it easier to make that interpretation.

I do think the current wording could do with some clarification, particularly as RELATIVE relocations are used in position independent executables.

May I suggest the alternative wording:

This relocation represents an optimization; a static linker can replace ``R_<CLS>_GLOB_DAT`` when the symbol is known at static link time to always resolve to the current link unit.

I've not explicitly mentioned pre-emptable but I think the always resolve catches that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That alternative wording sounds great to me; I'd maybe say "a static linker can use it to replace" or "a static linker can replace R_<CLS>_GLOB_DAT with it when" just to be explicit, but it's exactly what I was going for. Did you want to create a separate PR/commit for that or just edit this one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be easiest for me if you can choose one of your alternatives (use it to) or (with it) and update this PR, I'm happy with either variation! The ABI unfortunately uses merge and rebase rather than merge and squash, so to avoid a load of fixup commits we do a squash before merging. With such a simple change you may as well just force-push a replacement.

The company prefers for us not to edit PRs, so it is either update this one or submit a new PR which seems like more work than is necessary.

The optimization is applied by the static linker, so we should reword
the sentence from that perspective. Thanks to @smithp35 for suggesting
the wording.
@smeenai smeenai changed the title Clarify R_<CLS>_RELATIVE wording Clarify R_<CLS>_RELATIVE optimization wording Jan 31, 2024
@smeenai
Copy link
Contributor Author

smeenai commented Jan 31, 2024

Thanks, I updated the wording with your suggestion. I also updated the PR title and description to match the commit, because I wasn't sure which one would be ultimately used.

Copy link
Contributor

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update LGTM.

I'll leave it open for a week before merging to give any of my colleagues a chance to comment.

@smeenai
Copy link
Contributor Author

smeenai commented Feb 15, 2024

Can we merge this now? :)

@stuij
Copy link
Member

stuij commented Feb 19, 2024

Yes :) As per @smithp35's previous comments.

@stuij stuij merged commit c87dc3a into ARM-software:main Feb 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants