[LLDB] Set the right address size on output DataExtractors from ObjectFile

[mstorsjo]

Summary

If filling in a DataExtractor from an ObjectFile, e.g. via the ReadSectionData method, the output DataExtractor gets the address size from the m_data member.

ObjectFile's m_data member is initialized without knowledge about the address size (so the address size is set based on the host's sizeof(void*), and at that point within ObjectFile's constructor, virtual methods implemented in subclasses (like GetAddressByteSize()) can't be called, therefore fix it up when filling in external DataExtractors.

This makes sure that line tables from executables with a different address size are parsed properly; previously this tripped up DWARFDebugLine::LineTable::parse for 32 bit executables on a 64 bit host, as the address size in the line table (4) didn't match the one set in the DWARFDataExtractor.

[clayborg]

lldb/source/Symbol/ObjectFile.cpp

480–486

I would vote to make this happen within DataExtractor::SetData(const DataExtractor &...)

[mstorsjo]

lldb/source/Symbol/ObjectFile.cpp

480–486

Do you mean that we'd extend DataExtractor::SetData(const DataExtractor &...) to take a byte address size parameter, or that we'd update m_data's byte address size before doing data.SetData() here?

Ideally I'd set the right byte address size in m_data as soon as it is known and available. It's not possible to do this in ObjectFile's constructor, as that is called before the subclass is constructed and its virtual methods are available, but is there a better point in the lifetime where we could update it?

[labath]

I just realized I didn't press "sumbit" yesterday...

lldb/source/Symbol/ObjectFile.cpp

480–486

I too would prefer that, but I couldn't see a way to achieve that (which is why I stamped this version).

Today, with a fresh set of eyes, I think it may be reasonable to have this happen in ObjectFile::ParseHeader. After the header has been parsed, all object formats (I hope) should be able to determine their address size and endianness, and the operating invariant would be that the address size is only valid after the ParseHeader has been called. WDYT?

[labath]

BTW, I don't know if you've noticed, but the new test is failing on windows: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/11217. It looks like a path separator problem. We actually already have code which tries to guess the path style of a compile unit, but it is keyed off of the DW_AT_comp_dir attribute, which it looks like you stripped from the test case. My guess is that adding this attribute back will fix this problem...

[mstorsjo]

In D70848#1768730, @labath wrote:

BTW, I don't know if you've noticed, but the new test is failing on windows: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/11217. It looks like a path separator problem. We actually already have code which tries to guess the path style of a compile unit, but it is keyed off of the DW_AT_comp_dir attribute, which it looks like you stripped from the test case. My guess is that adding this attribute back will fix this problem...

I did notice it, and tried to fix it (https://reviews.llvm.org/rG7d019d1a3be252a885e8db1ee7af11c90), but I forgot to check that it actually worked - sorry about that. Apparently I got the backslash escaping wrong (from some clang test). I'll see if DW_AT_comp_dir helps (and remove that failed regex fix), or fix the backslash matching.

lldb/source/Symbol/ObjectFile.cpp

480–486

Sounds sensible! I'll give it a shot.

[mstorsjo]

lldb/source/Symbol/ObjectFile.cpp

480–486

ObjectFile::ParseHeader is currently pure virtual, and thus the subclass concrete implementations of it don't call the base class version of the method. Do you want me to make the base class method non-pure and add calls to it in the subclasses, or add calls to m_data.SetAddressByteSize(GetAddressByteSize()); in all of the subclasses ParseHeader?

[labath]

lldb/source/Symbol/ObjectFile.cpp

480–486

Yeah.. I don't know... I am starting to wonder if this really is a good idea.. From the looks of it, there doesn't seem to be anyone (outside of the object file classes themselves) who is calling this method, so it's not clear to me why is it even present on the base class...

Overall, the scheme that I think I'd like the most would be something like:

class ObjectFile {
  /*nonvirtual*/ uint32_t GetAddressByteSize() { ParseHeader(); return m_data.GetAddressByteSize(); }
  // same for byte order
  
  /*nonvirtual*/ void ParseHeader() { std::tie(address_size, byte_order) = DoParseHeader(); m_data.SetAddressByteSize(address_size); m_data.SetByteOrder(byte_order); }
  virtual ??? DoParseHeader() = 0;
};

However, I don't know how hard would it be to get to that point, so I think I'd settle for just making sure that each subclass calls m_data.SetByteOrder internally. It looks like ObjectFileELF does that already. I haven't checked, but I wouldn't be surprised if MachO does the same...

[mstorsjo]

lldb/source/Symbol/ObjectFile.cpp

480–486

I checked, and both MachO and ELF seem to be calling both m_data.SetByteOrder() and m_data.SetAddressByteSize(), while PECOFF only calls m_data.SetByteOrder(). So that makes the fix rather straightforward.

lldb/source/Symbol/ObjectFile.cpp

DataBufferSP ObjectFile::ReadMemory(const ProcessSP &process_sp,
   }
   return data_sp;
 }
 
 size_t ObjectFile::GetData(lldb::offset_t offset, size_t length,
                            DataExtractor &data) const {
   // The entire file has already been mmap'ed into m_data, so just copy from
   // there as the back mmap buffer will be shared with shared pointers.
-  return data.SetData(m_data, offset, length);
+  size_t ret = data.SetData(m_data, offset, length);
+  // DataExtractor::SetData copies the address byte size from m_data, but
+  // m_data's address byte size is only set from sizeof(void*), and we can't
+  // access subclasses GetAddressByteSize() when setting up m_data in the
+  // constructor.
+  data.SetAddressByteSize(GetAddressByteSize());
+  return ret;
 }
 
 size_t ObjectFile::CopyData(lldb::offset_t offset, size_t length,
                             void *dst) const {
   // The entire file has already been mmap'ed into m_data, so just copy from
   // there Note that the data remains in target byte order.
   return m_data.CopyData(offset, length, dst);
 }

lldb/test/Shell/SymbolFile/DWARF/win-i386-line-table.s

+# Test that lldb can read a line table for an architecture with a different
+# address size than the one that of the host.
+
+# REQUIRES: lld, x86
+
+# RUN: llvm-mc -triple i686-windows-gnu %s -filetype=obj > %t.o
+# RUN: lld-link %t.o -out:%t.exe -debug:dwarf -entry:entry -subsystem:console -lldmingw
+# RUN: %lldb %t.exe -o "image dump line-table -v win-i386-line-table.c" -b | FileCheck %s
+
+# CHECK: Line table for win-i386-line-table.c in `win-i386-line-table.s.tmp.exe
+# CHECK: 0x00401000: /path/to/src/win-i386-line-table.c:2:1
+# CHECK: 0x00401001: /path/to/src/win-i386-line-table.c:2:1
+
+        .text
+        .file   "win-i386-line-table.c"
+        .globl  _entry                  # -- Begin function entry
+_entry:                                 # @entry
+        .file   1 "/path/to/src" "win-i386-line-table.c"
+        .loc    1 1 0                   # win-i386-line-table.c:1:0
+        .cfi_sections .debug_frame
+        .cfi_startproc
+        .loc    1 2 1 prologue_end      # win-i386-line-table.c:2:1
+        retl
+        .cfi_endproc
+                                        # -- End function
+        .section        .debug_str,"dr"
+Linfo_string1:
+        .asciz  "win-i386-line-table.c"
+        .section        .debug_abbrev,"dr"
+Lsection_abbrev:
+        .byte   1                       # Abbreviation Code
+        .byte   17                      # DW_TAG_compile_unit
+        .byte   1                       # DW_CHILDREN_yes
+        .byte   3                       # DW_AT_name
+        .byte   14                      # DW_FORM_strp
+        .byte   16                      # DW_AT_stmt_list
+        .byte   23                      # DW_FORM_sec_offset
+        .byte   0                       # EOM(1)
+        .byte   0                       # EOM(2)
+        .byte   0                       # EOM(3)
+        .section        .debug_info,"dr"
+Lsection_info:
+Lcu_begin0:
+        .long   Ldebug_info_end0-Ldebug_info_start0 # Length of Unit
+Ldebug_info_start0:
+        .short  4                       # DWARF version number
+        .secrel32       Lsection_abbrev # Offset Into Abbrev. Section
+        .byte   4                       # Address Size (in bytes)
+        .byte   1                       # Abbrev [1] 0xb:0x2d DW_TAG_compile_unit
+        .secrel32       Linfo_string1   # DW_AT_name
+        .secrel32       Lline_table_start0 # DW_AT_stmt_list
+        .byte   0                       # End Of Children Mark
+Ldebug_info_end0:
+        .section        .debug_line,"dr"
+Lline_table_start0: