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

[Greg Clayton via Phabricator via cfe-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

See inline comments. Changes needed are:

- test for eBroadcastBitSymbolsLoaded
- add "version" to module info
- test all data we expect in module info ('name', 'symbolFilePath', 'version', 'symbolStatus', 'addressRange')



================
Comment at: lldb/test/API/tools/lldb-vscode/module/Makefile:3
+
+include Makefile.rules
----------------
We need to test eBroadcastBitSymbolsLoaded in this test. I would suggest we make an "a.out.stripped"  (see the makefile form lldb/test/API/lang/objc/objc-ivar-stripped/Makefile for how to do this).


================
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:20
+    def test_modules_event(self):
+        program= self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
----------------
eBroadcastBitSymbolsLoaded needs to be tested: change this line to:
```
program_basename = 'a.out.stripped'
program = self.getBuildArtifact(program_basename)
```
after modifying the Makefile as suggested above.



================
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:27
+        self.continue_to_breakpoints(breakpoint_ids)
+        self.assertTrue('a.out' in self.vscode.get_active_modules(), 
+                        'Module: a.out is loaded')
----------------
Cached active modules, and use assertIn here and switch to "a.out.stripped", and verify the 'name' is in module info and that path is correct:
```
active_modules = self.vscode.get_active_modules()
self.assertIn(program_basename, active_modules, '%s module is in active modules' % (program_basename))
program_module = active_modules[program_basename]
self.assertIn('name', program_module, 'make sure 'path' key is in module')
self.assertEqual(program, program_module['name'])
```


================
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:29-30
+                        'Module: a.out is loaded')
+        self.assertTrue('symbolFilePath' in self.vscode.get_active_modules()['a.out'],
+                        'Symbol exists')
----------------
use self.assertIn(...) and use the cached "active_modules" variable:
```
self.assertIn('symbolFilePath', program_module, 'Make sure 'symbolFilePath' key is in module info')
self.assertEqual(program, program_module['symbolFilePath'])
```
Then you need to verify all other information that you put into the Module like "symbolStatus" and "addressRange".



================
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:29-30
+                        'Module: a.out is loaded')
+        self.assertTrue('symbolFilePath' in self.vscode.get_active_modules()['a.out'],
+                        'Symbol exists')
----------------
clayborg wrote:
> use self.assertIn(...) and use the cached "active_modules" variable:
> ```
> self.assertIn('symbolFilePath', program_module, 'Make sure 'symbolFilePath' key is in module info')
> self.assertEqual(program, program_module['symbolFilePath'])
> ```
> Then you need to verify all other information that you put into the Module like "symbolStatus" and "addressRange".
> 
Now we would send a LLDB command line command to load the "a.out" symbols:

```
symbols = self.getBuildArtifact("a.out")
response = self.vscode.request_evaluate('`%s' % ('target symbols add -s "%s" "%s"' % (program, symbols)))
```
This will cause LLDB to load the symbol file "a.out" for the executable "a.out.stripped". This should cause the eBroadcastBitSymbolsLoaded notification to fire and the modules to get updated. Then you will get the active modules again and verify:
- program is still "program" (the full path to the a.out.stripped binary)
- 'symbolFilePath' is equal to our "symbols" variable from above (full path to "a.out")
- 'symbolStatus' is now set to 'Symbols Loaded.'
- 'addressRange' is still what we expect



================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:344-345
+    module.GetSymbolFileSpec().GetPath(symbol_path_arr, sizeof(symbol_path_arr));
+    std::string symbol_path(symbol_path_arr);
+    object.try_emplace("symbolFilePath", symbol_path);
+  }
----------------
Do we only want to try and add this "symbolFilePath" key if the path differs from "module_path"?


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:351
+  return llvm::json::Value(std::move(object));
+}
+
----------------
We should also fetch the version number from the module and populate the "version" key. To get the version number from a module you can use:

```
uint32_t version_nums[5];
const uint32_t num_versions = module.GetVersion(version_nums, sizeof(version_nums));
std::string version_str;
for (uint32_t i=0; i<num_versions; ++i) {
  if (!version_str.empty())
    version_str += ".";
  version_str += std::to_string(version_nums[i])
}
if (!version_str.empty()) 
  object.try_emplace("version", version_str);
```

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82477