[lldb] Remerge #136236 (Avoid force loading symbols in statistics collection

[royitaqi]

Fix a test failure in #136236, apply a minor renaming of statistics, and remerge. See details below.

Changes in #136236

Currently, DebuggerStats::ReportStatistics() calls Module::GetSymtab(/*can_create=*/false), but then the latter calls SymbolFile::GetSymtab(). This will load symbols if haven't yet. See stacktrace below.

The problem is that DebuggerStats::ReportStatistics should be read-only. This is especially important because it reports stats for symtab parsing/indexing time, which could be affected by the reporting itself if it's not read-only.

This patch fixes this problem by adding an optional parameter SymbolFile::GetSymtab(bool can_create = true) and receiving the false value passed down from Module::GetSymtab(/*can_create=*/false) when the call is initiated from DebuggerStats::ReportStatistics().

---

Notes about the following stacktrace:

1. This can be reproduced. Create a helloworld program on macOS with dSYM, add settings set target.preload-symbols false to ~/.lldbinit, do lldb a.out, then statistics dump.
2. ObjectFile::GetSymtab has llvm::call_once. So the fact that it called into ObjectFileMachO::ParseSymtab means that the symbol table is actually being parsed.

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = step over
    frame #0: 0x0000000124c4d5a0 LLDB`ObjectFileMachO::ParseSymtab(this=0x0000000111504e40, symtab=0x0000600000a05e00) at ObjectFileMachO.cpp:2259:44
  * frame #1: 0x0000000124fc50a0 LLDB`lldb_private::ObjectFile::GetSymtab()::$_0::operator()(this=0x000000016d35c858) const at ObjectFile.cpp:761:9
    frame #5: 0x0000000124fc4e68 LLDB`void std::__1::__call_once_proxy[abi:v160006]<std::__1::tuple<lldb_private::ObjectFile::GetSymtab()::$_0&&>>(__vp=0x000000016d35c7f0) at mutex:652:5
    frame #6: 0x0000000198afb99c libc++.1.dylib`std::__1::__call_once(unsigned long volatile&, void*, void (*)(void*)) + 196
    frame #7: 0x0000000124fc4dd0 LLDB`void std::__1::call_once[abi:v160006]<lldb_private::ObjectFile::GetSymtab()::$_0>(__flag=0x0000600003920080, __func=0x000000016d35c858) at mutex:670:9
    frame #8: 0x0000000124fc3cb0 LLDB`void llvm::call_once<lldb_private::ObjectFile::GetSymtab()::$_0>(flag=0x0000600003920080, F=0x000000016d35c858) at Threading.h:88:5
    frame #9: 0x0000000124fc2bc4 LLDB`lldb_private::ObjectFile::GetSymtab(this=0x0000000111504e40) at ObjectFile.cpp:755:5
    frame #10: 0x0000000124fe0a28 LLDB`lldb_private::SymbolFileCommon::GetSymtab(this=0x0000000104865200) at SymbolFile.cpp:158:39
    frame #11: 0x0000000124d8fedc LLDB`lldb_private::Module::GetSymtab(this=0x00000001113041a8, can_create=false) at Module.cpp:1027:21
    frame #12: 0x0000000125125bdc LLDB`lldb_private::DebuggerStats::ReportStatistics(debugger=0x000000014284d400, target=0x0000000115808200, options=0x000000014195d6d1) at Statistics.cpp:329:30
    frame #13: 0x0000000125672978 LLDB`CommandObjectStatsDump::DoExecute(this=0x000000014195d540, command=0x000000016d35d820, result=0x000000016d35e150) at CommandObjectStats.cpp:144:18
    frame #14: 0x0000000124f29b40 LLDB`lldb_private::CommandObjectParsed::Execute(this=0x000000014195d540, args_string="", result=0x000000016d35e150) at CommandObject.cpp:832:9
    frame #15: 0x0000000124efbd70 LLDB`lldb_private::CommandInterpreter::HandleCommand(this=0x0000000141b22f30, command_line="statistics dump", lazy_add_to_history=eLazyBoolCalculate, result=0x000000016d35e150, force_repeat_command=false) at CommandInterpreter.cpp:2134:14
    frame #16: 0x0000000124f007f4 LLDB`lldb_private::CommandInterpreter::IOHandlerInputComplete(this=0x0000000141b22f30, io_handler=0x00000001419b2aa8, line="statistics dump") at CommandInterpreter.cpp:3251:3
    frame #17: 0x0000000124d7b5ec LLDB`lldb_private::IOHandlerEditline::Run(this=0x00000001419b2aa8) at IOHandler.cpp:588:22
    frame #18: 0x0000000124d1e8fc LLDB`lldb_private::Debugger::RunIOHandlers(this=0x000000014284d400) at Debugger.cpp:1225:16
    frame #19: 0x0000000124f01f74 LLDB`lldb_private::CommandInterpreter::RunCommandInterpreter(this=0x0000000141b22f30, options=0x000000016d35e63c) at CommandInterpreter.cpp:3543:16
    frame #20: 0x0000000122840294 LLDB`lldb::SBDebugger::RunCommandInterpreter(this=0x000000016d35ebd8, auto_handle_events=true, spawn_thread=false) at SBDebugger.cpp:1212:42
    frame #21: 0x0000000102aa6d28 lldb`Driver::MainLoop(this=0x000000016d35ebb8) at Driver.cpp:621:18
    frame #22: 0x0000000102aa75b0 lldb`main(argc=1, argv=0x000000016d35f548) at Driver.cpp:829:26
    frame #23: 0x0000000198858274 dyld`start + 2840

Changes in this PR top of the above

Fix a test failure in TestStats.py. The original version of the added test checks that all modules have symbol count zero when target.preload-symbols == false. The test failed on macOS. Due to various reasons, on macOS, symbols can be loaded for dylibs even with that setting, but not for the main module. For now, the fix of the test is to limit the assertion to only the main module. The test now passes on macOS. In the future, when we have a way to control a specific list of plug-ins to be loaded, there may be a configuration that this test can use to assert that all modules have symbol count zero.

Apply a minor renaming of statistics, per the suggestion in #136226 after merge.

Tests

Manually tested with a helloworld program, that:

1. Without this patch, a breakpoint in ObjectFileMachO::ParseSymtab() is hit when we do lldb a.out then statistics dump.
2. With this patch, said breakpoint is not hit.

Unit test:

$ cd ~/public_llvm/build

$ ninja lldb-unit-test-deps
$ tools/lldb/unittests/Symbol/SymbolTests
...
[==========] 85 tests from 16 test suites ran. (120 ms total)
[  PASSED  ] 85 tests.

$ ninja lldb-api-test-deps
$ bin/llvm-lit -sv ../llvm-project/lldb/test/API/commands/statistics/basic/TestStats.py
Total Discovered Tests: 1
  Passed: 1 (100.00%)

[dmpots]

It would be good to update the description to include what you did to fix the test failure (looks like just checking the main module stats).

I think it is also useful to include the original commit description when re-applying a commit since this is the one that will show up most often in a history/blame command and having the full context readily available is useful.

[royitaqi]

Updated the PR:

1. Updated the description according to @dmpots 's suggestion.
2. Renamed the statistics according to @clayborg 's suggestion.

[clayborg]

Looks good. Watch the buildbot, and please run your tests on both macOS and linux prior to merging to ensure we don't fail. I am a bit worried about system runtime plug-ins causing symbols to be loaded on some configs.

[royitaqi]

Ran ninja check-lldb on both macOS and Linux and compared test counts before/after this patch. No regression observed. Merging.

[royitaqi]

Checked bin/llvm-lit -sv ../llvm-project/lldb/test/API/commands/statistics/basic/TestStats.py again on both macOS and Linux after merging with main.