-
Notifications
You must be signed in to change notification settings - Fork 111
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
Temporary workaround for unsupported DWARF location types #626
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #626 +/- ##
===========================================
- Coverage 69.60% 40.98% -28.62%
===========================================
Files 92 89 -3
Lines 7633 7441 -192
===========================================
- Hits 5313 3050 -2263
- Misses 2068 4217 +2149
+ Partials 252 174 -78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Closing by now, as the original issue seems to come from a wrong language detection and not from a weird Go compilation. |
I think we need this :), can we continue with it? I think we can craft a unit test to expose this issue, I think since we give into inspecting ELF files, we'll hit this again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I think it would be nice if we can make a unit test that checks the code works, since we don't have an executable with this kind of DWARF info?
} else { | ||
// Temporary workaround | ||
// TODO: properly address issue https://github.com/grafana/beyla/issues/625 | ||
return fmt.Errorf("at the moment, Beyla only supports constant values for DW_AT_data_member_location;"+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use the logger for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be logged as debug from the code that receives the error.
I added a unit test with mocked data but once we fix the original issue (wrong language detection) I'll add an integration test with a real R executable. |
This is a temporary, first workaround to quickly unblock some non-working users.
As reported in issue #625, we are assuming that all the variable offsets are stored as a constant. According to DWARF specification (page 227) this value can also be an exprloc or a loclist.
Since completely supporting the DWARF spec might take some extra time to address and support, by now we will fallback to the preloaded offsets database when we find a non-constant location.