[Lldb-commits] [PATCH] D82477: [lldb-vscode] Add Support for Module Event
Greg Clayton via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Wed Jun 24 12:28:41 PDT 2020
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
So great first patch Yifan! A few things to fix up, see inlined comments.
We need to add a test for lldb::SBTarget::eBroadcastBitSymbolsLoaded if possible. This can be done by stripping "a.out" into "a.out.stripped" and then debugging "a.out.stripped" and then adding symbols using "target symbols add ..." to add the "a.out".
================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:335
+ object.try_emplace("id", std::string(module.GetUUIDString()));
+ object.try_emplace("name", std::string(module.GetFileSpec().GetFilename())); // Path in remote
+ std::string module_path = std::string(module.GetFileSpec().GetDirectory()) + "/" + std::string(module.GetFileSpec().GetFilename());
----------------
LLVM coding guidelines limit source lines to 80 characters. So just reformat as requested here.
================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:340
+ object.try_emplace("symbolStatus", "Symbols loaded.");
+ std::string symbol_path = std::string(module.GetSymbolFileSpec().GetDirectory()) + "/" + std::string(module.GetSymbolFileSpec().GetFilename());
+ object.try_emplace("symbolFilePath", symbol_path);
----------------
Ditto
================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:343
+ }
+ std::string loaded_addr = std::to_string(module.GetObjectFileHeaderAddress().GetLoadAddress(g_vsc.target));
+ object.try_emplace("addressRange", loaded_addr);
----------------
Ditto
================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:346
+ return llvm::json::Value(std::move(object));
+
+}
----------------
remove empty line.
================
Comment at: lldb/tools/lldb-vscode/JSONUtils.h:241
+llvm::json::Value CreateModule(lldb::SBModule &module);
+
----------------
Need to add header documentation like the other functions in this file.
================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:357-359
+ listener.StartListeningForEvents(
+ this->target.GetBroadcaster(),
+ lldb::SBTarget::eBroadcastBitModulesLoaded | lldb::SBTarget::eBroadcastBitModulesUnloaded);
----------------
reformat per pre-merge checks
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:440
+ event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded ||
+ event_mask & lldb::SBTarget::eBroadcastBitSymbolsLoaded) {
+ int solib_count = lldb::SBTarget::GetNumModulesFromEvent(event);
----------------
you check for "lldb::SBTarget::eBroadcastBitSymbolsLoaded" here, but didn't listen for that event in VSCode.cpp. Either remove lldb::SBTarget::eBroadcastBitSymbolsLoaded or add it to the events we want to listen for in VSCode.cpp
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:441
+ event_mask & lldb::SBTarget::eBroadcastBitSymbolsLoaded) {
+ int solib_count = lldb::SBTarget::GetNumModulesFromEvent(event);
+ int i = 0;
----------------
rename "solib_count" to "num_modules"?
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:442-445
+ int i = 0;
+ while (i < solib_count) {
+ auto module = lldb::SBTarget::GetModuleAtIndexFromEvent(i, event);
+ i++;
----------------
Use for loop:
```
for (int i=0; i<num_modules; ++i) {
auto module = lldb::SBTarget::GetModuleAtIndexFromEvent(i, event);
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82477/new/
https://reviews.llvm.org/D82477
More information about the lldb-commits
mailing list