Skip to content

Improve elfehframe handling#564

Merged
fabled merged 5 commits intoopen-telemetry:mainfrom
fabled:elfehframe-cleanup
Jul 2, 2025
Merged

Improve elfehframe handling#564
fabled merged 5 commits intoopen-telemetry:mainfrom
fabled:elfehframe-cleanup

Conversation

@fabled
Copy link
Copy Markdown
Contributor

@fabled fabled commented Jun 27, 2025

  • elfehtable: improvements in code, remove concurrency limitation
  • refactor tests not to need temporary files
  • refactor sorted out from fdeInfo as it does not belong there

fabled added 3 commits June 27, 2025 23:27
Give the information via a ehframeHooks instead. This information
does not belong in the fdeInfo.
- minor improvements and bug fixes
- remove restriction on concurrency
@fabled fabled force-pushed the elfehframe-cleanup branch from 3d515cd to 4d2e214 Compare June 27, 2025 21:00
@fabled fabled marked this pull request as ready for review June 27, 2025 21:10
@fabled fabled requested review from a team as code owners June 27, 2025 21:10
Comment thread nativeunwind/elfunwindinfo/elfehframe.go
Comment thread nativeunwind/elfunwindinfo/elfehframetable.go
@fabled fabled requested a review from christos68k June 28, 2025 06:12
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.

LGTM

Comment on lines +90 to +92
if ee.ref == nil {
return nil
}
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.

Do we need this check? Looking at the current code I don't see a case where ee.ref will ever be nil.

If there is such a case, maybe it makes sense to lift this check higher in the call stack to avoid getting to this point.

Suggested change
if ee.ref == nil {
return nil
}

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.

There is a test case that sets elf reference to nil. I'll relocate this test.

@fabled fabled merged commit b3452d1 into open-telemetry:main Jul 2, 2025
27 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.

3 participants