[PATCH] D89949: [yaml2obj][obj2yaml] - Teach tools to work with regular archives.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ArchiveYAML.cpp:27-29
+  // MappingTraits<ArchYAML::Archive>::validate() is not invoked for
+  // top-level ArchYAML::Archive YAML description. It is perhaps not a critical
+  // limitation. Just call validate() directly from here.
----------------
jhenderson wrote:
> Could you explain why it doesn't get invoked? Presumably the equivalent functions for other formats are?
> Presumably the equivalent functions for other formats are?

No. Below is the piece of the code that calls mapping for the top level for each format:


```
void MappingTraits<YamlObjectFile>::mapping(IO &IO,
                                            YamlObjectFile &ObjectFile) {
  if (IO.outputting()) {
...
  } else {
    Input &In = (Input &)IO;
    if (IO.mapTag("!Arch")) {
      ObjectFile.Arch.reset(new ArchYAML::Archive());
      MappingTraits<ArchYAML::Archive>::mapping(IO, *ObjectFile.Arch);
    } else if (IO.mapTag("!ELF")) {
      ObjectFile.Elf.reset(new ELFYAML::Object());
      MappingTraits<ELFYAML::Object>::mapping(IO, *ObjectFile.Elf);
    } else if (IO.mapTag("!COFF")) {
      ObjectFile.Coff.reset(new COFFYAML::Object());
      MappingTraits<COFFYAML::Object>::mapping(IO, *ObjectFile.Coff);
    } else if (IO.mapTag("!mach-o")) {
      ObjectFile.MachO.reset(new MachOYAML::Object());
      MappingTraits<MachOYAML::Object>::mapping(IO, *ObjectFile.MachO);
    } else if (IO.mapTag("!fat-mach-o")) {
      ObjectFile.FatMachO.reset(new MachOYAML::UniversalBinary());
      MappingTraits<MachOYAML::UniversalBinary>::mapping(IO,
                                                         *ObjectFile.FatMachO);
    } else if (IO.mapTag("!minidump")) {
      ObjectFile.Minidump.reset(new MinidumpYAML::Object());
      MappingTraits<MinidumpYAML::Object>::mapping(IO, *ObjectFile.Minidump);
    } else if (IO.mapTag("!WASM")) {
      ObjectFile.Wasm.reset(new WasmYAML::Object());
      MappingTraits<WasmYAML::Object>::mapping(IO, *ObjectFile.Wasm);
    } else if (const Node *N = In.getCurrentNode()) {
      if (N->getRawTag().empty())
        IO.setError("YAML Object File missing document type tag!");
      else
        IO.setError("YAML Object File unsupported document type tag '" +
                    N->getRawTag() + "'!");
    }
  }
}
```

The code above just doesn't call validate. I think it just because it doesn't need it atm. E.g. when
we describe an ELF file, we can't specify both "Sections" and "Content" as ELF parser doesn't allow
us to set a content.

I've moved this piece of code to `MappingTraits<YamlObjectFile>::mapping`


================
Comment at: llvm/test/tools/obj2yaml/Archives/regular.yaml:58-66
+Entries:
+  - Name:         ''
+    LastModified: ''
+    UID:          ''
+    GID:          ''
+    AccessMode:   ''
+    Size:         '0'
----------------
jhenderson wrote:
> Can we omit the Entries here?
Now, when all fields are optional - yes.


================
Comment at: llvm/test/tools/yaml2obj/Archives/regular.yaml:31
+
+# RUN: yaml2obj --docnum=1 -DMAGIC="12345678" %s -o %t.magic.a
+# RUN: wc -c < %t.magic.a | FileCheck %s --check-prefix=EMPTY-SIZE
----------------
jhenderson wrote:
> (fun fact: the ability to change the magic bytes arbitrarily allows someone to use yaml2obj as a kind of "echo" tool, by just specifying the arbitrary bytes to anything they want and omitting content and entries)
Not exactly. They still need to provide a file type first, e.g. "--- !Arch".


================
Comment at: llvm/test/tools/yaml2obj/Archives/regular.yaml:46
+
+## Check we can specify magic bytes of less greater than the normal size (size of "!<arch>\n").
+
----------------
jhenderson wrote:
> Maybe we don't need the "same size" case?
OK. I've removed the "same size" case.




================
Comment at: llvm/test/tools/yaml2obj/Archives/regular.yaml:59-73
+# RUN: od -t x1 -v %t.two.a | FileCheck %s --ignore-case --check-prefix=TWO-DATA
+
+# TWO:      {{^}}bbbbbbbbbbbbbbb{{$}}
+# TWO-NEXT: {{^}}a{{$}}
+# TWO-NOT:  {{.}}
+
+# TWO-DATA:      21 3c 61 72 63 68 3e 0a 62 62 62 62 62 62 62 62
----------------
jhenderson wrote:
> Rather than using `od`, I wonder if you could just use `FileCheck` directly on the archive, since the archive is a textual format?
> 
> ```
> # RUN: FileCheck --input-file=%t.two.a --check-prefix=TWO-DATA --strict-whitespace --match-full-lines --implicit-check-not={{.}}
> 
> #      TWO-DATA:!<arch>
> # TWO-DATA-NEXT:bbbbbbbbbbbbbbb/1234567890abqwertyasdfgh876543217`
> # TWO-DATA-NEXT:<some ASCII data>
> ...
> ```
> 
> I think the only change you'd need to make to the YAML is to use an ASCII character for the padding byte (e.g. 0x0a).
Done.


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

https://reviews.llvm.org/D89949