Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

btf: synthesise instruction comments into line info #1417

Merged
merged 2 commits into from
Apr 8, 2024

Conversation

MarcusWichelmann
Copy link
Contributor

@MarcusWichelmann MarcusWichelmann commented Apr 3, 2024

When an asm.Comment is added to an instruction, existing source information like btf.Line will be replaced. But in some cases, instructions without line information won't pass the verifier. The expected behavior would be, that the comment gets synthesized into a line info where everything but the line string is zero. This PR implements this.

Fixes #1413

btf/ext_info.go Outdated Show resolved Hide resolved
btf/ext_info.go Show resolved Hide resolved
btf/ext_info.go Outdated Show resolved Hide resolved
When an asm.Comment is added to an instruction, existing source
information like btf.Line will be replaced. But in some cases,
instructions without line information won't pass the verifier. The
expected behavior would be, that the comment gets synthesized into a
line info where everything but the line string is zero. This commit
implements this.

Fixes cilium#1413

Signed-off-by: Marcus Wichelmann <mail@marcusw.de>
Signed-off-by: Marcus Wichelmann <mail@marcusw.de>
@MarcusWichelmann
Copy link
Contributor Author

I'm not sure why the arm64 CI job fails and if this could be related to this PR.

The job running on runner 9cc722ffc9bb5c1dff1132904f63cda3c36d7769 has exceeded the maximum execution time of 10 minutes.

@dylandreimerink
Copy link
Member

Yea, its a bit odd. It almost looks like the test runner gets stuck, perhaps a struck test. It seems flaky, we had the same on the initial push, which was resolved after a retry.

Unfortunately the CI action gets killed before the test framework itself, so we don't get any info about which test might be stuck. The default go timeout is 10m as well, so I will see if we can extend the runner timeout to something like 15m so that the go tests can exit on its own accord on a stuck test and give us some clue about its source.

@dylandreimerink
Copy link
Member

I am fairly certain its #1419 since I ran into it while making the PR to up the timeout, so don't worry about it.

Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Thanks!

@lmb lmb merged commit aa3208e into cilium:main Apr 8, 2024
15 checks passed
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.

Allow changing line info data in btf.Line
3 participants