amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.
Thanks @mstorsjo for clarifying for me.
================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:642
DataExtractor symtab_data =
ReadImageData(m_coff_header.symoff, symbol_data_size + 4);
lldb::offset_t offset = symbol_data_size;
----------------
mstorsjo wrote:
> amccarth wrote:
> > The `+4` at the end of this expression is from the same patch. I wonder if
> > it was an attempt to make space for the four bytes of zeros at offset 0
> > that you're eliminating?
> >
> > I suggest removing the `+4` and trying the tests again unless it's obvious
> > to you why it's still necessary. The comment above seems like it might be
> > trying to explain it, but that comment came later.
> No, the `+4` here was present before 0076e71592a6a (if viewing that commit,
> view it with a bit more than the default git context size and you'll find "//
> Include the 4 bytes string table size at the end of the symbols" existing
> already before that.
>
> The +4 here can't be eliminated; without it, the `const uint32_t strtab_size
> = symtab_data.GetU32(&offset)` two lines below would be out of range. So this
> first reads the symbol table and the 4 byte size of the string table, and if
> the string table turns out to be nonzero in size, it also loads a separate
> DataExtractor with the string table contents, with the two DataExtractors
> overlapping for that size field. It doesn't have anything to do with
> overwriting the start, just with the code layout in general.
Sorry, I mixed up `strtab_data` and `symtab_data` when comparing to the old
patch, which is why I didn't see the comment where I expected it.
The old patch actually _removed_ a `+4` when computing the offset for
`strtab_data`. It seemed weird this patch didn't have to restore that in order
to back out this part of the change. But I think I understand it now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83881/new/
https://reviews.llvm.org/D83881
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
