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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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
----------------
jhenderson wrote:
> 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.
> This may be me being forgetful, but I don't follow how this YAML will generate anything different from the previous YAML doc.

Previous YAML does not specify sections. `.strtab` will be created implicitly before `.shstrtab`:

```
  std::vector<StringRef> ImplicitSections;
  if (Doc.Symbols)
    ImplicitSections.push_back(".symtab");
  ImplicitSections.insert(ImplicitSections.end(), {".strtab", ".shstrtab"});

  if (!Doc.DynamicSymbols.empty())
    ImplicitSections.insert(ImplicitSections.end(), {".dynsym", ".dynstr"});

  // Insert placeholders for implicit sections that are not
  // defined explicitly in YAML.
  for (StringRef SecName : ImplicitSections) {
    if (DocSections.count(SecName))
      continue;

    std::unique_ptr<ELFYAML::Section> Sec = std::make_unique<ELFYAML::Section>(
        ELFYAML::Section::SectionKind::RawContent, true /*IsImplicit*/);
    Sec->Name = SecName;
    Doc.Sections.push_back(std::move(Sec));
  }
```

This YAML specifies `.shstrtab` and `strtab`. These are not created implicitly and yaml2obj hence will follow the order requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69526