[Lldb-commits] [PATCH] D83881: [lldb/COFF] Remove strtab zeroing hack

Thu Jul 16 11:46:07 PDT 2020

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