[PATCH] D69526: [llvm-objcopy] Address post-commit reviews of D69093

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I think this part of one of my comments may have been missed, as I don't see a response to it anywhere:

> Is there anything stopping the section header string table having the SHF_ALLOC bit set however? If not, we need a separate test for this case, I believe.

I had another thought about this, and I wonder if we might mess things up if we try to change the section names when the section header string table has SHF_ALLOC set. But this seems unlikely, so is probably a non-issue.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test:35
+# RUN: llvm-objcopy --add-symbol='abs1=1' %t.no %t.add.symtab
+# RUN: llvm-readobj -S %t.add.symtab | FileCheck --check-prefix=SEC2 %s
+# RUN: llvm-readelf -s %t.add.symtab | FileCheck %s
----------------
Do you think this check should also have an `--implicit-check-not=.strtab`?


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-symbol-no-symtab.test:69-73
+Sections:
+  - Name: .shstrtab
+    Type: SHT_STRTAB
+  - Name: .strtab
+    Type: SHT_STRTAB
----------------
This may be me being forgetful, but I don't follow how this YAML will generate anything different from the previous YAML doc.

Also, the comment for this test case says that it is checking that we prefer the string table that is not the section header table, but the test itself checks that we add it to the section header string table when the string table is not there. We need both test cases, but I'm under the impression that the latter is already covered by the previous test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69526