[PATCH] D143508: [ELF][llvm-objcopy] Reject duplicate SHT_SYMTAB sections.

[Moshe via Phabricator via llvm-commits]

MosheBerman added a comment.

In D143508#4112290 <https://reviews.llvm.org/D143508#4112290>, @jhenderson wrote:

> Hi @MosheBerman,
>
> - You've uploaded your diff without full context. This can make it harder to review (although in this case it's fine). Please make sure to always upload your diff with full context, as per the instructions on the LLVM website.

Thanks for pointing this out. Can I trouble you for a link to the specific page on the LLVM website, or an example of good context, so I know what to do for next time? (I looked at the Contributing <https://llvm.org/docs/Contributing.html> and Developer Policy <https://llvm.org/docs/DeveloperPolicy.html> pages for "context" and did not see anything.



================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:1707
   case SHT_SYMTAB: {
+    // Duplicate SHT_SYMTAB sections are forbidden by the ELF gABI.
+    if (Obj.SymbolTable != nullptr)
----------------
jhenderson wrote:
> I'd use the term "Multiple" not "Duplicate", because "Duplicate" implies identical, which they don't need to be.
Great point. I struggled with the word choice in the message, and I did use "multiple" there for that exact reason. I'll change it here. Let's also rename the test file to match.


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:1710-1711
+      return createStringError(llvm::errc::invalid_argument,
+                               "found multiple SHT_SYMTAB sections. "
+                               "Currently, an object file may have only one");
     auto &SymTab = Obj.addSection<SymbolTableSection>();
----------------
jhenderson wrote:
> If you wanted to provide the user with more information, about the only information you could give them that would be useful are the section indices of the two symbol tables. However, without further changes, I don't believe this information is currently available here. I don't think it's worth complicating the interface of the function to support it either, as it should be fairly easy for the end user to identify the two (or more!) sections causing the issue using tools like llvm-readelf.
> 
> I think you can slightly simplify this message to "multiple SHT_SYMTAB sections are not supported". There's no need for the "Currently..." part of the message, since that's implied by the fact that we're emitting the error in the first place.
> If you wanted to provide the user with more information, about the only information you could give them that would be useful are the section indices of the two symbol tables. However, without further changes, I don't believe this information is currently available here. 

I experimented with displaying the section names with the `Shdr` parameter and was only able to get the `Name` from the original section. 

>  as it should be fairly easy for the end user to identify the two (or more!) sections causing the issue using tools like llvm-readelf.

How would you feel about mentioning that in the message? 

> There's no need for the "Currently..." part of the message, since that's implied by the fact that we're emitting the error in the first place.

Fair. The reason I included that phrase was because Google searching for that phrase matches the docs and leads right to it. Do you think there's a benefit to doing so, or is the goal brevity?

This is all helpful feedback, my goal was to go beyond "something broke" to "something broke. here's how to fix it." I'll definitely make changes based on your responses to my two questions. (1. Mentioning `llvm-readelf`and 2. Searchable phrase in the output.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143508