[PATCH] D81005: [yaml2obj] - Add a way to exclude specified sections from the section header.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:131
 
+  llvm::SetVector<StringRef> ExcludedSectionHeaders;
+
----------------
jhenderson wrote:
> I might have missed something, but what's the benefit of making this a `SetVector` rather than some form of `unordered_map`?
You're right. I've experimented with different ways to implement this patch and at some point wanted to iterate over headers in an insertion order using this variable.
(I've tried to make it a member of `SectionHeaderTable` I think). There is no need to require it to be `vector` with the current code.

I can't use `srd::unordered_set`, because it wants a `std::hash` function for `StringRef` which we do not have I think:
"error C2280: 'std::hash<_Kty>::hash(const std::hash<_Kty> &)': attempting to reference a deleted function"

I've used `StringSet<>`.




================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:523
+uint64_t ELFState<ELFT>::getSectionNameOffset(StringRef Name) {
+  // If a section is excluded from section headers, we do not save it's name in
+  // the string table.
----------------
jhenderson wrote:
> it's -> its
> 
> (it's == it is, its == belonging to it)
Thanks!


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:13-16
+# INCLUDE-SEC:      [Nr] Name      Type     Address          Off    Size   ES Flg Lk Inf Al
+# INCLUDE-SEC:      [ 1] [[SEC]]   PROGBITS 0000000000000000 000040 000000 00 0   0  0
+# INCLUDE-SEC-NEXT: [ 2] .strtab   STRTAB   0000000000000000 000040 000001 00 0   0  1
+# INCLUDE-SEC-NEXT: [ 3] .shstrtab STRTAB   0000000000000000 000041 000018 00 0   0  1
----------------
jhenderson wrote:
> Can we get rid of the ES and onwards columns? I'm not sure they add much value (but could be persuaded otherwise).
I've removed all columns except the "Name" one.


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:77
+## Check we report an error when the `Excluded` key is used, but an empty
+## section header is requested.
+# RUN: not yaml2obj %s --docnum=3 -o /dev/null 2>&1 | FileCheck %s --check-prefix=NO-SECTIONS
----------------
jhenderson wrote:
> section header -> section header table (?)
> 
> Assuming I've understood this correctly, why should this be an error? Perhaps a user wants to omit ALL section headers except the implicit null one?
Hmm. Yes, I've missed this case. Fixed.


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:113-114
+# RUN:   FileCheck %s --check-prefix=LINK -DSEC=.foo -DTARGET=.bar
+# RUN: not yaml2obj %s --docnum=5 -DINCLUDED=.bar -DEXCLUDED=.foo -o /dev/null 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=LINK -DSEC=.bar -DTARGET=.foo
+
----------------
jhenderson wrote:
> Do we really need both cases? It seems unnecessary to me.
Probably we can go with only one. Done.


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:139
+
+## Case B.1: check we report an error when a symbol table section has the Link field which
+##           points to an excluded section.
----------------
jhenderson wrote:
> We probably want a SHT_DYNSYM version of this same case.
Ok. Though the related code is in the `initSymtabSectionHeader()`, and it is shared for both cases.


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:215
+
+## Case C: check we report an error when a debug section has the Link field which
+##         points to an excluded section.
----------------
jhenderson wrote:
> What's the purpose of the debug section case?
To test the code I've changed in `initDWARFSectionHeader()`.
It is the same (i.e. shared) for all `.debug_*` sections currently.


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:237
+
+## Case D.1: check we do not link the SHT_REL[A] section with .symtab implicitly
+##           when the latter one is excluded.
----------------
jhenderson wrote:
> We probably want a case for an explicit .symtab reference when .symtab has been excluded.
I've added `D.3`


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:245
+
+## Case D.2: check we report an error when a relocatable section has the Info field which
+##           points to an excluded section.
----------------
jhenderson wrote:
> has the -> has a
has **an**?


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:349-350
+
+## Case E: check we do not link SHT_LLVM_CALL_GRAPH_PROFILE/SHT_LLVM_ADDRSIG sections
+##         with .symtab implicitly when the latter one is excluded.
+# RUN: yaml2obj %s --docnum=14 -o %t10
----------------
jhenderson wrote:
> Similar to above, we also probably want a test case for an explicit Link references to an excluded section.
> 
> Could all these implicit cases linking to .symtab be folded together into one?
> 
> Here and below: "the latter one" -> "the latter" (feels more natural to me).
All done.


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

https://reviews.llvm.org/D81005