[Lldb-commits] [PATCH] D70848: [LLDB] Set the right address size on output DataExtractors from ObjectFile

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo marked an inline comment as done.
mstorsjo added a comment.

In D70848#1768730 <https://reviews.llvm.org/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.



================
Comment at: lldb/source/Symbol/ObjectFile.cpp:480-486
+  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;
----------------
labath wrote:
> mstorsjo wrote:
> > clayborg wrote:
> > > I would vote to make this happen within DataExtractor::SetData(const DataExtractor &...)
> > 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?
> 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?
Sounds sensible! I'll give it a shot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70848