[PATCH] D90458: [yaml2obj] - ProgramHeaders: introduce FirstSec/LastSec instead of Sections list.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:488
 
-      reportError("unknown section or fill referenced: '" + SecName.Section +
-                  "' by program header");
-    }
+    size_t First = NameToIndex[*YamlPhdr.FirstSec];
+    if (!First)
----------------
The behaviour here is a little subtle (relying on the zero-value insertion of a size_t when a member doesn't exist). I wonder if it would be worth adding some comments to make it clearer?


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:493
+                  Twine(I));
+    size_t Last = YamlPhdr.LastSec ? NameToIndex[*YamlPhdr.LastSec] : First;
+    if (!Last && YamlPhdr.LastSec)
----------------
This behaviour isn't immediately clear to me as being the behaviour when just `FirstSec` is specified. It could easily be thought that just `FirstSec` implies that section and all later sections. What do you think about requiring both keys if either is specified?


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:498
+                  Twine(I));
+    if (First && Last && First > Last)
+      reportError("program header with index " + Twine(I) +
----------------
Maybe worth bailing out at this point if either `First` or `Last` are invalid, rahter than needing the `if (First && Last)` bits here.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:917
+  if (!FileHdr.FirstSec && FileHdr.LastSec)
+    return "the \"LastSec\" can't be used without the \"FirstSec\" key";
+  return "";
----------------



================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs.test:99
-      - Section: .foo
-      - Section: .rela.dyn
-      - Section: .rela.plt
----------------
Side-note: this one was previously missing .got.plt!


================
Comment at: llvm/test/tools/obj2yaml/ELF/program-headers.yaml:331-332
 
 ## Test we include non-allocatable sections in segments.
 ## We also document that SHT_NULL sections are not considered to be inside a segment.
 # RUN: yaml2obj --docnum=4 %s -o %t4
----------------
This comment probably needs updating.


================
Comment at: llvm/test/tools/yaml2obj/ELF/custom-fill.yaml:265-269
 ## Check we report an error when a program header references
 ## an unknown section or fill and have at least one Fill defined.
 
 # RUN: not yaml2obj --docnum=10 2>&1 %s | FileCheck %s --check-prefix=UNKNOWN-ERR
+# UNKNOWN-ERR: error: unknown section or fill referenced: 'fill' by the 'FirstSec' key of the program header with index 0
----------------
We probably need the same test case but for the `LastSec` key.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header.yaml:112
+
+## Check we report an error when the index of section spicified by the "FirstSec" key
+## is greater than the index of section specified by the "LastSec" key.
----------------



================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header.yaml:113
+## Check we report an error when the index of section spicified by the "FirstSec" key
+## is greater than the index of section specified by the "LastSec" key.
+
----------------



================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header.yaml:127
+
+## Check we can specify the "FirstSec" alone to create the segment that includes a single section.
+
----------------



================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header.yaml:145
+
+## Check that we include all sections in [FirstSec, LastSec] region to the segment when both keys are used.
+
----------------
Should we have a test case showing that Fills are included as well?


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

https://reviews.llvm.org/D90458