golabels: adjust offset extraction #890

florianl · 2025-10-24T11:05:58Z

Adapt the extraction for relative offsets from RIP.

This is the case that will be supported with this PR:

0000000005f06ca0 <runtime.stackcheck.abi0@@Base>:
 5f06ca0:	48 8b 0d 01 13 1b 10 	mov    0x101b1301(%rip),%rcx        # 160b7fa8 <runtime.tlsg@@Base+0x160b7fa8>
 5f06ca7:	64 48 8b 01          	mov    %fs:(%rcx),%rax
 5f06cab:	48 39 60 08          	cmp    %rsp,0x8(%rax)
 5f06caf:	77 05                	ja     5f06cb6 <runtime.stackcheck.abi0@@Base+0x16>
 5f06cb1:	e8 ca ff ff ff       	call   5f06c80 <runtime.abort.abi0@@Base>
 5f06cb6:	48 3b 20             	cmp    (%rax),%rsp
 5f06cb9:	77 05                	ja     5f06cc0 <runtime.stackcheck.abi0@@Base+0x20>
 5f06cbb:	e8 c0 ff ff ff       	call   5f06c80 <runtime.abort.abi0@@Base>
 5f06cc0:	c3                   	ret

Adapt the extraction for relative offsets from RIP. Signed-off-by: Florian Lehner <[email protected]>

interpreter/golabels/tls_amd64.go

Co-authored-by: Christos Kalkanis <[email protected]>

interpreter/golabels/tls_amd64.go

fabled

Could you add a unit test for the new sequence?

interpreter/golabels/tls_amd64.go

Signed-off-by: Florian Lehner <[email protected]>

florianl · 2025-10-27T10:11:27Z

Could you add a unit test for the new sequence?

So far we don't know the reason that causes this relative offset from RIP. I can name a bunch of Go executables (each multiple MB in size) that have this relative offset. But adding them to the repo as test feels wrong.
And the use of f.VirtualMemory() makes it hard to construct a mocked test. An option could be to split extractTLSGOffset() into several parts, like (a) identify gopclntab, (b) walk code and (c) introduce a function to read the memory, so that f.VirtualMemory() can be mocked for tests. Should we got this route @fabled ?

Signed-off-by: Florian Lehner <[email protected]>

florianl · 2025-10-28T08:58:30Z

@fabled I did add a unit test with abc27cc as described in #890 (comment).

interpreter/golabels/tls_amd64.go

Signed-off-by: Florian Lehner <[email protected]>

fabled · 2025-11-13T07:40:18Z

interpreter/golabels/tls_amd64.go

 )

+// readMemory is a helper that can be mocked for testing.
+type readMemory func(addr int64, sz, maxSize int) ([]byte, error)


I think this is not needed. io.ReadAt interface could be used instead. This would allow passing the pfelf.File in the main code path directly (it's ReadAt works on virtual memory). And then just ship a mock object on testing side implementing the inteface. That would avoid generating closures.

fabled · 2025-11-13T07:41:11Z

interpreter/golabels/tls_amd64.go

 		if err != nil {
 			break
 		}
+		pc += op.Len


The it asm.amd.Interpreter object tracks already pc. Could you instead add PC accessor to it to read it, and use it? I think adding it there would be useful in future.

fabled · 2025-11-13T07:44:10Z

interpreter/golabels/tls_amd64.go

+		// RIP-relative load: mov disp(%rip), %reg
+		if mem, ok := op.Args[1].(x86asm.Mem); ok && mem.Base == x86asm.RIP {
+			curAddr := int64(sym.Address) + int64(pc)
+			if dst, ok := op.Args[0].(x86asm.Reg); ok {
+				target := curAddr + int64(mem.Disp)
+
+				// Read 8-byte TLS value from target address
+				b, err := extract(target, 8, 8)
+				if err == nil && len(b) >= 8 {
+					ripRelLoads[dst] = int64(npsr.Uint64(b, 0))
+				}
+			}


I feel that this tracking should be done in the amd.Interpreter, there should a be a new expression type for RIP relative value. That would generalize the code greatly.

If this feels too complicated at this time, I can do it as a follow up PR.

sorry for the delay 🙏 I created the follow up with #984.

fabled · 2025-11-13T07:45:27Z

interpreter/golabels/tls_amd64.go

+				// Read 8-byte TLS value from target address
+				b, err := extract(target, 8, 8)
+				if err == nil && len(b) >= 8 {
+					ripRelLoads[dst] = int64(npsr.Uint64(b, 0))
+				}


I think the rip relative address should be stored to the map, not the dereferenced value. This might do unneeded extra reads. Only the actual rip relative value that is needed, should trigger the dereference. And again, the rip relative PC ideally comes from Interpreter.

fabled · 2025-11-13T07:46:19Z

interpreter/golabels/tls_amd64_test.go

+	if err != nil {
+		t.Fatal(err)
+	}


nit: use testify

Suggested change

if err != nil {

t.Fatal(err)

}

require.NoError(t, err)

fabled · 2025-11-13T07:47:03Z

interpreter/golabels/tls_amd64_test.go

+	if offset != 0 {
+		t.Fatalf("Expected offset '0' got '%d'", offset)
+	}


nit: use testify

Suggested change

if offset != 0 {

t.Fatalf("Expected offset '0' got '%d'", offset)

}

require.Equal(t, 0, offset)

golabels: adjust offset extraction

952ecd5

Adapt the extraction for relative offsets from RIP. Signed-off-by: Florian Lehner <[email protected]>

florianl marked this pull request as ready for review October 24, 2025 13:16

florianl requested review from a team as code owners October 24, 2025 13:16

christos68k reviewed Oct 27, 2025

View reviewed changes

interpreter/golabels/tls_amd64.go Outdated Show resolved Hide resolved

interpreter/golabels/tls_amd64.go Outdated Show resolved Hide resolved

interpreter/golabels/tls_amd64.go Show resolved Hide resolved

Update interpreter/golabels/tls_amd64.go

602b7b5

Co-authored-by: Christos Kalkanis <[email protected]>

christos68k reviewed Oct 27, 2025

View reviewed changes

interpreter/golabels/tls_amd64.go Outdated Show resolved Hide resolved

fabled reviewed Oct 27, 2025

View reviewed changes

interpreter/golabels/tls_amd64.go Outdated Show resolved Hide resolved

florianl added 2 commits October 27, 2025 10:46

use npsr

caf5b9f

Signed-off-by: Florian Lehner <[email protected]>

move MOV instruction check

4903d61

Signed-off-by: Florian Lehner <[email protected]>

florianl added 4 commits October 27, 2025 11:15

return for unhandled mov fs case

c7d0916

Signed-off-by: Florian Lehner <[email protected]>

Merge branch 'main' into golabels-gcflags

695f6b2

fix escape

b823396

Signed-off-by: Florian Lehner <[email protected]>

add unit test

abc27cc

Signed-off-by: Florian Lehner <[email protected]>

christos68k reviewed Oct 29, 2025

View reviewed changes

interpreter/golabels/tls_amd64.go Outdated Show resolved Hide resolved

florianl added 2 commits November 12, 2025 14:00

Merge branch 'main' into golabels-gcflags

03e427a

fix op.Len twice

0b8d298

Signed-off-by: Florian Lehner <[email protected]>

florianl requested review from christos68k and fabled November 12, 2025 13:20

fabled reviewed Nov 13, 2025

View reviewed changes

christos68k approved these changes Nov 13, 2025

View reviewed changes

Merge branch 'main' into golabels-gcflags

51914fc

florianl mentioned this pull request Nov 25, 2025

amd: handle RIP relative offset #984

Open

golabels: adjust offset extraction #890

Are you sure you want to change the base?

golabels: adjust offset extraction #890

Uh oh!

Conversation

florianl commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fabled left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

florianl commented Oct 27, 2025

Uh oh!

florianl commented Oct 28, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

florianl commented Oct 24, 2025 •

edited

Loading

fabled left a comment •

edited

Loading