[PATCH] D95505: [yaml2obj] Initial support for 32-bit XCOFF in yaml2obj.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:59
   XCOFF::StorageClass StorageClass;
-  uint8_t NumberOfAuxEntries; // Number of auxiliary entries
+  llvm::yaml::Hex8 NumberOfAuxEntries; // Number of auxiliary entries
 };
----------------
Nit: missing trailing full stop for this comment. But really, I'm not sure this comment adds anything, as the name is pretty clear to me.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:54
+bool XCOFFWriter::assignAddressesAndIndices() {
+  int16_t SectionIndex = 1;
+  for (auto &YamlSec : Obj.Sections) {
----------------
Two questions:

1) Why is this a signed type?
2) What is the maximum number of sections an XCOFF file can have? In ELF, it is effectively UINT32_MAX, for example. (In practice, it's unlikely that we'll see that many sections, but we shouldn't prevent it just due to the wrong type). This type should be big enough for whatever the max can be.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:55
+  int16_t SectionIndex = 1;
+  for (auto &YamlSec : Obj.Sections) {
+    if (YamlSec.Address % DefaultSectionAlign != 0) {
----------------
In general, we tend to avoid using `auto` these days in new code, unless the type is obvious. In this case, the type of the member of `Obj.Sections` is not obvious.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:65
+      if (SectionIndex > MaxSectionIndex) {
+        ErrHandler("Section index overflow!");
+        return false;
----------------
General point that applies here and in all other new error messages: the coding guidelines say error messages start with lower case and don't end in a full stop. They don't explicitly exclude ending in an exclamation mark, but I'd avoid it as well.

Specific point: it would be helpful if this error said what the maximum number of sections the code supports is.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:72
+      YamlSec.NumberOfRelocations = YamlSec.Relocations.size();
+    if (YamlSec.NumberOfRelocations != YamlSec.Relocations.size()) {
+      ErrHandler("Number of relocations is incorrect!");
----------------
Here and in similar cases below for sections etc, I don't think what you've done is good. I think you shouldn't emit an error if a user explicitly specifies the number of relocations (sections etc). Just use the specified value in the header.

Take a look at what we do for ELF yaml2obj already. For many fields there is a default value that is derived from the contents of the YAML, and an option to override that value. For example, the ELF header has an e_shnum field which states the number of sections in that object. This value is automatically populated with the number of sections provided in the YAML. However, if a different value is specified in the YAML for the e_shnum field, that value is written to e_shnum instead, whilst the real set of sections is still written out. This enables a user to create a deliberate inconsistency between the two properties, which allows for things like error handling testing of malformed objects.


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:84
     IO &IO, XCOFFYAML::FileHeader &FileHdr) {
   IO.mapRequired("MagicNumber", FileHdr.Magic);
+  IO.mapOptional("NumberOfSections", FileHdr.NumberOfSections);
----------------
In general, I think everything should be optional, unless there is no sensible default. I haven't looked at the spec yet, but presumably the magic is fixed, or used to distinguish between 32/64 bit. It seems reasonable that a user doesn't need to specify it and then yaml2obj just picks something like 32 bit automatically. I think it's important that you don't require more than you absolutely have to, because otherwise it bloats the YAML and makes it hard to identify what's actually important to the test case in question.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95505