Skip to content

symblib: Add test case for go binaries symbconv#411

Merged
florianl merged 2 commits intoopen-telemetry:mainfrom
simonswine:20250318_add-gosym-test
Apr 8, 2025
Merged

symblib: Add test case for go binaries symbconv#411
florianl merged 2 commits intoopen-telemetry:mainfrom
simonswine:20250318_add-gosym-test

Conversation

@simonswine
Copy link
Copy Markdown
Contributor

@simonswine simonswine commented Mar 19, 2025

Noticed Gosym(BadLineNumber) in the go-1.24.0 binary. So I have added handling in symbconv and a test.

WARN: bad line number of function (go:textfipsstart)
WARN: bad line number of function (go:textfipsend)

Noticed Gosym(BadLineNumber) in the go-1.24.0 binary:

WARN: bad line number of function (go:textfipsstart)
WARN: bad line number of function (go:textfipsend)
@simonswine simonswine marked this pull request as ready for review March 19, 2025 13:58
@simonswine simonswine requested review from a team as code owners March 19, 2025 13:58
Comment thread rust-crates/symblib/src/symbconv/go.rs Outdated
continue 'outer;
}
Err(e) => {
return Err(e.into());
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.

Just a thought and maybe we can add more context to the error.

Suggested change
return Err(e.into());
return Err(Error::GoSymbol(e).context("Error in line mapping"));

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.

Still fairly new to rust, but do think this would require me to use anyhow?

I have defined a new Error type GoSymbolBadLineMapping, that transports this context. Let me know you think.

Comment thread rust-crates/symblib/src/symbconv/go.rs Outdated
Comment thread rust-crates/symblib/src/symbconv/go.rs Outdated
Copy link
Copy Markdown
Member

@florianl florianl left a comment

Choose a reason for hiding this comment

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

Thanks for the update 👍

@florianl florianl requested a review from athre0z March 24, 2025 08:00
@florianl florianl merged commit 3af9dbc into open-telemetry:main Apr 8, 2025
26 checks passed
florianl added a commit that referenced this pull request Apr 8, 2025
#411 got created before Rust binary blobs were checked for differences but got merged after this check got in place. As #411 introduces some changes to the Rust code, the Rust binary blobs change and therefore the check for differences fails.
Fix this by compiling the Rust binary blobs.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
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.

3 participants