Allow symbolz to try symbolize unsymbolizable mappings. #907
Conversation
We added skipping unsymbolizable mappings in symbolz long time ago in PR google#368 to address google#339 where we saw error like "unexpected negative adjusted address". But that error was fixed in a more proper way in subsequent google#397 to fix another reported issue google#280 (and internal b/32020573). So skipping unsymbolized mappings shouldn't be needed anymore. I tested this by verifying that the test case from google#339 still works fine with the proposed change. And that it does fail if I roll back google#397 locally. This change is useful as we experiment with using symbolz to symbolize JIT locations from //anon (which is unsymbolizable per current terminology).
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #907 +/- ##
==========================================
+ Coverage 66.86% 66.97% +0.10%
==========================================
Files 44 44
Lines 9824 9855 +31
==========================================
+ Hits 6569 6600 +31
- Misses 2794 2816 +22
+ Partials 461 439 -22 ☔ View full report in Codecov by Sentry. |
| // attempts symbolization of addresses from unsymbolizable system | ||
| // mappings as those may look negative - e.g. "[vsyscall]". | ||
| // symbolize locations from mappings not already marked as HasFunctions. Does | ||
| // not skip unsymbolizable files since the symbolz handler can be flexible |
There was a problem hiding this comment.
I feel this comment may feel out of context after the patch is submitted and the check on m.Unsymbolizable() is not there anymore. Most people won't think "why aren't we skipping unsymbolizable mappings?".
Maybe just skip the comment here?
Optionally, perhaps the package comments could have a bit more details about symbolz and how it can get symbols for JITed code.
There was a problem hiding this comment.
I mostly added this comment for future us in case we consider adding the unsymbolizable check again.
There was a problem hiding this comment.
I don't have a strong opinion about it. We can submit as is.
| // attempts symbolization of addresses from unsymbolizable system | ||
| // mappings as those may look negative - e.g. "[vsyscall]". | ||
| // symbolize locations from mappings not already marked as HasFunctions. Does | ||
| // not skip unsymbolizable files since the symbolz handler can be flexible |
There was a problem hiding this comment.
I don't have a strong opinion about it. We can submit as is.
We added skipping unsymbolizable mappings in symbolz long time ago in PR #368 to address #339 where we saw error like "unexpected negative adjusted address".
But that error was fixed in a more proper way in subsequent #397 to fix another reported issue #280 (and internal b/32020573). So skipping unsymbolized mappings shouldn't be needed anymore.
I tested this by verifying that the test case from #339 still works fine with the proposed change. And that it does fail if I roll back #397 locally.
This change is useful as we experiment with using symbolz to symbolize JIT locations from //anon (which is unsymbolizable per current terminology).