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

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

Open
minux opened this Issue Feb 4, 2016 · 10 comments

Comments

Projects
None yet
8 participants
@minux
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

This comment has been minimized.

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

This comment has been minimized.

gopherbot commented Mar 21, 2016

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

gopherbot pushed a commit that referenced this issue Mar 22, 2016

cmd/internal/obj: add support for s390x
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

This comment has been minimized.

Contributor

mwhudson commented Mar 31, 2016

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

This comment has been minimized.

gopherbot commented Aug 28, 2016

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

@mundaym

This comment has been minimized.

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

This comment has been minimized.

Contributor

mwhudson commented Aug 29, 2016

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

This comment has been minimized.

Contributor

adg commented Oct 3, 2016

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

@mundaym

This comment has been minimized.

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 from proposal: serialize Reloc.Variant field to Go object files to 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

This comment has been minimized.

Member

bradfitz commented May 3, 2017

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

@mundaym

This comment has been minimized.

Member

mundaym commented May 3, 2017

SGTM

@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 modified the milestones: Go1.12, Unplanned Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment