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/asm: arm64 assembler generates code that is not execute-only compatible #59615

Open
4a6f656c opened this issue Apr 13, 2023 · 10 comments
Open

cmd/asm: arm64 assembler generates code that is not execute-only compatible #59615

4a6f656c opened this issue Apr 13, 2023 · 10 comments
Labels
arch-arm64 compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@4a6f656c
Copy link
Contributor

The Go assembler for arm64 generates code that is not execute-only compatible, since it stores constants in the text section rather than in rodata, which it then reads during execution.

OpenBSD 7.3 has enabled xonly by default on OpenBSD/arm64 - this means that externally linked Go binaries segfault on execution. This can be worked around in the interim by disabling execute-only when the external linker is invoked.

Longer term, the assembler should store constants in rodata and/or load them via instructions, such that the text section can be marked as execute-only.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 13, 2023
@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 13, 2023
@dr2chase
Copy link
Contributor

@golang/compiler

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/484555 mentions this issue: cmd/link/internal/ld: disable execute-only for external linking on openbsd/arm64

@randall77
Copy link
Contributor

I know of one place where we read instruction memory, to print instruction bytes after a SIGILL:

https://cs.opensource.google/go/go/+/refs/tags/go1.20.3:src/runtime/signal_unix.go;l=747

Not super critical, but if we're running in execute-only mode it would be good to disable that code.

@erifan
Copy link

erifan commented Apr 14, 2023

since it stores constants in the text section rather than in rodata, which it then reads during execution.

Do you mean literal pool ?

@4a6f656c
Copy link
Contributor Author

since it stores constants in the text section rather than in rodata, which it then reads during execution.

Do you mean literal pool ?

Yes.

gopherbot pushed a commit that referenced this issue Apr 14, 2023
@4a6f656c
cmd/link/internal/ld: disable execute-only for external linking on op…
dd53a43 
…enbsd/arm64

The Go arm64 assembler places constants into the text section of a binary.
OpenBSD 7.3 enabled xonly by default on OpenBSD/arm64. This means that any
externally linked Go binary now segfaults. Disable execute-only when invoking
the external linker on openbsd/arm64, in order to work around this issue.

Updates #59615

Change-Id: I1a291293da3c6e4409b21873d066ea15e9bfe280
Reviewed-on: https://go-review.googlesource.com/c/go/+/484555
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Aaron Bieber <deftly@gmail.com>
Run-TryBot: Joel Sing <joel@sing.id.au>
Reviewed-by: Than McIntosh <thanm@google.com>
@mknyszek mknyszek added this to the Backlog milestone Apr 19, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/512538 mentions this issue: cmd/internal/obj/arm64: improve classification of loads and stores

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/512540 mentions this issue: cmd/internal/obj/arm64: improve splitting of 24 bit unsigned scaled immediates

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/512539 mentions this issue: cmd/internal/obj/arm64: avoid unnecessary literal pool usage for moves

gopherbot pushed a commit that referenced this issue Jul 31, 2023
@4a6f656c
cmd/internal/obj/arm64: improve classification of loads and stores
3313b39 
Currently, pool literals are added when they are not needed, namely
in the case where the offset is a 24 bit unsigned scaled immediate.
By improving the classification of loads and stores, we can avoid
generating unused pool literals. However, more importantly this
provides a basis for further improvement of the load and store
code generation.

Updates #59615

Change-Id: Ia3bad1709314565a05894a76c434cca2fa4533c4
Reviewed-on: https://go-review.googlesource.com/c/go/+/512538
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Joel Sing <joel@sing.id.au>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Jul 31, 2023
@4a6f656c
cmd/internal/obj/arm64: avoid unnecessary literal pool usage for moves
208fc13 
In a number of load and store cases, the use of the literal pool can be
entirely avoided by simply adding or subtracting the offset from the
register. This uses the same number of instructions, while avoiding a
load from memory, along with the need for the value to be in the literal
pool. Overall this reduces the size of binaries slightly and should have
lower overhead.

Updates #59615

Change-Id: I9cb6a403dc71e34a46af913f5db87dbf52f8688c
Reviewed-on: https://go-review.googlesource.com/c/go/+/512539
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Joel Sing <joel@sing.id.au>
gopherbot pushed a commit that referenced this issue Jul 31, 2023
@4a6f656c
cmd/internal/obj/arm64: improve splitting of 24 bit unsigned scaled i…
6eaad82 
…mmediates

The previous implementation would limit itself to 0xfff000 | 0xfff << shift,
while the maximum possible value is 0xfff000 + 0xfff << shift. In practical
terms, this means that an additional ((1 << shift) - 1) * 0x1000 of offset
is reachable for operations that use this splitting format. In the case of
an 8 byte load/store, this is an additional 0x7000 that can be reached
without needing to use the literal pool.

Updates #59615

Change-Id: Ice7023104042d31c115eafb9398c2b999bdd6583
Reviewed-on: https://go-review.googlesource.com/c/go/+/512540
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Joel Sing <joel@sing.id.au>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/515617 mentions this issue: cmd/internal/obj/arm64: load large constants into vector registers from rodata

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/515615 mentions this issue: cmd/internal/obj/arm64: avoid unnecessary pool literal usage for load/store pairs

gopherbot pushed a commit that referenced this issue Aug 25, 2023
@4a6f656c
cmd/internal/obj/arm64: load large constants into vector registers fr…
0a17b2c 
…om rodata

Load large constants into vector registers from rodata, instead of placing them
in the literal pool. This treats VMOVQ/VMOVD/VMOVS the same as FMOVD/FMOVS and
makes use of the existing mechanism for storing values in rodata. Two additional
instructions are required for a load, however these instructions are used
infrequently and already have a high latency.

Updates #59615

Change-Id: I54226730267689963d73321e548733ae2d66740e
Reviewed-on: https://go-review.googlesource.com/c/go/+/515617
Reviewed-by: Eric Fang <eric.fang@arm.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Run-TryBot: Joel Sing <joel@sing.id.au>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Aug 28, 2023
@4a6f656c
cmd/internal/obj/arm64: avoid unnecessary pool literal usage for load…
19e2e3c 
…/store pairs

Implement better classification for load and store pair operations. This in
turn allows us to avoid using pool literals when the offset fits in a 24 bit
unsigned immediate. In this case, the offset can be calculated using two
add immediate instructions, rather than loading the offset from the pool
literal and then adding the offset to the base register. This requires the
same number of instructions, however avoids a load from memory and does
not require the offset to be stored in the literal pool.

Updates #59615

Change-Id: I316ec3d54f1d06ae9d930e98d0c32471775fcb26
Reviewed-on: https://go-review.googlesource.com/c/go/+/515615
Run-TryBot: Joel Sing <joel@sing.id.au>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Joedian Reid <joedian@golang.org>
Reviewed-by: Cherry Mui <cherryyz@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
arch-arm64 compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

No branches or pull requests
6 participants
@mknyszek @dr2chase @randall77 @4a6f656c @gopherbot @erifan