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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
