[Lldb-commits] [PATCH] D72597: [lldb][DWARF] Added support for new forms in DWARFv5 macro.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.cpp:62
+    const DWARFDataExtractor &debug_str_offset_data,
     const DWARFDataExtractor &debug_str_data, const bool offset_is_64_bit,
     lldb::offset_t *offset, SymbolFileDWARF *sym_file_dwarf,
----------------
We could also remove "offset_is_64_bit" if we fix DataExtractor to handle it as suggested below where "offset_is_64_bit" is used.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.cpp:128-131
       if (offset_is_64_bit)
         new_offset = debug_macro_data.GetU64(offset);
       else
         new_offset = debug_macro_data.GetU32(offset);
----------------
I have though about adding support to DataExtractor for extracting offsets. Anyone that initializes a DataExtractor would need to set the offset size once, and then everyone else could just call:

```
new_offset = debug_macro_data.GetOffset(offset);
```

We would need to add a field to DataExtractor:

```
  Optional<uint32_t> m_offset_size; ///< The address size to use when extracting offsets. Defaults to m_addr_size if this has no value
```

It would default to the m_addr_size if m_offset_size isn't set. The "m_context.getOrLoadXXXX()" calls would then set the offset size correctly for the current DWARF when it creates the DWARFDataExtractor. 


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h:54-57
+      const lldb_private::DWARFDataExtractor &debug_macro_data,
+      const lldb_private::DWARFDataExtractor &debug_str_offset_data,
+      const lldb_private::DWARFDataExtractor &debug_str_data,
+      const bool offset_is_64_bit, lldb::offset_t *sect_offset,
----------------
To future proof this a bit should we pass in the "lldb_private::DWARFContext" instead of the first three arguments?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1056-1057
 
   const DWARFDebugMacroHeader &header =
       DWARFDebugMacroHeader::ParseHeader(debug_macro_data, offset);
   DWARFDebugMacroEntry::ReadMacroEntries(
----------------
Move this into DWARFDebugMacroEntry::ReadMacroEntries() so that we don't have to pass "header" as an argument?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72597