[PATCH] D69093: [llvm-objcopy] --add-symbol: fix crash if SHT_SYMTAB does not exist

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1547
+    for (auto &Sec : Obj.sections()) {
+      if (Sec.Type == ELF::SHT_STRTAB && !(Sec.Flags & SHF_ALLOC)) {
+        StrTab = static_cast<StringTableSection *>(&Sec);
----------------
rupprecht wrote:
> Why do we need to check `SHF_ALLOC`?
.dynstr (SHT_STRTAB) has the SHF_ALLOC flag. We can't reuse .dynstr for .symtab (non-SHF_ALLOC).

The case is tested by `# SEC3:` in add-symbol-no-symtab.test


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1550-1552
+        // Prefer .strtab to .shstrtab.
+        if (Obj.SectionNames != &Sec)
+          break;
----------------
rupprecht wrote:
> Should `.dynstr` also be avoided? Is it better to just check that the name is literally `.strtab`?
.dynstr is avoided by `!(Sec.Flags & SHF_ALLOC)`. `.strtab` works as well but `Obj.SectionNames != &Sec` is more general. There are multiple ways to accomplish the samething. I just wanted to make the code less magical.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1546
+    StringTableSection *StrTab = nullptr;
+    for (auto &Sec : Obj.sections())
+      if (Sec.Type == ELF::SHT_STRTAB) {
----------------
grimar wrote:
> nit: I think you need to add curly bracers to wrap the 'for' body.
> 
> Perhaps something like the following?
> ```
> for (auto &Sec : Obj.sections()) {
>   if (Sec.Type == ELF::SHT_STRTAB)
>     continue:
>   StrTab = static_cast<StringTableSection *>(&Sec);
>   // Prefer .strtab to .shstrtab.
>   if (Obj.SectionNames != &Sec)
>     break;
> }
> ```
Added the brace, but I will not the early-return pattern here. The body is short and I think it might be clearer as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69093