[Lldb-commits] [PATCH] D82477: [lldb-vscode] Add Support for Module Event

[Greg Clayton via Phabricator via lldb-commits]

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