[PATCH] D130062: [lldb][DWARF5] Enable macro evaluation

[Greg Clayton via Phabricator via llvm-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:95-96
 const DWARFDataExtractor &DWARFContext::getOrLoadMacroData() {
-  return LoadOrGetSection(eSectionTypeDWARFDebugMacro, llvm::None,
-                          m_data_debug_macro);
+  return LoadOrGetSection(eSectionTypeDWARFDebugMacro,
+                          eSectionTypeDWARFDebugMacro, m_data_debug_macro);
 }
----------------
This enum should be different otherwise this code change does nothing.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.cpp:22
+  // LLDB doesn't support DWARF64, so we always have item size of 4.
+  uint64_t offset = cu_offset + 4 * index;
+  return data->GetU32(&offset);
----------------
I would make this DWARF64 compatible.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h:28
+struct DWARFStrOffsetsInfo {
+  lldb::offset_t cu_offset;
+  const lldb_private::DWARFDataExtractor *data;
----------------
"cu_offset" sounds like it is actually the offset of the compile unit. This is actually the compile unit offset for the string data for the compile unit in the string index section right? Maybe a name like "cu_str_offset" or "cu_strx_offset" would be a better name?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h:29
+  lldb::offset_t cu_offset;
+  const lldb_private::DWARFDataExtractor *data;
+
----------------
Can "data" ever be NULL? If not, then change this to a reference and add a constructor that inits the reference?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugMacro.h:31
+
+  bool IsValid() const { return cu_offset && cu_offset != DW_INVALID_OFFSET; }
+  uint64_t GetOffset(uint64_t index) const;
----------------
if you are going to keep "data" as a pointer, shouldn't this function also check "data" against nullptr?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1219
 
-  comp_unit.SetDebugMacros(ParseDebugMacros(&sect_offset));
+  DWARFStrOffsetsInfo str_offsets_info = {};
+  str_offsets_info.cu_offset = dwarf_cu->GetStrOffsetsBase();
----------------
If we want to change "data" to be a reference, we will need a constructor:
```
DWARFStrOffsetsInfo str_offsets_info(dwarf_cu->GetStrOffsetsBase(), symfile.GetDWARFContext().getOrLoadStrOffsetsData());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130062