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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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.
----------------
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."


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

https://reviews.llvm.org/D59496