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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D143508#4113157 <https://reviews.llvm.org/D143508#4113157>, @MosheBerman wrote:

> 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.~~
>
> Do you mean in the patch itself?

I'm assuming you've been uploading the diffs via the web UI? You need to ensure you've created the diff using something like `git diff -U9999999` (see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface). By default git diffs only have a very limited context available.



================
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>();
----------------
MosheBerman wrote:
> 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.)
I don't think your second sentence is really all that useful. Chances are that if a user is using llvm-objcopy and have an ELF in this weird format, they already know about llvm-readelf and how to inspect objects. They may well also know about the ELF gABI, and it appears on the front page of Google currently if you search using the exact error message.

We should aim for diagnostic messages to be clear and concise. In this case, the additional information you've provided is speculative and to many users potentially unhelpful. Including the section names on the other hand could potentially be useful, since that will help the user identify the offending sections.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/symbol-prohibit-multiple-symtab-sections.test:1-2
+# According to the ELF gABI, "Currently, an object file may have only one 
+# section of each type [SHT_SYMTAB and SHT_DYNSYM], but this restriction may be relaxed in the future.".
+# RUN: yaml2obj %s -o %t
----------------
We use `##` for comments in llvm-objcopy tests, to help them stand out from the real test lines.

The test name can be simplified to "multiple-symtab.test". The current name is unnecessarily verbose, and if the behaviour ever changes, this test could be adapted to cover the new behaviour instead, without needing to rename it. In other words the test name here can just describe the input.


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

https://reviews.llvm.org/D143508