[Lldb-commits] [PATCH] D51935: [LLDB] - Improve reporting source lines and variables (improved DWARF5 support).

[Greg Clayton via Phabricator via lldb-commits]

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

Nice patch! A few minor fixes, see inlined comments.



================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:78
 
+uint32_t DWARFCompileUnit::GetDWARF5HeaderSize(uint8_t unitType) {
+  switch (unitType) {
----------------
Rename to GetHeaderByteSize() and do:

```
uint32_t DWARFCompileUnit::GetHeaderByteSize() {
  if (m_version < 5)
    return m_is_dwarf64 ? 23 : 11;
  switch (m_unit_type) {
    case llvm::dwarf::DW_UT_compile:
    case llvm::dwarf::DW_UT_partial:
      return 12;
    case llvm::dwarf::DW_UT_skeleton:
    case llvm::dwarf::DW_UT_split_compile:
      return 20;
    case llvm::dwarf::DW_UT_type:
    case llvm::dwarf::DW_UT_split_type:
      return 24;
  }
  llvm_unreachable("invalid UnitType.");
}
```




================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:38-42
   uint32_t GetHeaderByteSize() const override {
+    if (m_version >= 5)
+        return GetDWARF5HeaderSize(m_unit_type);
     return m_is_dwarf64 ? 23 : 11;
   }
----------------
Move this to .cpp file and inline contents of GetDWARF5HeaderSize() into the the body of the function.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:45
 private:
+  static uint32_t GetDWARF5HeaderSize(uint8_t unitType);
+
----------------
Remove this


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp:442
+        ReadDescriptors(debug_line_data, offset_ptr);
+    uint8_t dirCount = debug_line_data.GetULEB128(offset_ptr);
+    for (int i = 0; i < dirCount; ++i) {
----------------
"dirCount" is documented in the DWARF 5 spec to be a "ubyte" not a ULEB128


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp:557-558
 
-const char *DWARFFormValue::AsCString() const {
-  SymbolFileDWARF *symbol_file = m_cu->GetSymbolFileDWARF();
-
+const char *DWARFFormValue::AsCString(SymbolFileDWARF *symbol_file,
+                                      bool is64Bit) const {
   if (m_form == DW_FORM_string) {
----------------
Revert signature and get info form m_cu. There are no other calls to AsCString() that manually specify a different symbol file and 64 bit value.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp:613-616
+const char *DWARFFormValue::AsCString() const {
+  return AsCString(m_cu->GetSymbolFileDWARF(), m_cu->IsDWARF64());
+}
+
----------------
Remove this as there are no other calls to AsCString() that manually specify a different symbol file and 64 bit value.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.h:77
   const char *AsCString() const;
+  const char *AsCString(SymbolFileDWARF *Dwarf, bool Is64Bit) const;
   dw_addr_t Address() const;
----------------
This doesn't seem to be needed in this patch? The SymbolFileDWARF and Is64Bit can be grabbed from the m_cu


https://reviews.llvm.org/D51935