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

liblink, cmd/ld: don't encode the instruction being relocated in Reloc.add #10050

Open
minux opened this issue Mar 2, 2015 · 5 comments

Comments

@minux
Copy link
Member

commented Mar 2, 2015

When I first implemented cgo for ARM, I made the wrong precedence to encode
the instruction itself in the relocation's addend field. It makes everything harder:

liblink must stuff the real addend into the instruction, put it in reloc.add, and then
in the linker, it must get the real addend out from the instruction, calculate the
real value, and stuff the value back into the instruction.

Additionally, it makes -S output unreadable.

rel 20+4 t=6 runtime.morestack_noctxt+9bfffffe
rel 40+4 t=6 runtime.duffzero+eb00005e

(Note: the 2nd line is calling into the middle of duffzero, but you can't tell
anything from the addend without some shifts and masks to figure out the
offset)
Compare this to the 6g -S output:

rel 16+4 t=4 runtime.morestack_noctxt+0
rel 37+4 t=4 runtime.duffzero+228

I suspect this is because in cmd/ld/lib.c, we always pass a zero val (the variable o)
to archreloc, instead, it really should pass the original value read from the symbol.

Should we correct this? so that archreloc is passed the real instrutions/data being
relocated in *val argument, and we place the real addend in Reloc.add field?

@minux minux added the repo-main label Mar 2, 2015

@minux minux self-assigned this Mar 2, 2015

@minux minux added this to the Go1.5Maybe milestone Mar 2, 2015

@mwhudson

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2015

I don't know how seriously my input should be taken here, but this whole game confused the heck out of me when I did arm64 external linking. Passing the original value to archreloc would make a lot more sense.

@minux

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2015

@rsc rsc removed the repo-main label Apr 14, 2015

@josharian

This comment has been minimized.

Copy link
Contributor

commented May 28, 2015

FWIW, this threw me for a while today.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2015

Too late for Go 1.5.

@rsc rsc modified the milestones: Unplanned, Go1.5Maybe Jun 29, 2015

mwhudson added a commit to mwhudson/go that referenced this issue Aug 3, 2015
cmd/link: pass value being relocated to archreloc
And clean up the mess on arm64 (the mess on arm is too confusing).

See issue golang#10050

Change-Id: I2ce813fe8646d4e818eb660612a7e4b2bb04de4c
mwhudson added a commit to mwhudson/go that referenced this issue Aug 3, 2015
cmd/link: pass value being relocated to archreloc
And clean up the mess on arm64 (the mess on arm is too confusing).

See issue golang#10050

Change-Id: I2ce813fe8646d4e818eb660612a7e4b2bb04de4c
@gopherbot

This comment has been minimized.

Copy link

commented Aug 25, 2015

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

mwhudson added a commit to mwhudson/go that referenced this issue Aug 27, 2015
cmd/link: pass value being relocated to archreloc
And clean up the mess on arm64 (the mess on arm is too confusing).

See issue golang#10050

Change-Id: I2ce813fe8646d4e818eb660612a7e4b2bb04de4c
ianlancetaylor added a commit that referenced this issue Aug 31, 2015
cmd/link: pass value being relocated to archreloc
And clean up the mess on arm64 (the mess on arm is too confusing).

See issue #10050

Change-Id: I2ce813fe8646d4e818eb660612a7e4b2bb04de4c
Reviewed-on: https://go-review.googlesource.com/13884
Reviewed-by: Ian Lance Taylor <iant@golang.org>
mwhudson added a commit to mwhudson/go that referenced this issue Sep 2, 2015
cmd/link: pass value being relocated to archreloc
And clean up the mess on arm64 (the mess on arm is too confusing).

See issue golang#10050

Change-Id: I2ce813fe8646d4e818eb660612a7e4b2bb04de4c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.