[Lldb-commits] [PATCH] D83731: Add Debug Info Size to Symbol Status

[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 you should revert any clang format changes that are not in modified lines of source, mostly revert a lot of lines in lldb-vscode.cpp.



================
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:19
 
-
-    @skipIfWindows
-    @skipUnlessDarwin
-    @skipIfRemote
-    def test_modules_event(self):
-        program_basename = "a.out.stripped"
+    def get_modules(self, program_basename):
         program= self.getBuildArtifact(program_basename)
----------------
Name should be more descriptive. Maybe "setup_test_common" is a better name


================
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:19
 
-
-    @skipIfWindows
-    @skipUnlessDarwin
-    @skipIfRemote
-    def test_modules_event(self):
-        program_basename = "a.out.stripped"
+    def get_modules(self, program_basename):
         program= self.getBuildArtifact(program_basename)
----------------
clayborg wrote:
> Name should be more descriptive. Maybe "setup_test_common" is a better name
So all of these tests can re-use this function if we switch it up a bit. Here is what I was thinking:

```
def run_test(self, symbol_basename, expect_debug_info_size):
    program_basename = "a.out.stripped"
    program = self.getBuildArtifact(program_basename)
    self.build_and_launch(program)
    functions = ['foo']
    breakpoint_ids = self.set_function_breakpoints(functions)
    self.assertEquals(len(breakpoint_ids), len(functions), 'expect one breakpoint')
    self.continue_to_breakpoints(breakpoint_ids)
    active_modules = self.vscode.get_active_modules()
    program_module = active_modules[program_basename]
    self.assertIn(program_basename, active_modules, '%s module is in active modules' % (program_basename))
    self.assertIn('name', program_module, 'make sure name is in module')
    self.assertEqual(program_basename, program_module['name'])
    self.assertIn('path', program_module, 'make sure path is in module')
    self.assertEqual(program, program_module['path'])
    self.assertTrue('symbolFilePath' not in program_module, 'Make sure a.out.stripped has no debug info')
    self.assertEqual('Symbols not found.', program_module['symbolStatus'])
    symbols_path = self.getBuildArtifact(symbol_basename)
    self.vscode.request_evaluate('`%s' % ('target symbols add -s "%s" "%s"' % (program, symbols_path)))

    def checkSymbolsLoaded():
        active_modules = self.vscode.get_active_modules()
        program_module = active_modules[program_basename]
        return 'Symbols loaded.' == program_module['symbolStatus']

    def checkSymbolsLoadedWithSize():
        active_modules = self.vscode.get_active_modules()
        program_module = active_modules[program_basename]
        symbolsStatus = program_module['symbolStatus']
        symbol_regex = re.compile(r"Symbols loaded. \([0-9]+(\.[0-9]*)?[KMG]?B\)")
        return symbol_regex.match(program_module['symbolStatus'])
            
    if expect_debug_info_size:
        self.waitUntil(checkSymbolsLoadedWithSize)
    else:
        self.waitUntil(checkSymbolsLoaded)
```

Then your tests would be:
```
@skipIfWindows
@skipIfRemote    
def test_module_event(self):
    # Mac or linux.

    # On mac, if we load a.out as our symbol file, we will use DWARF with .o files and we will
    # have debug symbols, but we won't see any debug info size because all of the DWARF
    # sections are in .o files.

    # On other platforms, we expect a.out to have debug info, so we will expect a size.
    expect_debug_info_size = platform.system() != 'Darwin'
    return run_test("a.out", expect_debug_info_size)

@skipIfWindows
@skipUnlessDarwin
@skipIfRemote    
def test_module_event_dsym(self):
    # Darwin only test with dSYM file.

    # On mac, if we load a.out.dSYM as our symbol file, we will have debug symbols and we
    # will have DWARF sections added to the module, so we will expect a size.
    return run_test("a.out.dSYM", True)
```
This should cover both mac and non windows correctly.


================
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:20
+    def get_modules(self, program_basename):
         program= self.getBuildArtifact(program_basename)
         self.build_and_launch(program)
----------------
add a space after program:

```
program = self.getBuildArtifact(program_basename)
```


================
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:32
+    @skipIfWindows
+    @skipUnlessDarwin
+    @skipIfRemote    
----------------
Remove @skipUnlessDarwin here. This should work on linux.


================
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:59
+        # it will end up using the executable and the .o files as the debug info.
+        # We won't see any debug information size for this case since all of the debug info sections are in the .o files.
+        self.assertEqual('Symbols loaded.', program_module['symbolStatus'])    
----------------
wrap comment to 80 chars


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:335
+    debug_info_size += section.GetFileByteSize();
+  if (section_name.startswith(".apple") || section_name.startswith("__apple"))
+    debug_info_size += section.GetFileByteSize();
----------------
yes, merge the if statements


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83731