Add support for offline callstack symbol resolving #665

tiago-rodrigues · 2023-11-17T21:06:30Z

Add support for offline symbol resolving by setting the "TRACY_SYMBOL_OFFLINE_RESOLVE=1" env var.
Add symbol resolving functionality to the "update" tool, with
offline symbol resolvers for linux (using addr2line) and windows (using dbghelper).

There are a few reasons why resolving symbols offline might be useful:

runtime symbol resolving can be slow and use quite a bit memory - specially on windows (which is counter productive when we are doing tracy captures to investigate memory usage and we have a lot of backtrace traffic as we instrument every memory allocation with a an interposer and inject backtraces for each alloc/free).
libbacktrace does not work with dlopenned shared libraries after the first symbol resolve has happened, this can be quite problematic on applications that rely heavily on dynamically loaded libs like Omniverse (we can have ~200 shared libs loaded after starting our app due to the way the extension system works. Still I'll investigate adding a potential solution for this in another PR where we refresh libbacktrace internal state.
doing offline symbol resolving can allow a bit more flexibility on retargeting the resolving to a different set shared libraries (if symbols have been stripped for example) etc.

wolfpld · 2023-11-17T21:58:46Z

I haven't added documentation about this in the PR already as I wanted to float the idea of doing the offline symbol resolving first and see if there were any major objections to it first.

Looks like a very nice addition, judging by the description. Go ahead and write the documentation. I'm not sure about the name "tracy-edit" though, but nothing comes to mind that has a nice ring to it.

I'll do a proper review later.

YaLTeR · 2023-11-18T04:03:03Z

Oh, this would be quite useful for GNOME Shell profiling. I currently have to disable symbol resolution there because GJS seems to very heavily contend on dlsym() with Tracy's symbol workers, causing slowdowns. And callstacks are very useful.

wolfpld · 2023-11-18T14:30:46Z

manual/tracy.tex

+ the minimal set of info required for offline resolution (a shared library path and an offset into that shared library).
+
+The generated tracy capture will have callstack frames symbols showing \texttt{[unresolved]}.
+The \texttt{tracy-edit} tool can be used to load that capture, perforce symbol resolution offline


perforce -> perform

wolfpld · 2023-11-18T14:34:33Z

public/client/TracyCallstack.cpp

@@ -93,6 +93,14 @@ extern "C" const char* ___tracy_demangle( const char* mangled )
 namespace tracy
 {

+// when "TRACY_SYMBOL_OFFLINE_RESOLVE" is set to "1", instead of fully resolving symbols at runtime,
+// simply resolve the offset and image name (which will be enough the resolving to be done offline)
+bool getDoOfflineSymbolResolve()


It would probably be a good idea to add a #ifdef TRACY_SYMBOL_OFFLINE_RESOLVE to enable hardcode enabling this at build time, like you can do with some other options. Also, rename this function to ShouldResolveSymbolsOffline.

makes sense, I'll try to add support for the compile time ifdef in another commit later today/tomorrow as it will require a bit of #ifdefing in that file to make sure the libbacktrace/dbghelper is ifdefed out

Add the ifdef as an option to the user, but also keep the current env variable based solution.

Ah, I see you already committed the changes. I'm not sure it's worth having ifdefs scattered all over the code. Sure, it's dead code, but it also increases the mental effort required to understand what's going on.

wolfpld · 2023-11-18T14:36:28Z

public/client/TracyCallstack.cpp

+
+DWORD64 dbgHelpLoadSymbolsForModule(PCSTR imageName, DWORD64 baseOfDll, DWORD bllSize)
+{
+    if( !s_doOfflineSymbolResolve )


if( s_doOfflineSymbolResolve ) return 0;

wolfpld · 2023-11-18T14:36:50Z

public/client/TracyCallstack.cpp

+    if( s_doOfflineSymbolResolve )
+    {
+        return "[unresolved]";
+    }


Write as a one-liner.

wolfpld · 2023-11-18T14:38:21Z

public/client/TracyCallstack.cpp


 void InitCallstackCritical()
 {
    ___tracy_RtlWalkFrameChain = (___tracy_t_RtlWalkFrameChain)GetProcAddress( GetModuleHandleA( "ntdll.dll" ), "RtlWalkFrameChain" );
 }

-void InitCallstack()
+void dbgHelpInit()


Keep function names capitalized.

wolfpld · 2023-11-18T14:38:54Z

public/client/TracyCallstack.cpp

+#endif
+}
+
+DWORD64 dbgHelpLoadSymbolsForModule(PCSTR imageName, DWORD64 baseOfDll, DWORD bllSize)


Follow the convention wrt to how spaces are used, etc.

wolfpld · 2023-11-18T14:40:00Z

public/client/TracyCallstack.cpp

+    // when doing offline symbol resolution, we must store the full path of the dll for the resolving to work 
+    if( s_doOfflineSymbolResolve )
+    {
+        cachedModule->name = (char*)tracy_malloc_fast(imageNameLength + 2);


Why is there need for two additional characters?

you are right, it was a copy paste from the code below - fixed

wolfpld · 2023-11-18T14:43:29Z

server/TracyWorker.cpp

@@ -2411,6 +2426,19 @@ const char* Worker::GetString( const StringIdx& idx ) const
    return m_data.stringData[idx.Idx()];
 }

+uint32_t Worker::AddNewString(const char* newString)


Why is there a need for a new function to add strings?

There is no need really, the only thing diff was this one was public and had that extra assert.
I've removed it and made StoreString public instead.

wolfpld · 2023-11-18T14:46:19Z

tracy-edit/src/tracy-edit.cpp

+    if (resolver)
+    {
+        PatchSymbols(resolver, worker, pathSubstitutionList, args.verbose);
+


Remove newline.

wolfpld · 2023-11-18T14:47:11Z

tracy-edit/src/OfflineSymbolResolverDbgHelper.cpp

+    return false;
+}
+
+#endif // #ifdef _WIN32


Add EOF newlines.

wolfpld · 2023-11-18T14:47:44Z

tracy-edit/src/OfflineSymbolResolverDbgHelper.cpp

+                    const FrameEntryList& inputEntryList,
+                    SymbolEntryList& resolvedEntries)
+{
+    if( resolver )


if( !resolver ) return false;

wolfpld · 2023-11-18T14:49:13Z

tracy-edit/src/OfflineSymbolResolverDbgHelper.cpp

+void DestroySymbolResolver(SymbolResolver* resolver)
+{
+    delete resolver;
+}


Why is new and delete relegated to functions here?

The code was a bit different before when I had more state in SymbolResolver which is a private type in the CPP. There is no need for this right, removed it.

wolfpld · 2023-11-19T13:35:36Z

The conflicting gcc.yml has been split into linux.yml and macos.yml, you should be able to just copy and paste your change to resolve the conflict. See 906f73c.

…_OFFLINE_RESOLVE=1" env var - Add a tool "tracy-edit" that allows loading a tracy capture, patching symbols and recompress the result - Add offline symbol resolvers for linux (using addr2line) and windows (using dbghelper)

- delete CreateResolver/DestroySymbolResolver

… only the offline symbol resolving codepath

…w only force enables the offline symbol resolving codepath.

…e linux github workflow (but not locally, likely due to diff compiler)

tiago-rodrigues · 2023-11-19T15:03:12Z

Oh, this would be quite useful for GNOME Shell profiling. I currently have to disable symbol resolution there because GJS seems to very heavily contend on dlsym() with Tracy's symbol workers, causing slowdowns. And callstacks are very useful.

@YaLTeR: although it won't call into libbacktrace anymore to resolve symbols, not sure if it will still have some contention as it still uses dladdr() (to get the .so path and base address)? If so, maybe maintaining our own .so range mapping and updating with dl_iterate_phdr when we we get a request that doesn't land in any known ranges, would be a way to avoid it.

YaLTeR · 2023-11-19T15:36:46Z

Hmm, if it still uses dladdr() then I suspect it will still contend :/

tiago-rodrigues · 2023-11-20T21:00:11Z

Hmm, if it still uses dladdr() then I suspect it will still contend :/

It's probably best to tackle this in another PR? However, I added a possible solution here tiago-rodrigues@08db7d3, adding an image range cache to avoid calling dladdr() when doing offline symbol resolution (and dl_iterate_phdr is supported). It should only refresh when an address is encountered that's outside of the known ranges. It is also used for the runtime symbol resolution case to re-create libbacktrace state and allow symbol resolution for dynamically loaded shared libs (at the cost of mem leaking when it refreshes as I can't see any libbacktrace state freeing/recreation.)

wolfpld · 2023-11-24T20:58:17Z

CMakeLists.txt

@@ -85,6 +85,7 @@ set_option(TRACY_FIBERS "Enable fibers support" OFF)
 set_option(TRACY_NO_CRASH_HANDLER "Disable crash handling" OFF)
 set_option(TRACY_TIMER_FALLBACK "Use lower resolution timers" OFF)
 set_option(TRACE_CLIENT_LIBUNWIND_BACKTRACE "Use libunwind backtracing where supported" OFF)
+set_option(TRACY_SYMBOL_OFFLINE_RESOLVE "Instead of runtime symbol resolving, sore offset and sharedlib path for offline symbol resolution" OFF)


Is "sore" the right word here?

sorry it was a typo, fixed

wolfpld · 2023-11-24T21:05:49Z

public/client/TracyCallstack.cpp

@@ -401,16 +468,29 @@ CallstackSymbolData DecodeSymbolAddress( uint64_t ptr )

 CallstackEntryData DecodeCallstackPtr( uint64_t ptr )
 {
+    InitRpmalloc();
+
+    const ModuleNameAndBaseAddress moduleNameAndAddress = GetModuleNameAndPrepareSymbols( ptr );


This is outside dbghelp lock.

I've moved the locking up to include it.
Maybe it would be better to have a scoped wrapper for this lock though.

wolfpld · 2023-11-24T21:08:04Z

public/client/TracyCallstack.cpp

@@ -938,20 +1039,42 @@ void SymInfoError( void* /*data*/, const char* /*msg*/, int /*errnum*/ )
    cb_data[cb_num-1].symAddr = 0;
 }

+void getSymbolForOfflineResolve(void* address, Dl_info& dlinfo, CallstackEntry& cbEntry)


Capitalize function names.

wolfpld · 2023-11-24T21:09:51Z

public/client/TracyCallstack.cpp

+            symloc = dlinfo.dli_fname;
+        }
+
+        if(cb_bts && !s_shouldResolveSymbolsOffline)


Is there a case where both these checks are necessary?

no there shouldn't be, fixed it

wolfpld · 2023-11-24T21:32:03Z

tracy-edit/src/tracy-edit.cpp

+        { "compression", required_argument, NULL, 'c' },
+        { "compressesionLevel", required_argument, NULL, 'l' },
+        { NULL, 0, NULL, 0 }
+    };


Can you unify processing of compression parameters with the update utility?

oh, I have to admit I didn't realize the update tool already did that. Maybe I should actually move the symbol resolution to it instead of creating the tracy-edit tool then?

It may make sense to do that. Try to keep the new functionality in a separate source file.

wolfpld · 2023-11-24T21:35:49Z

tracy-edit/src/OfflineSymbolResolver.cpp

+    uint64_t callstackFrameCount = worker.GetCallstackFrameCount();
+    std::string relativeSoNameMatch = "[unresolved]";
+
+    std::cout << "Found '" << callstackFrameCount << "' callstack frames. Batching into image groups..." << std::endl;


Is the count supposed to be quoted here?

I've removed all the numeric quoting in there

…tead of creating a new one

wolfpld · 2023-11-25T15:52:41Z

I did a quick test and the resolved symbols don't show up in the symbol statistics list. Is this something you will be working on? If not, I think the documentation should state that there are functional differences.

…n using offline symbol resolving

tiago-rodrigues · 2023-11-27T15:25:15Z

I did a quick test and the resolved symbols don't show up in the symbol statistics list. Is this something you will be working on? If not, I think the documentation should state that there are functional differences.

Right, I forgot to tackle that one. I was mainly using the memory view for the workflow I'm working right now.
I've updated the documentation to reflect this limitation.

I'll try to do another PR after where I'll attempt to fixup the symbol map for the newly resolved symbols. However it might be a bit more involved if I ended up avoiding calling dladdr (which I'll have another PR as well in order to reduce the contention mentioned by YalTeR : #665 (comment)).

wolfpld reviewed Nov 18, 2023

View reviewed changes

Tiago Rodrigues added 15 commits November 19, 2023 06:32

try to add tracy-edit to github workflows

a24fec9

add tracy-edit debug compilation workflow

5c3e303

enable addr2line symbol resolver in all platforms except windows

a887d55

use pkg-config to get libcapstone config

94e7da1

Add some basic documentation for offline symbols

fe0e5f3

Add support for symbol path replacement using regex in tracy-edit

0491cad

fix typo

3fdd1e1

fix formatting

c302b50

Instead of adding new method, make StoreString public

687d681

- Fix formatting

5c05139

- delete CreateResolver/DestroySymbolResolver

fix linux compilation

b3f6880

Add support for using defining TRACY_SYMBOL_OFFLINE_RESOLVE to enable…

79618c5

… only the offline symbol resolving codepath

revert #ifdef of a lot of code by TRACY_SYMBOL_OFFLINE_RESOLVE, it no…

301c9fb

…w only force enables the offline symbol resolving codepath.

update linux github workflow after rebase

e04e595

tiago-rodrigues force-pushed the trodrigues/offline_symbol_resolve branch from 8b8815b to e04e595 Compare November 19, 2023 14:37

add mising cstdint include that was saucing compilation failure in th…

6c0bb0a

…e linux github workflow (but not locally, likely due to diff compiler)

tiago-rodrigues force-pushed the trodrigues/offline_symbol_resolve branch from 1ebf8fe to 6c0bb0a Compare November 20, 2023 20:53

tiago-rodrigues requested a review from wolfpld November 24, 2023 14:56

wolfpld reviewed Nov 24, 2023

View reviewed changes

update cmake option description

e5e8442

wolfpld reviewed Nov 24, 2023

View reviewed changes

fixes according to review comments

a1ca8dd

wolfpld reviewed Nov 24, 2023

View reviewed changes

Tiago Rodrigues added 2 commits November 24, 2023 20:48

move the symbol resolving functionality to the update application ins…

de6e7d2

…tead of creating a new one

update github workflows to remore tracy-edit

69b9707

update documentation to reflect that symbol statistics are broken whe…

2509a4b

…n using offline symbol resolving

wolfpld merged commit af73dba into wolfpld:master Nov 27, 2023
5 checks passed

tiago-rodrigues mentioned this pull request Nov 29, 2023

Add image cache to avoid calling dladdr() and add libbacktrace elf image list refresh #674

Merged

Add support for offline callstack symbol resolving #665

Add support for offline callstack symbol resolving #665

Conversation

tiago-rodrigues commented Nov 17, 2023 • edited Loading

wolfpld commented Nov 17, 2023

YaLTeR commented Nov 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolfpld commented Nov 19, 2023

tiago-rodrigues commented Nov 19, 2023

YaLTeR commented Nov 19, 2023

tiago-rodrigues commented Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiago-rodrigues Nov 25, 2023 • edited Loading

Choose a reason for hiding this comment

wolfpld commented Nov 25, 2023

tiago-rodrigues commented Nov 27, 2023

tiago-rodrigues commented Nov 17, 2023 •

edited

Loading

YaLTeR commented Nov 18, 2023 •

edited

Loading

tiago-rodrigues commented Nov 20, 2023 •

edited

Loading

tiago-rodrigues Nov 25, 2023 •

edited

Loading