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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:367
+    if (!Sec->Link.empty())
+      SHeader.sh_link = toSectionIndex(Sec->Link, Sec->Name);
 
----------------
jhenderson wrote:
> 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).
Done in section-link.yaml.


================
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)) {
----------------
jhenderson wrote:
> 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.
I am not sure it worth testing so hard honestly.

There is nothing like
```
if (HasError)
  return;
```

around, so I think no reason to assume that loop can terminate.
It feels that testing a single `writeSectionContent` should be enough.

However, the current situation is following.
`writeSectionContent` can fail for:

1) `writeSectionContent(...const ELFYAML::DynamicSection &Section...)`
It is tested in: dynamic-section-raw-content.yaml

2) `writeSectionContent(...const ELFYAML::RelocationSection &Section...)`
I added a test to reloc-sec-info.yaml

3) `writeSectionContent(...const ELFYAML::Group &Section...)`
It is tested in: elf-comdat-broken-info.yaml


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:710
+  if (!Section.RelocatableSec.empty())
+    SHeader.sh_info = toSectionIndex(Section.RelocatableSec, Section.Name);
 
----------------
jhenderson wrote:
> 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.
I added a test to reloc-sec-info.yaml


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


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


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


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

https://reviews.llvm.org/D67182