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.

Sign up for GitHub

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/internal/obj/arm: should not put instruction encoding into Reloc.Add #19811

Open
aarzilli opened this issue Mar 31, 2017 · 7 comments
Open

cmd/internal/obj/arm: should not put instruction encoding into Reloc.Add #19811

aarzilli opened this issue Mar 31, 2017 · 7 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@aarzilli
Copy link
Contributor

aarzilli commented Mar 31, 2017 •
edited

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

What operating system and processor architecture are you using (go env)?

Whatever the trybots for linux-arm are using

What did you do?

Built a sample file with -N -l then attempted to load it with cmd/internal/goobj:

https://go-review.googlesource.com/#/c/39230/

What did you expect to see?

honest object files.

What did you see instead?

https://storage.googleapis.com/go-build-log/e81c4c0b/linux-arm_e2ed47ce.log
@cherrymui
Copy link
Member

  1. It has nothing to do with -N -l. I can reproduce it without the flags.

  2. The error of "corrupt object file" happens when decoding an Reloc's Add field, where it expects a value that fits in an int (32-bit on ARM) whereas the ARM backend sometimes puts values that doesn't fit into a 32-bit int, like 0xebfffffe. If you read the same object file (compiled for ARM) on a 64-bit machine, it has no error.

There are actually two issues:

  1. cmd/internal/goobj's readInt probably should allow int64, as cmd/internal/obj's writeInt can write any int64 value, without enforcing it to fit in 32-bit. There is a TODO at https://go.googlesource.com/go/+/78f6622/src/cmd/internal/goobj/read.go#387 (cc @rsc).

  2. In the cases where the ARM backend writes values like 0xebfffffe to Reloc's Add field, it is actually the instruction's encoding, instead of an addend of a relocation. Arguably it is a misuse of the Add field. We probably want to fix this.

@cherrymui cherrymui added this to the Go1.9 milestone Apr 1, 2017
@cherrymui cherrymui changed the title cmd/compile: corrupt object file on linux-arm with -N -l cmd/internal/goobj, cmd/internal/obj/arm: ARM object file cannot be parsed by cmd/internal/goobj Apr 1, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Jun 7, 2017

@cherrymui, do you want to fix this for Go 1.9? If not, feel free to kick it to Go 1.10.

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 7, 2017
@cherrymui
Copy link
Member

I'll try the goobj part.

I tried the assembler part. It is a bit invasive, involving change in both the assembler (writing the object file) and the linker (reading the object file). This part will not be in Go 1.9.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jul 6, 2017
@0intro 0intro mentioned this issue Sep 10, 2017
Closed
@gopherbot
Copy link

Change https://golang.org/cl/69010 mentions this issue: cmd/internal/goobj: accept int64 in readInt

gopherbot pushed a commit that referenced this issue Nov 1, 2017
@cherrymui
cmd/internal/goobj: accept int64 in readInt
7bd0b61 
The counter part, writeInt in cmd/internal/obj, writes int64s.
So the reader side should also read int64s. This may cause a
larger range of values being accepted, some of which should
not be that large. This is probably ok: for example, for
size/index/length, the very large value (due to corruption)
may be well past the end and causes other errors. And we did
not do much bound check anyway.

One exmaple where this matters is ARM32's object file. For one
type of relocation it encodes the instruction into Reloc.Add
field (which itself may be problematic and worth fix) and the
instruction encoding overflows int32, causing ARM32 object
file being rejected by goobj (and so objdump and nm) before.

Unskip ARM32 object file tests in goobj, nm, and objdump.

Updates #19811.

Change-Id: Ia46c2b68df5f1c5204d6509ceab6416ad6372315
Reviewed-on: https://go-review.googlesource.com/69010
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@rsc
Copy link
Contributor

rsc commented Nov 22, 2017

If there's any work left here, it will need to wait for Go 1.11.

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@cherrymui
Copy link
Member

The read of ARM object file is resolved by the CL above.
What is remaining is that it may be good for the ARM backend not to put the instruction encoding into Reloc.Add.

@cherrymui cherrymui changed the title cmd/internal/goobj, cmd/internal/obj/arm: ARM object file cannot be parsed by cmd/internal/goobj cmd/internal/obj/arm: should not put instruction encoding into Reloc.Add Nov 22, 2017
@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018
@cherrymui cherrymui mentioned this issue Aug 13, 2020
Closed
@gopherbot
Copy link

Change https://golang.org/cl/248399 mentions this issue: cmd/link: emit correct jump instruction on ARM for DYNIMPORT

gopherbot pushed a commit that referenced this issue Aug 17, 2020
@cherrymui
cmd/link: emit correct jump instruction on ARM for DYNIMPORT
a2a2237 
On ARM, for a JMP/CALL relocation, the instruction bytes is
encoded in Reloc.Add (issue #19811). I really hate it, but before
it is fixed we have to follow the rule and emit the right bits
from r.Add.

Fixes #40769.

Change-Id: I862e105408d344c5cc58ca9140d2e552e4364453
Reviewed-on: https://go-review.googlesource.com/c/go/+/248399
Reviewed-by: Jeremy Faller <jeremy@golang.org>
Reviewed-by: Joel Sing <joel@sing.id.au>
Reviewed-by: Than McIntosh <thanm@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@bradfitz @rsc @aarzilli @gopherbot @cherrymui