[PATCH] D59496: [llvm-objcopy] - Fix a st_name of the first symbol table entry.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lib/MC/StringTableBuilder.cpp:163
+
+  // Empty strings in according to ELF specification should have zero indexes.
+  // In initSize() we explicitly reserved a null byte at the start for that.
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > Whilst I think the change is good, I can't find anything in the specification that requires empty strings to have index zero, only that the first byte must be an empty string. I think this comment needs to be updated to say something about "the first byte must always be an empty string" etc.
> > What about something like the following?
> > 
> > "The first byte in ELF string table must be null. In according to ELF specification a string whose index is zero specifies either
> > no name or a null name, depending on the context. In 'initSize()' we reserve the first byte to hold null for that and here we use it.
> > That also allows 'getOffset()' called for any empty string to always return a fixed zero offset."
> I don't think we need to be that complicated. How about this:
> 
> "The first byte in an ELF string table must be null, according to the ELF specification. In 'initSize()' we reserved the first byte to hold null for this purpose and here we actually add the string to allow 'getOffset()' to be called on an empty string."
Looks good, thanks!


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

https://reviews.llvm.org/D59496