[go] internal/bytealg: add MTE-safe IndexByte implementation for ARM64 iOS
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems with your PR:
1. You have a long 147 character line in the commit message body. Please add line breaks to long lines that should be wrapped. Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. (Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)
2. You usually need to reference a bug number for all but trivial or cosmetic fixes. For this repo, the format is usually 'Fixes #12345' or 'Updates #12345' at the end of the commit message. Should you have a bug reference?
Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
internal/bytealg: add MTE-safe IndexByte implementation for ARM64 iOS
Add optimized ARM64 implementation of IndexByte and IndexByteString for iOS that never reads before string start or beyond end, making it MTE-safe.
Also extend the findnull fallback to iOS since it
may use ARM64 with restrictions similar to Plan 9.
Includes comprehensive tests for different SIMD code paths and alignments.
This PR will be imported into Gerrit with the title and first
comment (this text) used to generate the subject and body of
the Gerrit change.
Change diff
diff --git a/src/bytes/bytes_test.go b/src/bytes/bytes_test.go
index 891aef2..c5b0051 100644
--- a/src/bytes/bytes_test.go
+++ b/src/bytes/bytes_test.go
@@ -435,6 +435,93 @@
}
}
+// TestIndexByteSIMDPaths tests edge cases for SIMD implementations of IndexByte,
+// specifically targeting the different code paths: unaligned prefix, 32-byte chunks,
+// 16-byte chunks, and scalar tail handling. This is particularly relevant for
+// ARM64 MTE-safe implementations.
+func TestIndexByteSIMDPaths(t *testing.T) {
+ // Test various sizes that exercise different SIMD code paths
+ sizes := []int{
+ 0, 1, 2, 7, 8, 15, 16, 17, 31, 32, 33, 47, 48, 49,
+ 63, 64, 65, 95, 96, 97, 127, 128, 129, 255, 256, 257,
+ }
+
+ for _, size := range sizes {
+ b := make([]byte, size)
+ // Fill with non-target byte
+ for i := range b {
+ b[i] = 'a'
+ }
+
+ // Test 1: Target byte not present
+ if got := IndexByte(b, 'x'); got != -1 {
+ t.Errorf("IndexByte(size=%d, no match) = %d; want -1", size, got)
+ }
+
+ // Test 2: Target byte at each position
+ for pos := 0; pos < size; pos++ {
+ b[pos] = 'x'
+ if got := IndexByte(b, 'x'); got != pos {
+ t.Errorf("IndexByte(size=%d, pos=%d) = %d; want %d", size, pos, got, pos)
+ }
+ b[pos] = 'a'
+ }
+ }
+}
+
+// TestIndexByteUnaligned tests IndexByte with slices starting at various
+// alignments. This tests the unaligned prefix handling in SIMD implementations.
+func TestIndexByteUnaligned(t *testing.T) {
+ // Create a large buffer and test slices at different offsets
+ const bufSize = 512
+ buf := make([]byte, bufSize)
+ for i := range buf {
+ buf[i] = 'a'
+ }
+
+ // Test with different starting offsets (0 to 63 to cover 32-byte alignment cases)
+ for offset := 0; offset < 64 && offset < bufSize; offset++ {
+ // Test with different lengths from this offset
+ for length := 1; length <= bufSize-offset && length <= 256; length++ {
+ slice := buf[offset : offset+length]
+
+ // Reset slice
+ for i := range slice {
+ slice[i] = 'a'
+ }
+
+ // Test: no match
+ if got := IndexByte(slice, 'x'); got != -1 {
+ t.Errorf("IndexByte(offset=%d, len=%d, no match) = %d; want -1", offset, length, got)
+ }
+
+ // Test: match at first position
+ slice[0] = 'x'
+ if got := IndexByte(slice, 'x'); got != 0 {
+ t.Errorf("IndexByte(offset=%d, len=%d, pos=0) = %d; want 0", offset, length, got)
+ }
+ slice[0] = 'a'
+
+ // Test: match at last position
+ slice[length-1] = 'x'
+ if got := IndexByte(slice, 'x'); got != length-1 {
+ t.Errorf("IndexByte(offset=%d, len=%d, pos=%d) = %d; want %d", offset, length, length-1, got, length-1)
+ }
+ slice[length-1] = 'a'
+
+ // Test: match in middle (if length > 2)
+ if length > 2 {
+ mid := length / 2
+ slice[mid] = 'x'
+ if got := IndexByte(slice, 'x'); got != mid {
+ t.Errorf("IndexByte(offset=%d, len=%d, pos=%d) = %d; want %d", offset, length, mid, got, mid)
+ }
+ slice[mid] = 'a'
+ }
+ }
+ }
+}
+
func TestIndexRune(t *testing.T) {
tests := []struct {
in string
diff --git a/src/internal/bytealg/indexbyte_arm64.s b/src/internal/bytealg/indexbyte_arm64.s
index 1101122ec..b35ca3a 100644
--- a/src/internal/bytealg/indexbyte_arm64.s
+++ b/src/internal/bytealg/indexbyte_arm64.s
@@ -4,6 +4,152 @@
#include "textflag.h"
+#ifdef GOOS_ios
+// func IndexByte(b []byte, c byte) int
+// input:
+// R0: b ptr
+// R1: b len
+// R2: b cap (unused)
+// R3: c byte to search
+// return
+// R0: result
+TEXT ·IndexByte<ABIInternal>(SB),NOSPLIT,$0-40
+ MOVD R3, R2
+ B ·IndexByteString<ABIInternal>(SB)
+
+// func IndexByteString(s string, c byte) int
+// input:
+// R0: s ptr
+// R1: s len
+// R2: c byte to search
+// return
+// R0: result
+TEXT ·IndexByteString<ABIInternal>(SB),NOSPLIT,$0-32
+ // MTE-safe implementation: never reads before start or beyond end.
+ // For each 32-byte chunk we calculate a 64-bit syndrome value,
+ // with two bits per byte. Bit 0 is set if the relevant byte
+ // matched the requested character. Counting trailing zeros
+ // identifies which byte matched.
+
+ CBZ R1, fail
+ MOVD R0, R11 // Save original pointer
+ ADD R0, R1, R4 // R4 = end pointer
+
+ // Setup SIMD constants
+ // Magic constant 0x40100401 allows us to identify which lane matches.
+ // 0x40100401 = ((1<<0) + (4<<8) + (16<<16) + (64<<24))
+ VMOV R2, V0.B16 // Broadcast search byte to all lanes
+ MOVD $0x40100401, R5
+ VMOV R5, V5.S4
+
+ // Check alignment - process unaligned prefix byte-by-byte
+ // This avoids reading before the string start (MTE violation)
+ ANDS $0x1f, R0, R9
+ BEQ aligned_start
+
+unaligned_loop:
+ MOVBU (R0), R5
+ CMP R2, R5
+ BEQ found_scalar
+ ADD $1, R0
+ CMP R0, R4
+ BEQ fail
+ TST $0x1f, R0
+ BNE unaligned_loop
+
+aligned_start:
+ // Check if we have at least 32 bytes remaining BEFORE loading
+ SUB R0, R4, R5 // R5 = remaining bytes
+ CMP $32, R5
+ BLO tail16_check
+
+loop32:
+ // Load 32 aligned bytes - safe because we verified 32+ bytes remain
+ VLD1.P 32(R0), [V1.B16, V2.B16]
+
+ // Compare both vectors against search byte
+ VCMEQ V0.B16, V1.B16, V3.B16
+ VCMEQ V0.B16, V2.B16, V4.B16
+
+ // Quick check: any matches in either vector?
+ VORR V4.B16, V3.B16, V6.B16
+ VADDP V6.D2, V6.D2, V6.D2
+ VMOV V6.D[0], R6
+ CBNZ R6, found32
+
+ // Check bounds BEFORE next iteration
+ SUB R0, R4, R5
+ CMP $32, R5
+ BHS loop32
+
+tail16_check:
+ // Try 16-byte chunks if available
+ CMP $16, R5
+ BLO tail_scalar
+
+loop16:
+ // Load 16 aligned bytes
+ VLD1.P 16(R0), [V1.B16]
+
+ VCMEQ V0.B16, V1.B16, V3.B16
+ VAND V5.B16, V3.B16, V3.B16
+ VADDP V3.B16, V3.B16, V3.B16 // 128->64
+ VADDP V3.B16, V3.B16, V3.B16 // 64->32
+ VMOV V3.S[0], R6
+ CBNZ R6, found16
+
+ // Check bounds BEFORE next iteration
+ SUB R0, R4, R5
+ CMP $16, R5
+ BHS loop16
+
+tail_scalar:
+ // Handle remaining bytes one at a time (always safe)
+ CMP R0, R4
+ BEQ fail
+
+tail_loop:
+ MOVBU (R0), R5
+ CMP R2, R5
+ BEQ found_scalar
+ ADD $1, R0
+ CMP R0, R4
+ BNE tail_loop
+ B fail
+
+found32:
+ // Calculate full syndrome for precise position
+ VAND V5.B16, V3.B16, V3.B16
+ VAND V5.B16, V4.B16, V4.B16
+ VADDP V4.B16, V3.B16, V6.B16 // 256->128
+ VADDP V6.B16, V6.B16, V6.B16 // 128->64
+ VMOV V6.D[0], R6
+
+ RBIT R6, R6
+ SUB $32, R0, R0 // Compensate post-increment
+ CLZ R6, R6
+ ADD R6>>1, R0, R0 // R6 is twice the byte offset
+ SUB R11, R0, R0
+ RET
+
+found16:
+ RBIT R6, R6
+ SUB $16, R0, R0 // Compensate post-increment
+ CLZ R6, R6
+ ADD R6>>1, R0, R0 // R6 is twice the byte offset
+ SUB R11, R0, R0
+ RET
+
+found_scalar:
+ SUB R11, R0, R0
+ RET
+
+fail:
+ MOVD $-1, R0
+ RET
+
+#else
+
// func IndexByte(b []byte, c byte) int
// input:
// R0: b ptr
@@ -143,3 +289,5 @@
fail:
MOVD $-1, R0
RET
+
+#endif
diff --git a/src/runtime/export_test.go b/src/runtime/export_test.go
index bc471e5..477b7ff 100644
--- a/src/runtime/export_test.go
+++ b/src/runtime/export_test.go
@@ -228,6 +228,15 @@
return
}
+// Findnull is an entry point for testing findnull.
+// It returns the index of the first NUL byte in a NUL-terminated byte slice.
+func Findnull(s []byte) int {
+ if len(s) == 0 {
+ return 0
+ }
+ return findnull(&s[0])
+}
+
var Open = open
var Close = closefd
var Read = read
diff --git a/src/runtime/string.go b/src/runtime/string.go
index 7207f83..8c01082 100644
--- a/src/runtime/string.go
+++ b/src/runtime/string.go
@@ -493,7 +493,7 @@
// Avoid IndexByteString on Plan 9 because it uses SSE instructions
// on x86 machines, and those are classified as floating point instructions,
// which are illegal in a note handler.
- if GOOS == "plan9" {
+ if GOOS == "plan9" || GOOS == "ios" {
p := (*[maxAlloc/2 - 1]byte)(unsafe.Pointer(s))
l := 0
for p[l] != 0 {
diff --git a/src/runtime/string_test.go b/src/runtime/string_test.go
index 942e7ac..9de8931 100644
--- a/src/runtime/string_test.go
+++ b/src/runtime/string_test.go
@@ -214,6 +214,57 @@
}
}
+// TestFindnull tests the findnull function which finds the index of the
+// first NUL byte in a NUL-terminated byte slice. This tests the byte-by-byte
+// fallback path used on Plan 9 and iOS
+func TestFindnull(t *testing.T) {
+ // Test various sizes that might exercise different code paths
+ sizes := []int{
+ 0, 1, 2, 7, 8, 15, 16, 17, 31, 32, 33, 47, 48, 49,
+ 63, 64, 65, 127, 128, 129, 255, 256, 257, 1023, 1024, 1025,
+ }
+
+ for _, size := range sizes {
+ // Create a NUL-terminated byte slice
+ b := make([]byte, size+1) // +1 for NUL terminator
+ for i := 0; i < size; i++ {
+ b[i] = 'a'
+ }
+ b[size] = 0
+
+ got := runtime.Findnull(b)
+ if got != size {
+ t.Errorf("Findnull(size=%d) = %d; want %d", size, got, size)
+ }
+ }
+
+ // Test with NUL at various positions
+ for _, total := range []int{64, 128, 256} {
+ for pos := 0; pos < total; pos++ {
+ b := make([]byte, total+1)
+ for i := 0; i < total; i++ {
+ b[i] = 'a'
+ }
+ b[pos] = 0 // Place NUL at position pos
+ b[total] = 0
+
+ got := runtime.Findnull(b)
+ if got != pos {
+ t.Errorf("Findnull(total=%d, nul_at=%d) = %d; want %d", total, pos, got, pos)
+ }
+
+ // Restore for next iteration
+ b[pos] = 'a'
+ }
+ }
+
+ // Test empty slice (nil pointer behavior)
+ got := runtime.Findnull(nil)
+ if got != 0 {
+ t.Errorf("Findnull(nil) = %d; want 0", got)
+ }
+}
+
func TestLargeStringConcat(t *testing.T) {
output := runTestProg(t, "testprog", "stringconcat")
want := "panic: " + strings.Repeat("0", 1<<10) + strings.Repeat("1", 1<<10) +
diff --git a/src/strings/strings_test.go b/src/strings/strings_test.go
index edfeb0e..62fe1ec 100644
--- a/src/strings/strings_test.go
+++ b/src/strings/strings_test.go
@@ -266,6 +266,100 @@
}
}
+// TestIndexByteSIMDPaths tests edge cases for SIMD implementations of IndexByte,
+// specifically targeting the different code paths: unaligned prefix, 32-byte chunks,
+// 16-byte chunks, and scalar tail handling. This is particularly relevant for
+// ARM64 MTE-safe implementations.
+func TestIndexByteSIMDPaths(t *testing.T) {
+ // Test various sizes that exercise different SIMD code paths
+ sizes := []int{
+ 0, 1, 2, 7, 8, 15, 16, 17, 31, 32, 33, 47, 48, 49,
+ 63, 64, 65, 95, 96, 97, 127, 128, 129, 255, 256, 257,
+ }
+
+ for _, size := range sizes {
+ // Create a string of the given size
+ b := make([]byte, size)
+ for i := range b {
+ b[i] = 'a'
+ }
+ s := string(b)
+
+ // Test 1: Target byte not present
+ if got := IndexByte(s, 'x'); got != -1 {
+ t.Errorf("IndexByte(size=%d, no match) = %d; want -1", size, got)
+ }
+
+ // Test 2: Target byte at each position
+ for pos := 0; pos < size; pos++ {
+ b[pos] = 'x'
+ s = string(b)
+ if got := IndexByte(s, 'x'); got != pos {
+ t.Errorf("IndexByte(size=%d, pos=%d) = %d; want %d", size, pos, got, pos)
+ }
+ b[pos] = 'a'
+ }
+ }
+}
+
+// TestIndexByteStringUnaligned tests IndexByte with strings created from
+// slices at various alignments. This tests the unaligned prefix handling
+// in SIMD implementations.
+func TestIndexByteStringUnaligned(t *testing.T) {
+ // Create a large buffer and test strings at different offsets
+ const bufSize = 512
+ buf := make([]byte, bufSize)
+ for i := range buf {
+ buf[i] = 'a'
+ }
+
+ // Test with different starting offsets (0 to 63 to cover 32-byte alignment cases)
+ for offset := 0; offset < 64 && offset < bufSize; offset++ {
+ // Test with different lengths from this offset
+ for length := 1; length <= bufSize-offset && length <= 256; length++ {
+ slice := buf[offset : offset+length]
+
+ // Reset slice
+ for i := range slice {
+ slice[i] = 'a'
+ }
+ s := string(slice)
+
+ // Test: no match
+ if got := IndexByte(s, 'x'); got != -1 {
+ t.Errorf("IndexByte(offset=%d, len=%d, no match) = %d; want -1", offset, length, got)
+ }
+
+ // Test: match at first position
+ slice[0] = 'x'
+ s = string(slice)
+ if got := IndexByte(s, 'x'); got != 0 {
+ t.Errorf("IndexByte(offset=%d, len=%d, pos=0) = %d; want 0", offset, length, got)
+ }
+ slice[0] = 'a'
+
+ // Test: match at last position
+ slice[length-1] = 'x'
+ s = string(slice)
+ if got := IndexByte(s, 'x'); got != length-1 {
+ t.Errorf("IndexByte(offset=%d, len=%d, pos=%d) = %d; want %d", offset, length, length-1, got, length-1)
+ }
+ slice[length-1] = 'a'
+
+ // Test: match in middle (if length > 2)
+ if length > 2 {
+ mid := length / 2
+ slice[mid] = 'x'
+ s = string(slice)
+ if got := IndexByte(s, 'x'); got != mid {
+ t.Errorf("IndexByte(offset=%d, len=%d, pos=%d) = %d; want %d", offset, length, mid, got, mid)
+ }
+ slice[mid] = 'a'
+ }
+ }
+ }
+}
+
func simpleIndex(s, sep string) int {
n := len(sep)
for i := n; i <= len(s); i++ {
Change information
- M src/bytes/bytes_test.go
- M src/internal/bytealg/indexbyte_arm64.s
- M src/runtime/export_test.go
- M src/runtime/string.go
- M src/runtime/string_test.go
- M src/strings/strings_test.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Message from Gopher Robot![]( "Open in Gerrit")
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Ian Lance Taylor
- Keith Randall
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
I think I fixed the immediate problem of triggering MTE with CL 751020.
Let me know if that fixes it for you.
The non-assembly parts of this CL are probably still needed (or at least, should be considered). The iOS conditioning, and the set of tests (thanks for those!).
Related details
- Ian Lance Taylor
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marco (Gerrit)
Marco added 1 comment![]( "Open in Gerrit")
I spotted some possible problems with your PR:
1. You have a long 147 character line in the commit message body. Please add line breaks to long lines that should be wrapped. Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. (Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)
2. You usually need to reference a bug number for all but trivial or cosmetic fixes. For this repo, the format is usually 'Fixes #12345' or 'Updates #12345' at the end of the commit message. Should you have a bug reference?Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Done
Marco (Gerrit)
Marco added 1 comment![]( "Open in Gerrit")
I think I fixed the immediate problem of triggering MTE with CL 751020.
Let me know if that fixes it for you.The non-assembly parts of this CL are probably still needed (or at least, should be considered). The iOS conditioning, and the set of tests (thanks for those!).
For me it was necessary to update the assembly code section in order to make the application run. With only the go code, it started but then closes on splash-screen when MTE was enabled. After the updated assembly section, no more crashes were seen
Marco (Gerrit)
Marco added 1 comment![]( "Open in Gerrit")
MarcoI think I fixed the immediate problem of triggering MTE with CL 751020.
Let me know if that fixes it for you.The non-assembly parts of this CL are probably still needed (or at least, should be considered). The iOS conditioning, and the set of tests (thanks for those!).
For me it was necessary to update the assembly code section in order to make the application run. With only the go code, it started but then closes on splash-screen when MTE was enabled. After the updated assembly section, no more crashes were seen
- Ian Lance Taylor
- Keith Randall
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
MarcoI think I fixed the immediate problem of triggering MTE with CL 751020.
Let me know if that fixes it for you.The non-assembly parts of this CL are probably still needed (or at least, should be considered). The iOS conditioning, and the set of tests (thanks for those!).
MarcoFor me it was necessary to update the assembly code section in order to make the application run. With only the go code, it started but then closes on splash-screen when MTE was enabled. After the updated assembly section, no more crashes were seen
Done
Where does it crash? Do you have a stack trace, or even just a PC where the application was aborted?
Did you add iOS to the ifdefs? The MTE compatibility code that is checked in is just enabled on android currently.
Related details
- Ian Lance Taylor
- Marco
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marco (Gerrit)
Marco added 1 comment![]( "Open in Gerrit")
MarcoI think I fixed the immediate problem of triggering MTE with CL 751020.
Let me know if that fixes it for you.The non-assembly parts of this CL are probably still needed (or at least, should be considered). The iOS conditioning, and the set of tests (thanks for those!).
MarcoFor me it was necessary to update the assembly code section in order to make the application run. With only the go code, it started but then closes on splash-screen when MTE was enabled. After the updated assembly section, no more crashes were seen
Keith RandallDone
Where does it crash? Do you have a stack trace, or even just a PC where the application was aborted?
Did you add iOS to the ifdefs? The MTE compatibility code that is checked in is just enabled on android currently.
Here goes the stack trace, obtained before my assembly section proposal
internal/bytealg.IndexByteString (indexbyte_arm64.s:54)
runtime.findnull (string.go:520)
Related details
- Ian Lance Taylor
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
MarcoI think I fixed the immediate problem of triggering MTE with CL 751020.
Let me know if that fixes it for you.The non-assembly parts of this CL are probably still needed (or at least, should be considered). The iOS conditioning, and the set of tests (thanks for those!).
MarcoFor me it was necessary to update the assembly code section in order to make the application run. With only the go code, it started but then closes on splash-screen when MTE was enabled. After the updated assembly section, no more crashes were seen
Keith RandallDone
MarcoWhere does it crash? Do you have a stack trace, or even just a PC where the application was aborted?
Did you add iOS to the ifdefs? The MTE compatibility code that is checked in is just enabled on android currently.
Here goes the stack trace, obtained before my assembly section proposal
internal/bytealg.IndexByteString (indexbyte_arm64.s:54)
runtime.findnull (string.go:520)
Hm, there's no instruction at that line in the assembly.
Just to be clear do:
1. sync to tip
2. Add `|| GOOS_ios` to the three ifdefs in indexbyte_arm64.s
3. Change the line at runtime/string.go:511 to
```
const pageSize = 4096*(1-(goos.IsAndroid | goos.IsIos)) + 16*(goos.IsAndroid | goos.IsIos)
```
Does it crash then, and if so, where?
Marco (Gerrit)
Marco added 1 comment![]( "Open in Gerrit")
Good morning,
Regarding the fact that there are no instructions on that line of the assembly file, comments are stripped from the final build file, so that line does in fact contain instructions.
Changing the pagesize value for iOS to 16 (at runtime/string.go:511) has no effect because in this PR proposal the previous if statement has been modified to include iOS
(if GOOS == ‘plan9’ || GOOS == ‘ios’)
so the if statement on line 511 is not reached.
In any case, as reported, WITHOUT modifying the assembly section for iOS, where a conservative and MTE-safe implementation is made that always checks the limits before loading data (first processes unaligned bytes, then 32/16 byte chunks, and finally single bytes), the iOS application crashes when run on iPhone 17 with MTE enabled, at line 54 of the indexbyte_arm64 file, as previously mentioned.
Therefore, I request a review of the content of this PR to make GO available for use in an iOS context with MTE enabled on iPhone 17 devices, which involves changes to both the assembly file and the src/runtime/string.go file.
Thank you.
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
Regarding the fact that there are no instructions on that line of the assembly file, comments are stripped from the final build file, so that line does in fact contain instructions.
What is stripping comments? The Go build system doesn't do that.
In any case, I need to know which instruction in asm_arm64.s is triggering MTE. Is it the 54th non-comment line? Does that skip empty lines also?
A disassemble listing pointing to the triggering instruction would also work.
Changing the pagesize value for iOS to 16 (at runtime/string.go:511) has no effect because in this PR proposal the previous if statement has been modified to include iOS
(if GOOS == ‘plan9’ || GOOS == ‘ios’)
> so the if statement on line 511 is not reached.
I mean, do this experiment *without this CL*:
Just to be clear do:
1. sync to tip
2. Add || GOOS_ios to the three ifdefs in indexbyte_arm64.s
3. Change the line at runtime/string.go:511 to
```
const pageSize = 4096*(1-(goos.IsAndroid | goos.IsIos)) + 16*(goos.IsAndroid | goos.IsIos)
```
Does it crash then, and if so, where?
Marco (Gerrit)
Marco added 1 comment![]( "Open in Gerrit")
Hi @k...@golang.org,
I have done the tests you asked me.
First of all, I have tried the exact same test you suggested me (sync - GOOS_ios on assembly file - updating line 511 of string go, all without my CL), and the result is that the iOS application crashes at splash-screen at this instruction:
0x10482978c <+44>: ld1.16b { v1 }, [x3], #16
of internal/bytealg.IndexByteString
for this cause: "Thread 3: Heap buffer underflow by 16 bytes", when MTE is enabled on iPhone 17.
Note aside: I have also did the exact same test, but this time removing all comments from bytealg/indexbyte_arm64.s.
The result is exactly the same as the one above, 0x10482978c <+44>: ld1.16b { v1 }, [x3], #16 is the instruction that causes the crash.
So apparently comments are not taken into account on how to determinate which line is involved in the crash that is occurring, at least for golang dependencies made for final iOS applications.
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
0x10482978c <+44>: ld1.16b { v1 }, [x3], #16
That instruction is not at tip. That looks like the one that was fixed by https://go-review.googlesource.com/c/go/+/751020