mstorsjo marked an inline comment as done.
mstorsjo added inline comments.
================
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:
> > mstorsjo wrote:
> > > 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.
> > `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`?
> 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...
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70848/new/
https://reviews.llvm.org/D70848
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
