Skip to content

php: cgo-less amd routine decoders#604

Merged
christos68k merged 3 commits intoopen-telemetry:mainfrom
grafana:korniltsev/cgoless_php
Jul 14, 2025
Merged

php: cgo-less amd routine decoders#604
christos68k merged 3 commits intoopen-telemetry:mainfrom
grafana:korniltsev/cgoless_php

Conversation

@korniltsev
Copy link
Copy Markdown
Contributor

This PR is the continuation of #447

Use the new amd.Interpreter instead of cgo decoders in the php package.

Copy link
Copy Markdown
Contributor

@fabled fabled left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@korniltsev korniltsev marked this pull request as ready for review July 14, 2025 13:32
@korniltsev korniltsev requested review from a team as code owners July 14, 2025 13:32
Copy link
Copy Markdown
Member

@christos68k christos68k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some nits

Comment thread interpreter/php/decode_aarch64.go Outdated
Comment thread interpreter/php/decode_aarch64.go Outdated
"golang.org/x/arch/x86/x86asm"
)

// retrieveZendVMKindX86. This function reads the code blob and recovers
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// retrieveZendVMKindX86. This function reads the code blob and recovers
// retrieveZendVMKindX86 reads the code blob and recovers

Comment thread interpreter/php/decode_x86.go Outdated
Comment thread interpreter/php/decode_x86.go Outdated
Comment thread interpreter/php/decode_x86.go Outdated
if res.Match(imm) {
return uint(imm.CapturedValue()), nil
}
return 0, errors.New("failed to decode zend_vm_kind: target not found")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We can turn this error into a global variable

Suggested change
return 0, errors.New("failed to decode zend_vm_kind: target not found")
return 0, errors.New("failed to decode zend_vm_kind: target not found")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these 3 errors are different cause include the function name in it.

Comment thread interpreter/php/decode_x86.go Outdated
return libpf.SymbolValue(imm.CapturedValue()), nil
}
return libpf.SymbolValueInvalid,
fmt.Errorf("failed to decode execute_ex: %s", "target not found")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to use a format string here, also we can turn this error into a global var

Suggested change
fmt.Errorf("failed to decode execute_ex: %s", "target not found")
fmt.Errorf("failed to decode execute_ex: %s", "target not found")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed format string, kept a local errors.New

Comment thread interpreter/php/decode_x86.go Outdated
return rdiValue, rsiValue, nil
}
return libpf.SymbolValueInvalid, libpf.SymbolValueInvalid,
fmt.Errorf("failed to recover jit buffer: %s", "target not found")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to use a format string here, we can turn this error into a global var

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed format string, kept a local errors.New

@korniltsev korniltsev force-pushed the korniltsev/cgoless_php branch from 4fbe2cc to 18095be Compare July 14, 2025 16:29
@christos68k christos68k merged commit 2188142 into open-telemetry:main Jul 14, 2025
27 checks passed
gnurizen pushed a commit to parca-dev/opentelemetry-ebpf-profiler that referenced this pull request Sep 8, 2025
gnurizen pushed a commit to parca-dev/opentelemetry-ebpf-profiler that referenced this pull request Sep 8, 2025
gnurizen pushed a commit to parca-dev/opentelemetry-ebpf-profiler that referenced this pull request Sep 9, 2025
gnurizen pushed a commit to parca-dev/opentelemetry-ebpf-profiler that referenced this pull request Sep 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants