Fix the OSX demangling fallback comparison.#939
Conversation
This is a follow-up to google#924. In the comparison we should compare to the string with stripped underscore, otherwise the comparison will never be true and we strip the underscore in cases we shouldn't. See the added test case. Also, while we are here, add copying demangling options that we also missed. Hopefully we can get ianlancetaylor/demangle#24 in and this won't be needed.
|
@alk @ianlancetaylor FYI |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #939 +/- ##
==========================================
+ Coverage 66.86% 66.89% +0.03%
==========================================
Files 44 44
Lines 9824 9870 +46
==========================================
+ Hits 6569 6603 +34
- Misses 2794 2828 +34
+ Partials 461 439 -22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for catching this. I apologize for the trouble. |
Recent pprof update fixed another issue in leading underscore handling on OSX, but that somehow broke built-in filtering out of tcmalloc symbols. So for now lets manually hide tc_newarray there while investigating proper fix upstream. pprof reference: google/pprof#939
|
So this "broke" gperftools tools tests on OSX. Basically there are some unmangled symbols that we expected to be hidden when processing legacy profiles. In this case it was tc_newarray. Which of course on OSX is _tc_newarray. So this "side effect" of removing '_' that was removed in this fix, we kinda needed it. For now I am manually passing --hide= flag to fix the test. But looks like the underlying issue is that we really should un-tangle underscore prefix removal on OSX from demangling. Or most ideally when extracting raw symbols whatever OSX-specific thing does (well we don't, we just invoke usual set of tools; some of those are actually already automagically strip this stuff). Curious to see people's opinion on this. |
|
If I'm reading the code correctly, it tries to demangle the symbol, and if the demangling fails and the symbol starts with an underscore, it tries demangling the symbols without the underscore. In my opinion that is a bad approach. Whether symbols have a leading underscore or not is determined by the platform—the combination of For example, see https://go.googlesource.com/go/+/3432c68467d50ffc622fed230a37cd401d82d4bf/src/cmd/link/internal/ld/ar.go#227 for the (rather simple) code that strips a leading underscore for targets supported by Go. If there are cases where pprof doesn't know the appropriate |
5 participants
This is a follow-up to #924. In the comparison we should compare to the string with stripped underscore, otherwise the comparison will never be true and we strip the underscore in cases we shouldn't. See the added test case.
Also, while we are here, add copying demangling options that we also missed. Hopefully we can get
ianlancetaylor/demangle#24 in and this won't be needed.