fix(python): fix stub decoding routine by korniltsev · Pull Request #412 · open-telemetry/opentelemetry-ebpf-profiler

korniltsev · 2025-03-20T07:17:49Z

This PR changes amd64 python stub decoding routine.

registers state is tracked
adds support for add instruction
adds support for register mov
returns the last target register state value at the termination instructions (call/jmp)
removes argno as it is always 0

This fixes the failure to resolve autoTLSKey
with my self compiled cpython 312 , 313 as well as /usr/lib/google-cloud-sdk/platform/bundledpythonunix/bin/python3.11 from google/cloud-sdk:502.0.0 docker image.

This is a draft to initiate discussion. More testdata will be added.

gnurizen · 2025-03-20T11:45:30Z

-uint64_t decode_stub_argument(const uint8_t* code, size_t codesz, uint8_t argument_no,
-    uint64_t rip_base, uint64_t memory_base) {
+// todo update comment
+// todo rewrite in go


For what its worth we've gotten pretty good mileage out of go's builtin disassembler: https://github.com/parca-dev/opentelemetry-ebpf-profiler/blob/main/interpreter/luajit/extractor_x86.go

Its shortcomings don't really manifest for simple cases.

The long history is that we needed Zydis originally for the stack delta extraction filtering, and the go disassembler did not recognize all opcodes. We wanted to keep only single disassembler so Zydis was used for all other purposes as well. I think the go built in disassembler has improved since, and the stub disassembly is more constrained, this could probably an option. Especially since we use the go builtin disassembler for all arm64 side.

The go x86 disassembler is definitely still broken for anything meaty but I found for simple tasks like this it works well. Personally I find avoiding cgo when possible is worth it and if I was gonna undertake any new decoder tasks I'd probably take a hard look at https://github.com/zyantific/zydis-go.

I've drafted a go version.

It's a bit slower but I think it's worth having less unsafe code in the program running as privileged root parsing untrusted user data.

cpu: 13th Gen Intel(R) Core(TM) i7-1365U │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ DecodeAmd64-12 2.000µ ± 13% 2.732µ ± 8% +36.63% (p=0.000 n=10)

I think rewriting in go should be a separate issue though. I will submit a separate change if we merge #412
Did a quick look into zydis-go and If I understand correctly - it embeds some precompiled native binaries and I'm not excited to use this for the same reasons described above to not use memory unsafe languages and lack of transparency.

Although I'm happy to apply it here in this PR as well. Both go and C versions looks like almost complete rewrite (touches all the lines )

@christos68k @florianl @athre0z Do you see any blockers for not using the built-in Go disassembler for amd64?

Currently we have Zydis used in interpreter/php, interpreter/python, and tpbase (for kernel/fsbase and libc/pthread).

The commit referenced would fix php. I'm available to rewrite the three others which are simpler. This would allow cross-running amd64 tests on arm64 builds, and remove the 12M .c drop from this repository.

I believe the Go code will be more maintainable and safer. The original reason for including Zydis was the stack delta extraction code inspecting call return locations to determine which stack deltas to keep and thus had requirements the Go disassembler was not able to handle. But this code is long gone, and I'd like to simplify our codebase now.

Does any anyone object this? If no, I would prefer to review the Go version for this fix.

gnurizen · 2025-03-20T12:01:36Z

This is great and thanks for undertaking this! You might consider a docker based testing approach (https://github.com/parca-dev/opentelemetry-ebpf-profiler/blob/main/interpreter/luajit/offsets_test.go) but I think this approach here is fine too. As long as we have tests!

gnurizen · 2025-03-20T14:11:15Z

The nice thing about docker tests is that you can have one be for the "latest" so when new dot releases CI can fail if something changes. I suspect most of the "interesting" instruction sequences we're seeing are coming from PGO/LTO builds which the stock docker builds are not, but apparently the google cloud sdk builds are so we could use those in addition.

gnurizen

LGTM!

korniltsev · 2025-03-21T04:24:20Z

This is great and thanks for undertaking this! You might consider a docker based testing approach (https://github.com/parca-dev/opentelemetry-ebpf-profiler/blob/main/interpreter/luajit/offsets_test.go) but I think this approach here is fine too. As long as we have tests!

Thank you for suggestion.
I love the idea with docker images overall and I think we should add them but not in this particular case. Maybe for better integration testing, or automated-coredump-generation-testing.
The reason I don't see how docker images could be useful here - is that the tests needs some expected values to assert and currently in this PR it is a manual process to prepare them. it's probably possible to reliably extract the expected values from debug packages, but they may not be available in all cases/distros.

Overall I like the idea to have more tests with the help of docker images. We should create a separate issue to not overwhelm the scope of this PR.

merging open-telemetry#412 Squashed commit of the following: commit b0d9b46 Author: Tolya Korniltsev <korniltsev.anatoly@gmail.com> Date: Fri Mar 21 11:18:40 2025 +0700 lint commit aa5ef11 Author: Tolya Korniltsev <korniltsev.anatoly@gmail.com> Date: Thu Mar 20 23:53:53 2025 +0700 more tests, debug dump code on failure commit e4f1c2a Merge: 6a8e9fc 192111e Author: Tolya Korniltsev <korniltsev.anatoly@gmail.com> Date: Thu Mar 20 14:57:17 2025 +0700 Merge branch 'main' into python-fix commit 6a8e9fc Author: Tolya Korniltsev <korniltsev.anatoly@gmail.com> Date: Thu Mar 20 14:37:15 2025 +0700 lint commit b8f926a Author: Tolya Korniltsev <korniltsev.anatoly@gmail.com> Date: Thu Mar 20 11:39:13 2025 +0700 python stub decoding improvements

korniltsev · 2025-03-27T12:29:27Z

Tagging this as review-ready to get some eyes on the pull request. Still want and intend to upload some coredumps once we have a green light.

fabled

Thanks! Looks pretty good already. Some suggestions added. I'd mostly like to see the debug flag go away and use normal logging. The useful pieces of the code can be moved to a disassembly helper module in a follow up PR if preferred that way.

fabled · 2025-04-06T07:54:10Z

-import "C"
+const debugDecodeAMD64 = false
+
+func regIndex(reg x86asm.Reg) int {


We may want this in adm64helpers or similar module to share with the other places to be converted. Similar to armhelpers which already exist. Perhaps those should go to some better descriptive subdirectory like disasm/amd64 and disasm/arm64. This can be also done in a follow up MR if preferred.

fabled · 2025-04-06T07:54:44Z

+	value      uint64
+}
+
+func decodeStubArgumentAMD64(code []byte, codeAddress, memoryBase uint64) uint64 {


This should be in decode.go where the equivalent arm decoder is. That allows running the amd64 tests in arm64 builds.

fabled · 2025-04-06T07:56:56Z

-func decodeStubArgumentWrapper(code []byte, argNumber uint8, symbolValue,
-	addrBase libpf.SymbolValue) libpf.SymbolValue {
-	return decodeStubArgumentWrapperX64(code, argNumber, symbolValue, addrBase)
+func decodeStubArgumentWrapper(


Purpose of this wrapper was to hide the CGO stuff from other files. But now there is no CGO, so perhaps the wrapper could be just directly implementation, or drop the wrapper portion from name and update call sites?

fabled · 2025-04-06T07:58:28Z

@@ -0,0 +1,204 @@
+// Copyright The OpenTelemetry Authors


This whole file should be merged to decode_test.go once the disassembler implementation is moved to decode.go. Then test suite works identically on arm64/amd64 and does not matter which CPU is on developers computer.

fabled · 2025-04-06T08:01:20Z

+	dumpCode := func() {
+		log.Debugf("python stub code: %s", hex.Dump(code))


How about changing debugStub to return also an error and put the dump there?

fabled · 2025-04-06T08:03:04Z

+		if len(rem) >= 4 &&
+			code[instructionOffset] == 0xf3 &&
+			code[instructionOffset+1] == 0x0f &&
+			code[instructionOffset+2] == 0x1e &&
+			code[instructionOffset+3] == 0xfa {


So this is a instruction the disassembler does not support, but we need to recognize and skip? This should go to a helper module. But if preferred, I can do this when working with the other conversions and try to see what would be a nice api.

fabled · 2025-04-06T08:05:44Z

-// #include "decode_amd64.h"
-// #include "../../support/ebpf/types.h"
-import "C"
+const debugDecodeAMD64 = false


I would prefer to drop this. The debug prints that are not important can be dropped, and the meaningful ones should use the same logging mechanism as other code. It was #ifdef in the C code only because adding interface to the logger from C was not found important enough.

korniltsev · 2025-04-06T11:58:00Z

sent coredump blobs here https://cloud-native.slack.com/archives/C03J794L0BV/p1743940210220949?thread_ts=1742556865.154259&cid=C03J794L0BV

@fabled Thanks for review. I've updated the code accordingly. ptal

korniltsev · 2025-04-07T07:32:43Z

+        "fib+3 in /home/korniltsev/trash/fib.py:4",
+        "fib+3 in /home/korniltsev/trash/fib.py:4",
+        "<module>+6 in /home/korniltsev/trash/fib.py:7",
+        "ffffffffffffffffffffffffffffffff+0x0",


This worth creating an issue

fabled

LGTM. Perhaps the modulestore change should be separate PR? I think this is good enough to go in, and we can do rest of the asm helper improvements in follow up PRs when transforming the other instances. Thanks!

fabled · 2025-04-07T12:04:05Z

+	if resp.StatusCode != http.StatusOK {
+		errorResponse, _ := io.ReadAll(resp.Body)
+		return "", fmt.Errorf("store returned %d %s", resp.StatusCode, errorResponse)
+	}
+
+	// Download the file to a temporary location to prevent half-complete modules on crashes.
+	file, err := os.CreateTemp(store.localCachePath, localTempPrefix)
+	if err != nil {
+		return "", fmt.Errorf("failed to create local file: %w", err)
+	}
+	defer file.Close()


This perhaps should/could be a separate PR?

yep, lets split #433

florianl · 2025-04-07T12:14:45Z

+
+package amd // import "go.opentelemetry.io/ebpf-profiler/asm/amd"
+
+func IsEndbr64(code []byte) (isEndbr bool, size int) {


Can we add a comment to the function?

Co-authored-by: Florian Lehner <florianl@users.noreply.github.com>

fabled · 2025-04-09T17:23:20Z

+// IsEndbr64 returns true if the first 4 bytes of the code is endbr64 instruction
+// https://www.felixcloutier.com/x86/endbr64
+// The second returned argument is the size of the instruction which is always 4
+func IsEndbr64(code []byte) (isEndbr bool, size int) {


Could this instead just a Decode, DecodeSkippable or similar? I suspect we may need to add few more here...

fabled

Some stylistic nits that I'd like fixed. I can do moving of code to make the helpers more useful later on (so not a requirement for now).

fabled · 2025-04-09T17:26:13Z

+	if len(code) >= 4 &&
+		code[0] == 0xf3 &&
+		code[1] == 0x0f &&
+		code[2] == 0x1e &&
+		code[3] == 0xfa {


opcodeEndBr64 := []byte{0xf3,0x0f,0x1e,0xfa}

...

switch { cast bytes.HasPrefix(code, opcodeEndbr64): ... }

Possibly needs another case to keep lint happy?

fabled · 2025-04-09T17:27:30Z

+	case x86asm.RSP, x86asm.ESP:
+		return 8
+	case x86asm.RIP:
+		return 9


This is missing registers r8 ... r15. But probably not needed yet?

Let's add them

fabled · 2025-04-09T17:29:08Z

+		if (inst.Op == x86asm.LEA || inst.Op == x86asm.MOV) && inst.Args[0] != nil {
+			if reg, ok := inst.Args[0].(x86asm.Reg); ok {
+				var value uint64
+				var loadedFrom uint64
+
+				switch src := inst.Args[1].(type) {
+				case x86asm.Imm:
+					value = uint64(src)
+				case x86asm.Mem:
+					baseAddr, _ := regs.Get(src.Base)
+					displacement := uint64(src.Disp)
+
+					if inst.Op == x86asm.MOV {
+						value = memoryBase
+						loadedFrom = baseAddr + displacement
+						if src.Index != 0 {
+							indexValue, _ := regs.Get(src.Index)
+							loadedFrom += indexValue * uint64(src.Scale)
+						}
+					} else if inst.Op == x86asm.LEA {
+						value = baseAddr + displacement
+						if src.Index != 0 {
+							indexValue, _ := regs.Get(src.Index)
+							value += indexValue * uint64(src.Scale)
+						}
+					}
+
+				case x86asm.Reg:
+					value, _ = regs.Get(src)
+				}
+
+				regs.Set(reg, value, loadedFrom)
+			}
+		}
+
+		if inst.Op == x86asm.ADD && inst.Args[0] != nil && inst.Args[1] != nil {
+			if reg, ok0 := inst.Args[0].(x86asm.Reg); ok0 {
+				if _, ok1 := inst.Args[1].(x86asm.Mem); ok1 {
+					oldValue, _ := regs.Get(reg)
+					value := oldValue + memoryBase
+					regs.Set(reg, value, 0)
+				}
+			}
+		}
+	}


This probably could go to the helpers. This probably directly reusable in the other modules. Though, I can do this in followup PRs.

Yes, I would prefer to do this in a followup when switching some other decoding routine from c to go

I have a draft for libc + reusability, but it's not ready for review yet
a9f1733#diff-0cf1ffa2fb979e85389ef109707ee2440dbc759f65a2a2f1bf51ea19ef259defR71

fabled · 2025-04-09T17:30:47Z

+	if runtime.GOARCH == "arm64" {
+		return decodeStubArgumentWrapperARM64(code, memoryBase), nil
+	}
+	if runtime.GOARCH == "amd64" {
+		return decodeStubArgumentAMD64(code, uint64(codeAddress), uint64(memoryBase))
+	}


switch runtime.GOARCH { case "arm64": ... default: return error }

korniltsev · 2025-04-15T04:12:56Z

Can I have another round of review please? I'd like to work on a followup which depends on this.

fabled

Thanks! LGTM. Let's do more stuff in follow up PRs.

python stub decoding improvements

b8f926a

This was referenced Mar 20, 2025

Python: frames missing due to failure to resolve autoTLSKey #251

Closed

Fix python3.12 on ubuntu x86-64 #284

Closed

korniltsev force-pushed the python-fix branch from f5c2444 to ab0af10 Compare March 20, 2025 07:47

lint

6a8e9fc

korniltsev force-pushed the python-fix branch from ab0af10 to 6a8e9fc Compare March 20, 2025 07:49

Merge branch 'main' into python-fix

e4f1c2a

gnurizen reviewed Mar 20, 2025

View reviewed changes

florianl reviewed Mar 20, 2025

View reviewed changes

Comment thread interpreter/python/decode_amd64_test.go Outdated

gnurizen approved these changes Mar 20, 2025

View reviewed changes

korniltsev added 2 commits March 21, 2025 09:41

more tests, debug dump code on failure

aa5ef11

lint

b0d9b46

korniltsev mentioned this pull request Mar 25, 2025

pyroscope.ebpf: otel profiler implementation grafana/alloy#2920

Merged

4 tasks

korniltsev marked this pull request as ready for review March 27, 2025 12:29

korniltsev requested review from a team as code owners March 27, 2025 12:29

korniltsev added 4 commits April 4, 2025 20:23

rewrite in go

d1bf67c

lint

3eff3a8

legal

93e569e

Merge branch 'main' into python-fix

3c066cc

fabled reviewed Apr 6, 2025

View reviewed changes

korniltsev added 4 commits April 6, 2025 18:02

remove debug printings

b4e7b57

decodeStub: return error, include hexdump of the code into the error

f42edc8

merge tests into decode_tet.go

893a170

extract regs state into a new amd package

2bd6bdd

korniltsev added 3 commits April 6, 2025 18:03

rm printf

06daab6

add coredump tests

6def09d

update modulestore to include meaningfull errors

36bb7a0

korniltsev commented Apr 7, 2025

View reviewed changes

lint

303cd1a

fabled approved these changes Apr 7, 2025

View reviewed changes

florianl reviewed Apr 7, 2025

View reviewed changes

korniltsev and others added 4 commits April 7, 2025 20:40

Apply suggestions from code review

ab12dbd

Co-authored-by: Florian Lehner <florianl@users.noreply.github.com>

revert modulestore changes

796a617

add endbr64 comment

4372fc3

add extra test for lea edit (32bit)

6573aef

korniltsev requested review from christos68k, fabled and florianl April 7, 2025 14:06

fabled reviewed Apr 9, 2025

View reviewed changes

review fixes

7a1593b

korniltsev requested a review from fabled April 14, 2025 04:42

add r8l-r15l regs

6c6f8ca

fabled approved these changes Apr 17, 2025

View reviewed changes

florianl approved these changes Apr 22, 2025

View reviewed changes

florianl merged commit 49fdb40 into open-telemetry:main Apr 22, 2025
26 checks passed

korniltsev mentioned this pull request Apr 23, 2025

Reusable amd64 interpreter for extraction of values from disassembly #447

Merged

gnurizen mentioned this pull request May 19, 2025

go labels dd parca-dev/opentelemetry-ebpf-profiler#70

Closed

This was referenced Jul 8, 2025

Update with changes from upstream parca-dev/opentelemetry-ebpf-profiler#74

Merged

fix jits parca-dev/opentelemetry-ebpf-profiler#75

Closed

This was referenced Aug 14, 2025

fix golabels tests parca-dev/opentelemetry-ebpf-profiler#92

Closed

fix golabels test parca-dev/opentelemetry-ebpf-profiler#97

Closed

		dumpCode := func() {
		log.Debugf("python stub code: %s", hex.Dump(code))


		package amd // import "go.opentelemetry.io/ebpf-profiler/asm/amd"

		func IsEndbr64(code []byte) (isEndbr bool, size int) {

Conversation

korniltsev commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gnurizen commented Mar 20, 2025

Uh oh!

gnurizen commented Mar 20, 2025

Uh oh!

Uh oh!

gnurizen left a comment

Choose a reason for hiding this comment

Uh oh!

korniltsev commented Mar 21, 2025

Uh oh!

korniltsev commented Mar 27, 2025

Uh oh!

fabled left a comment

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!

korniltsev commented Apr 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabled left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabled left a comment

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!

korniltsev commented Mar 20, 2025 •

edited

Loading