[Lldb-commits] [PATCH] D56595: SymbolFileBreakpad: Add line table support

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So LLDB treats compile units special depending on the inline strategy. Because of this, I outlined some examples of how and why we should create a compile unit per "FUNC" token. Let me know if anything above was unclear



================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:119
 
+uint32_t SymbolFileBreakpad::GetNumCompileUnits() { return 1; }
+
----------------
I would vote to make 1 compile unit for each "FUNC". The hard part will be to select the right source file for each function. I would just start by selecting the first line entry for the function. Compile units are expected to give a list of support files, and in this case, you would make a set of files that are in the line entries only. So if you had:

```
MODULE Linux x86_64 761550E08086333960A9074A9CE2895C0 a.out
INFO CODE_ID E05015768680393360A9074A9CE2895C
FILE 0 /tmp/a.c
FILE 1 /tmp/b.c
FILE 2 /usr/include/foo.h
FUNC b0 10 0 func1
b0 1 1 0
b1 1 2 0
b2 1 2 1
b4 1 3 1
FUNC b0 10 0 func2
b0 1 1 1
b1 1 2 1
b2 1 2 2
```
We would have a compile unit for "func1" with a cu for "/tmp/a.c" and with support files:
support_file[0] = "/tmp/a.c"
support_file[1] = "/usr/include/foo.h"

We would have a compile unit for "func2" with a cu for "/tmp/b.c" and with support files:

support_file[0] = "/tmp/b.c"
support_file[1] = "/usr/include/foo.h"

The main reason for make individual compile units, is LLDB treats a compile unit specially when settings breakpoints depending on if we ask for inline functions to be set.  If we set the following setting:
```
(lldb) settings set target.inline-breakpoint-strategy never
```
Then we will check the name of the compile unit to ensure it matches. So if we did:
```
(lldb) b a.c:12
```
This would only work if the actual lldb_private::CompileUnit has a FileSpec that matches "a.c". 


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:122
 CompUnitSP SymbolFileBreakpad::ParseCompileUnitAtIndex(uint32_t index) {
-  // TODO
-  return nullptr;
+  assert(index == 0);
+  m_obj_file->GetModule()->GetSymbolVendor()->SetCompileUnitAtIndex(
----------------
return the count of the number of "FUNC" objects


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:123-125
+  m_obj_file->GetModule()->GetSymbolVendor()->SetCompileUnitAtIndex(
+      0, m_comp_unit_sp);
+  return m_comp_unit_sp;
----------------
parse a compile unit per function. We might want to cache all "FILE" entries in a list inside the SymbolFileBreakpad so we can easily pull out the FileSpecs when creating each compile unit.

Also, each compile unit's ID can be the line number in the breakpad file to the "FUNC" entry. This allows easy access to each "FUNC" entry in the breakpad file when we are asked to parse more information about it (get compile unit support files, or any parsing of info for a compile unit.


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:158-179
+    llvm::StringRef token_str;
+    std::tie(token_str, line) = getToken(line);
+    if (toToken(token_str) == Token::Func)
+      continue;
+
+    // address size line filenum
+    addr_t address;
----------------
It would be nice to put this parsing code into the lldb_private::breakpad::Line" class I talked about in the other patch?

It would be great if this code looked like:

```
lldb_private::breakpad::Line bp_line(line);
switch (bp_line.GetToken()) {
  case Token::Func:
    // Create the compile unit and store into cu_sp
    cu_sp = bp_line.CreateCompileUnit();
    break;
  case Token::Line: {
    addr_t address;
    size_t size;
    uint32_t line_num, file_num;
    if (bp_line.ParseLineEntry(address, size, line_num, file_num)) {
      // Discontiguous entries. Finish off the previous sequence and reset.
      if (next_addr && *next_addr != address)
        finish_sequence();
      line_table_up->AppendLineEntryToSequence(
          line_seq_up.get(), address, line_num, 0, file_num, true, false, false, false,  false);
    }
```    


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:199
+
+bool SymbolFileBreakpad::ParseCompileUnitSupportFiles(
+    const SymbolContext &sc, FileSpecList &support_files) {
----------------
This function will need to populate support_files for a given FUNC as mentioned above


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:234
+  if (resolve_scope & eSymbolContextLineEntry) {
+    if (sc.comp_unit->GetLineTable()->FindLineEntryByAddress(so_addr,
+                                                             sc.line_entry)) {
----------------
We need to search each compile unit to see which compile unit contains the address now.


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h:99
   bool CompleteType(CompilerType &compiler_type) override { return false; }
+
   uint32_t ResolveSymbolContext(const Address &so_addr,
----------------
Remove whitespace only change


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

https://reviews.llvm.org/D56595