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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.

> Do we want to assert here instead if an if check? (I looked at the relevant parts of the LLVM Coding Standards for guidance on formatting the logic checking, and found "assert liberally" section the but didn't see anything else explicit about this kind of check. )

- Asserts are for programmer errors within LLVM. They are not for end-user facing errors. Another way you could think about it is a user could somehow craft an input that hits this check, it isn't an assert.

> Is there anything we can add to the error to guide the reader to unblock themselves? (I included the quote from the ELF gABI docs in the error message to make it more searchable online.)

- We can't know how a user created the input, therefore it is impossible to give them more guidance than "don't have multiple symbol tables in your input" (which is essentially what the error says).



================
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)
----------------
I'd use the term "Multiple" not "Duplicate", because "Duplicate" implies identical, which they don't need to be.


================
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>();
----------------
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.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/symtab-duplicate.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: not llvm-objcopy -O binary %t - 2>&1 | FileCheck %s --check-prefix=SYMTAB
----------------
It might be useful to add a comment to the top of this test file with the gABI quote in, in case anybody runs into this behaviour and wonders about it. This also gives the test a little more context.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/symtab-duplicate.test:2-3
+# RUN: yaml2obj %s -o %t
+# RUN: not llvm-objcopy -O binary %t - 2>&1 | FileCheck %s --check-prefix=SYMTAB
+# SYMTAB: found multiple SHT_SYMTAB sections. Currently, an object file may have only one
+
----------------
These lines can be simplified as per the inline edit.

I'd also add a blank line between the RUN and CHECK lines, as indicated, and I'd check the full error message that llvm-objcopy produces.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/symtab-duplicate.test:9
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
----------------
ET_DYN should probably be ET_REL, although it doesn't really matter (ET_DYN files usually have dynamic symbols etc).


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/symtab-duplicate.test:10
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
----------------
I believe the `Machine` line is unnecessary these days, as a default value is provided that should be sufficient (I could be getting mixed up with something else though, so if an error starts to be generated after removing it, put it back in!).


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/symtab-duplicate.test:14
+    Type: SHT_SYMTAB
+    Flags: [ SHF_ALLOC ]
+  - Name: .symtab2
----------------
Symbol tables generally don't have the SHF_ALLOC flag, so you can delete this and the equivalent line below.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/symtab-duplicate.test:18-19
+    Flags: [ SHF_ALLOC ]
+    
\ No newline at end of file

----------------
Looks like there's trailing whitespace on the final line, hence the "No newline at end of file" message, even though there is a blank line here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143508