labath 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;
----------------
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...
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
