[PATCH] D67182: [lib/ObjectYAML] - Improve and cleanup error reporting in ELFState<ELFT> class.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:367
+    if (!Sec->Link.empty())
+      SHeader.sh_link = toSectionIndex(Sec->Link, Sec->Name);
 
----------------
I think we need a test case showing this can be called even after an earlier sh_link fails (e.g. two sections with bad sh_link values).


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:380
     } else if (auto S = dyn_cast<ELFYAML::RawContentSection>(Sec)) {
-      if (!writeSectionContent(SHeader, *S, CBA))
-        return false;
+      writeSectionContent(SHeader, *S, CBA);
     } else if (auto S = dyn_cast<ELFYAML::SymtabShndxSection>(Sec)) {
----------------
Arguably, the same goes for each of the changes here too - if the writeSectionContent can fail for a given section type, we should have a test showing that the loop continues.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:710
+  if (!Section.RelocatableSec.empty())
+    SHeader.sh_info = toSectionIndex(Section.RelocatableSec, Section.Name);
 
----------------
It might be interesting to have a test case showing that a bad sh_info and an unknown symbol index are both reported for the same relocation section. Up to you.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:768
+    else
+      SectionIndex = toSectionIndex(Member.sectionNameOrType, Section.Name);
     support::endian::write<uint32_t>(OS, SectionIndex, ELFT::TargetEndianness);
----------------
Perhaps worth a test for a bad symbol and multiple bad section indexes in the same section.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:945
+                  "' at YAML section number " + Twine(I));
       return false;
     }
----------------
Could this just be a `continue` and this function return void?


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:960
+      reportError("repeated symbol name: '" + Sym.Name + "'");
+      return false;
+    }
----------------
Could this be a `continue` and return void?


================
Comment at: test/tools/yaml2obj/elf-comdat-broken-info.yaml:1
-# RUN: yaml2obj %s -o %t
-# RUN: llvm-readobj --sections %t | FileCheck %s
+## Check we are able to produce SHT_GROUP section with a custom Info value (12345).
+
----------------
produce an SHT_GROUP


================
Comment at: test/tools/yaml2obj/elf-comdat-broken-info.yaml:30
+
+## Check we report multiple errors when unknown symbols are referenced by SHT_GROUP.
+
----------------
when multiple unknown symbols
by SHT_GROUP sections.


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

https://reviews.llvm.org/D67182