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

[Yifan Shen via Phabricator via lldb-commits]

aelitashen marked 3 inline comments as done.
aelitashen added inline comments.


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:337-340
+    std::string(module.GetFileSpec().GetFilename()));
+  std::string module_path = std::string(module.GetFileSpec().GetDirectory()) +
+                            "/" +
+                            std::string(module.GetFileSpec().GetFilename());
----------------
clayborg wrote:
> Let LLDB take care of the directory delimiter. Otherwise this will be bad on windows:
> ```
> char module_path[PATH_MAX];
> module.GetFileSpec().GetPath(module_path, sizeof(module_path));
> ```
> 
This piece of code will make path unreadable in the log, and cause the modules view cannot be displayed properly.


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:344-346
+    std::string symbol_path =
+        std::string(module.GetSymbolFileSpec().GetDirectory()) + "/" +
+        std::string(module.GetSymbolFileSpec().GetFilename());
----------------
clayborg wrote:
> Let LLDB take care of the directory delimiter. Otherwise this will be bad on windows:
> 
> ```
> char symbol_path[PATH_MAX];
> module.GetSymbolFileSpec().GetPath(module_path, sizeof(symbol_path));
> ```
> 
Same reason as above.


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:349-350
+  }
+  std::string loaded_addr = std::to_string(
+      module.GetObjectFileHeaderAddress().GetLoadAddress(g_vsc.target));
+  object.try_emplace("addressRange", loaded_addr);
----------------
clayborg wrote:
> We need to make sure the address returned is valid and we want the string value to be in hex. Are we sure std::to_string() will create a hex number? If not we can use sprintf:
> 
> ```
> uint64_t load_addr = module.GetObjectFileHeaderAddress().GetLoadAddress(g_vsc.target);
> if (load_addr != LLDB_INVALID_ADDRESS) {
>   char load_addr_str[64];
>   snprintf(load_addr_str, sizeof(load_addr_str), "0x%016" PRIx64, load_addr);
>   object.try_emplace("addressRange", load_addr_str);
> }
> ```
Decimal to Hex conversion is done in the IDE, not sure if we need to move it here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82477/new/

https://reviews.llvm.org/D82477