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

Jump to bottom

cmd/link: serialize Reloc.Variant field to Go object files #14218

Closed
minux opened this issue Feb 4, 2016 · 11 comments
Closed

cmd/link: serialize Reloc.Variant field to Go object files #14218

minux opened this issue Feb 4, 2016 · 11 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone
Unplanned

Comments

@minux
Copy link
Member

minux commented Feb 4, 2016

In RISC architectures, address relocations usually need to
change a pair of instructions, and sometimes the fields to
stuff the constant in are different depending on the instruction.

For example, Power load/store instructions have two forms,
D form and DS-form.

Currently, we have two separate relocations in cmd/internal/obj:
R_ADDRPOWER and R_ADDRPOWER_DS, which really
are the same except how to stuff the const into instruction.

We already have a Reloc.Variant field to handle a similar concept:
slight variants on the same conceptual relocation type, but it's
only used by the linker to implement internal linking (to represent
ELF relocations).

The proposal is to serialize the Variant field into object file, so
that the compiler can also use the variant mechanism.

The benefit is that: the variants are defined in arch-specific
packages, and generic code handle the relocations defined
in cmd/internal/obj don't need to know about all the arch
specific variants of a relocation as the details are irrelevant
unless you are dealing with instruction encoding.
@minux minux added the Proposal label Feb 4, 2016
@mwhudson
Copy link
Contributor

mwhudson commented Feb 4, 2016

I think I finally understand how this is supposed to work. I'm not entirely convinced it will lead to cleaner code but I'm happy to wait and see on that!

I wonder if we'll be able to only have R_TLS_LE and R_TLS_IE relocs?

@gopherbot
Copy link

CL https://golang.org/cl/20941 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 22, 2016
@mundaym @bradfitz
cmd/internal/obj: add support for s390x
2a7e85f 
Adds a new R_PCRELDBL relocation for 2-byte aligned relative
relocations on s390x. Should be removed once #14218 is
implemented.

Change-Id: I79dd2d8e746ba8cbc26c570faccfdd691e8161e8
Reviewed-on: https://go-review.googlesource.com/20941
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@mwhudson
Copy link
Contributor

Given that we seem to be changing the object file fairly freely this cycle, maybe we should just do this?

@bradfitz bradfitz modified the milestone: Unplanned Apr 7, 2016
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unplanned Jul 19, 2016
@gopherbot
Copy link

CL https://golang.org/cl/27932 mentions this issue.

@mundaym
Copy link
Member

mundaym commented Aug 28, 2016

I've sent a basic CL that does this: https://go-review.googlesource.com/#/c/27932/

It increases object file size by about 1.6%. I think it would be possible to reduce the object sizes by reducing the sizes of the types we are using for RelocType & RelocVariant from int32 to int16. As it currently stands I suspect it's not a valuable enough change to warrant the size increase.

I'm not sure what other changes are planned that this could be submitted with/integrated into to minimize disruption from the object file format change.

@mwhudson
Copy link
Contributor

On 29 August 2016 at 03:20, Michael Munday notifications@github.com wrote:

I've sent a basic CL that does this: https://go-review.
googlesource.com/#/c/27932/

Personally I don't really have a problem with lots of reloc types vs a
reloc type x variant matrix but I can see this is somewhat cleaner.

You should update cmd/internal/goobj though.

It increases object file size by about 1.6%. I think it would be possible
to reduce the object sizes by reducing the sizes of the types we are using
for RelocType & RelocVariant from int32 to int16. As it currently stands
I suspect it's not a valuable enough change to warrant the size increase.

I don't think 1.6% in the object file format is much of a concern. Object
files in 1.7 are ~20% smaller than 1.6 and noone seemed too excited about
that :-)

I'm not sure what other changes are planned that this could be submitted
with/integrated into to minimize disruption from the object file format
change.

I think the experience of the 1.7 cycle was also that changing the object
file format is not as disruptive as one might think.

Cheers,
mwh

@adg
Copy link
Contributor

adg commented Oct 3, 2016

Should this be a proposal? What's the current state?

@mundaym
Copy link
Member

mundaym commented Oct 3, 2016

I have a CL that needs some updates to improve its efficiency and demonstrate its value better. I'll try and allocate it some time this week.

The change is very minor and would be fully reversible in future versions (Go makes no guarantees about object file format that I know of), so I don't know if it counts as a 'proposal'.

@adg adg modified the milestones: Go1.9Early, Proposal Oct 31, 2016
@bradfitz bradfitz changed the title proposal: serialize Reloc.Variant field to Go object files cmd/link: serialize Reloc.Variant field to Go object files May 3, 2017
@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.9Early May 3, 2017
@bradfitz
Copy link
Contributor

bradfitz commented May 3, 2017

@mundaym, I'm punting this to Go 1.10. Is that correct?

@mundaym
Copy link
Member

mundaym commented May 3, 2017

SGTM

@bradfitz bradfitz added early-in-cycle A change that should be done early in the 3 month dev cycle. and removed early-in-cycle A change that should be done early in the 3 month dev cycle. labels Jun 14, 2017
@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.10 Jun 14, 2017
@rsc rsc removed this from the Go1.10 milestone Nov 22, 2017
@rsc rsc added this to the Go1.11 milestone Nov 22, 2017
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 23, 2018
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 23, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Dec 7, 2018
@rsc rsc unassigned mundaym Jun 23, 2022
@cherrymui
Copy link
Member

The linker and the Go object file have gone through a major redesign. The discussion here is pretty much obsolete. It might still be helpful to serialize Reloc Variant, but I'm not sure adding a field that is used just for a small number of relocations is a good idea. Closing. If one still thinks adding Reloc Variant to the object file is helpful, feel free the send a CL and we can discuss form there. Thanks.

@cherrymui cherrymui closed this as not planned Won't fix, can't repro, duplicate, stale Mar 2, 2023
@golang golang locked and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
9 participants
@bradfitz @rsc @minux @mwhudson @ianlancetaylor @adg @gopherbot @cherrymui @mundaym