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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/ArchiveYAML.h:25
+struct Archive {
+  struct Entry {
+    struct Field {
----------------
I feel like Child or Member are more common terms for the parts of an archive. Probably best to pick one and use that? Same goes for the strings used in the YAML.


================
Comment at: llvm/include/llvm/ObjectYAML/yaml2obj.h:53
 
+bool yaml2arch(ArchYAML::Archive &Doc, raw_ostream &Out, ErrorHandler EH);
 bool yaml2coff(COFFYAML::Object &Doc, raw_ostream &Out, ErrorHandler EH);
----------------
Maybe rename to yaml2archive, since "arch" could be ambiguous (e.g. meaning "architecture").


================
Comment at: llvm/lib/ObjectYAML/ArchiveYAML.cpp:23
+  IO.mapTag("!Arch", true);
+  IO.mapRequired("Magic", A.Magic);
+  IO.mapOptional("Entries", A.Entries);
----------------
I think the magic should be optional, and default to a regular archive's magic.


================
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.
----------------
Could you explain why it doesn't get invoked? Presumably the equivalent functions for other formats are?


================
Comment at: llvm/lib/ObjectYAML/ArchiveYAML.cpp:48
+  for (auto &P : E.Fields)
+    IO.mapRequired(P.first.data(), P.second.Value);
+  IO.mapRequired("Content", E.Content);
----------------
I'd like these to be optional too, with sensible defaults for the various fields. Having to specify them in most cases just generates noise.


================
Comment at: llvm/lib/ObjectYAML/ArchiveYAML.cpp:49
+    IO.mapRequired(P.first.data(), P.second.Value);
+  IO.mapRequired("Content", E.Content);
+  IO.mapOptional("PaddingByte", E.PaddingByte);
----------------
This should be optional, with a default of no content.


================
Comment at: llvm/test/tools/obj2yaml/Archives/regular.yaml:58-66
+Entries:
+  - Name:         ''
+    LastModified: ''
+    UID:          ''
+    GID:          ''
+    AccessMode:   ''
+    Size:         '0'
----------------
Can we omit the Entries here?


================
Comment at: llvm/test/tools/yaml2obj/Archives/regular.yaml:3
+
+## Check we create an empty section when neither of "Entries" nor "Content" are specified.
+
----------------



================
Comment at: llvm/test/tools/yaml2obj/Archives/regular.yaml:6
+# RUN: yaml2obj --docnum=1 %s -o %t.empty.a
+# RUN: llvm-ar -t %t.empty.a | FileCheck %s --allow-empty --implicit-check-not={{.}}
+# RUN: wc -c < %t.empty.a | FileCheck %s --check-prefix=EMPTY-SIZE
----------------
llvm-ar options traditionally don't have `-`.


================
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
----------------
(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)


================
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").
+
----------------
Maybe we don't need 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
----------------
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).


================
Comment at: llvm/test/tools/yaml2obj/Archives/regular.yaml:89
+## An arbitrary entry to demonstrate that we use the 0x20 byte (space character)
+## to fulfill gaps between field values.
+  - Name:         'a/'
----------------



================
Comment at: llvm/tools/obj2yaml/archive2yaml.cpp:17
+
+class ArchiveDumper final {
+public:
----------------
Personal opinion: I don't see any benefit to `final` here, or even in general, since it prevents users extending things. I don't think that's useful.


================
Comment at: llvm/tools/obj2yaml/archive2yaml.cpp:28
+    if (!Buffer.startswith(Magic))
+      return createStringError(std::errc::illegal_byte_sequence,
+                               "only regular archives are supported");
----------------



================
Comment at: llvm/tools/obj2yaml/archive2yaml.cpp:49-55
+      E.Fields["Name"].Value = ToString(Hdr.Name);
+      E.Fields["LastModified"].Value = ToString(Hdr.LastModified);
+      E.Fields["UID"].Value = ToString(Hdr.UID);
+      E.Fields["GID"].Value = ToString(Hdr.GID);
+      E.Fields["AccessMode"].Value = ToString(Hdr.AccessMode);
+      E.Fields["Size"].Value = ToString(Hdr.Size);
+      E.Fields["Terminator"].Value = ToString(Hdr.Terminator);
----------------
If we make the header fields optional in the YAML, it's probably worth omitting them from obj2yaml's output.


================
Comment at: llvm/tools/obj2yaml/archive2yaml.cpp:60
+        return createStringError(std::errc::illegal_byte_sequence,
+                                 "unable to read entry size");
+      if (Buffer.size() < Size)
----------------
This message maybe should include the Size field contents.


================
Comment at: llvm/tools/obj2yaml/archive2yaml.cpp:63
+        return createStringError(std::errc::illegal_byte_sequence,
+                                 "unable to read entry data");
+      E.Content = arrayRefFromStringRef(Buffer.take_front(Size));
----------------
This should maybe list the expected size and the buffer size.


================
Comment at: llvm/tools/obj2yaml/archive2yaml.cpp:66
+
+      const bool HasPaddingByte = (Size & 1) && Buffer.size() > Size;
+      if (HasPaddingByte)
----------------
Is there testing for the `Buffer.size() > Size` aspect of this? In other words, showing that if the member ends at the end of the file, no padding.


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

https://reviews.llvm.org/D89949