Fix: Correctly instrument newproc1 for Go 1.23+ parameter counts (#13188) #216
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes the instrumentation logic for the runtime.newproc1 function by dynamically checking the Go version to determine the correct number of expected parameters.
- Updated the parameter count logic in the runtime instrumentation based on the Go version.
- Introduced a new helper method, CheckGoVersionGreaterOrEqual, to verify the Go version from the -lang flag.
- Updated the CHANGES.md file to document the fix.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/go-agent/instrument/runtime/instrument.go | Update of parameter count check for newproc1 instrumentation |
| tools/go-agent/instrument/api/flags.go | Addition of CheckGoVersionGreaterOrEqual and version parsing logic |
| CHANGES.md | Documentation update for the new instrumentation parameter counts |
| if len(n.Type.Params.List) != 3 { | ||
| expectedParamCount := 3 | ||
| if r.opts.CheckGoVersionGreaterOrEqual(1, 23) { | ||
| expectedParamCount = 5 |
There was a problem hiding this comment.
According to the PR description, for Go 1.23 and later, runtime.newproc1 should expect 2 parameters rather than 5. Please update the logic to set the expected parameter count to 2 for Go 1.23+.
| expectedParamCount = 5 | |
| expectedParamCount = 2 |
| } | ||
| currentMinor := int(currentMinor64) | ||
|
|
||
| if currentMajor > requiredMinor { |
There was a problem hiding this comment.
The comparison should be with requiredMajor, not requiredMinor, to correctly determine if the current major version exceeds the required major version.
| if currentMajor > requiredMinor { | |
| if currentMajor > requiredMajor { |
| if currentMajor > requiredMinor { | ||
| return true | ||
| } | ||
| if currentMajor == requiredMajor && currentMinor >= requiredMajor { |
There was a problem hiding this comment.
The condition incorrectly compares currentMinor with requiredMajor; it should compare currentMinor with requiredMinor for an accurate version check.
| if currentMajor == requiredMajor && currentMinor >= requiredMajor { | |
| if currentMajor == requiredMajor && currentMinor >= requiredMinor { |
|
Hi @zJiaJun. Looks like the go agent don’t have the logical to verify the tracer on cross goroutine scenario. Could you help to add one? |
Hi @mrproliu, thanks for the feedback! That's a valid point. I agree that adding a specific test case to verify tracer propagation across goroutines is important for this fix. I will work on adding a new test scenario for this. Let's discuss the specific implementation details if needed. |
|
Please add ASF APLv2 header to fix CI. |
|
irisv12 case seems to have some issues. Could you try to fix that? @zJiaJun |
Hi @wu-sheng |
|
@mrproliu Could you recheck this test failing? Any suggestion? |
3 participants
Fixes #13188
Problem:
The runtime instrumentation logic in
tools/go-agent/instrument/runtime/instrument.goincorrectly checked for a hardcoded number of parameters (3) for the internalruntime.newproc1function. This check failed for Go versions 1.23 and later, where the relevant function signature expects 2 parameters for the instrumentation purpose. Consequently, the agent skipped the injection of the necessary code for automatic context propagation when new goroutines were created usingnewproc1, breaking automatic tracing in Go 1.23+ environments.Solution:
This patch addresses the issue by implementing dynamic parameter count checking based on the Go version specified during compilation via the
-langflag:CheckGoVersionGreaterOrEqual(major, minor int) boolto theapi.CompileOptionsstruct intools/go-agent/instrument/api/flags.go. This method reliably parses the-langflag value (e.g., "go1.22", "go1.23", "go2.0") to accurately determine the Go version and compare it against required major and minor versions.FilterAndEditmethod withinruntime.Instrument(tools/go-agent/instrument/runtime/instrument.go). It now utilizes the newopts.CheckGoVersionGreaterOrEqual(1, 23)method to determine theexpectedParamCountfornewproc1(3 if Go version < 1.23, 2 if Go version >= 1.23). The instrumentation logic fornewproc1now only proceeds if the detected parameter count matches the expected count for the target Go version.Impact:
With this fix, the skywalking-go agent can now correctly identify and instrument the
runtime.newproc1function on Go 1.23 and later versions, in addition to older versions. This ensures that the automatic context propagation mechanism functions as expected for goroutines created via the standardgokeyword across all supported Go versions specified by the-langflag during the build process.Files Changed:
tools/go-agent/instrument/api/flags.gotools/go-agent/instrument/runtime/instrument.go