[yaml2obj] - Add a support for "<none>" value for all optional fields.

[grimar]

Summary

It implements an approach suggested in the D84398 thread.

With it the following:

Sections:
  - Name:   .bar
    Type:   SHT_PROGBITS
    Offset: [[MACRO=<none>]]

works just like the Offset key was not specified.
It is useful for tests that want to have a default value for a field and to
have a way to override it at the same time.

[grimar]

• Move the comment line.

[MaskRay]

What about the alternative syntax mentioned in https://reviews.llvm.org/D84398#2169623 ?

[grimar]

In D84526#2176664, @MaskRay wrote:

What about the alternative syntax mentioned in https://reviews.llvm.org/D84398#2169623 ?

Other syntactic forms include ShOffset?: [[TEST]].

IMO "Offset: [[MACRO=<none>]]" looks/works better. Since we have default values, e.g. [[MACRO=0x5]], the <none> looks consistent.
Seems "?:" is only meaningful with marcos, so personally I'd try to avoid introducing one more syntax for them.

At the worst, we can use [[TEST=ShOffset: 0]]

It is seems to be not an option, because we might do not know the default ShOffset. The idea of the change is to avoid specifying it explicitly.
(e.g. you often do not know the value of ShName, for example, so ShName=<none> - works, but any other default value - usually doesn't).

Upd:
Oh, I seems misunderstood the last one. So you suggest to have something like:

Sections:
  - Type:   SHT_NULL
    [[NAME=ShName: 0xX]]
    [[OFF=ShOffset: 0xY]]

And then just disable lines, i.e do -DNAME="" -DOFF=""?
But this doesn't work well too:

1. What to specify as 0xX, 0xY? We often can't know valid default values, so will it be values that works for the first sub-test? It shouldn't work good for other sub-tests I think

(and probably reads harder than "=<none>")

2. When there are more than one marco, disabling other ones, which should be unused, might look too verbose.

[jhenderson]

What's the motivation for testing every Optional<T> type? It seems to me like this code is a common piece of code where a single example would be sufficient. In fact, this test doesn't include coverage for the DWARF tag, for example (see @Higuoxing's recent work), which has many optional fields.

llvm/test/tools/yaml2obj/ELF/none-value.yaml

4

Perhaps change "value was not set" to "field was not specified"? I think it avoids ambiguity of the term "value", which might be the thing on the right of the colon.

11

It is exist -> It exists

183

You can probably get rid of the notes from the second YAML. The initial comment saying it is a duplicate, barring the missing fields, is sufficient to convey that, I think.

[grimar]

In D84526#2178235, @jhenderson wrote:

What's the motivation for testing every Optional<T> type? It seems to me like this code is a common piece of code where a single example would be sufficient.

OK, probably you're right. What about having a full set for a section instead?
E.g I'll compare:

- Name:         .bar
  Type:         SHT_PROGBITS
  Offset:       [[TEST=<none>]]
  Address:      [[TEST=<none>]]
  Content:      [[TEST=<none>]]
  Size:         [[TEST=<none>]]
  ContentArray: [[TEST=<none>]]
  Info:         [[TEST=<none>]]
  EntSize:      [[TEST=<none>]]
  ShName:       [[TEST=<none>]]
  ShOffset:     [[TEST=<none>]]
  ShSize:       [[TEST=<none>]]
  ShFlags:      [[TEST=<none>]]

vs

- Name:         .bar
  Type:         SHT_PROGBITS

The first set contains some possible interactions between Content, Size, ContentArray and ShSize,
between Name and ShName, Offset and ShOffset etc. I.e. probably demonstrates
the having =<none> really does nothing.

[jhenderson]

In D84526#2178269, @grimar wrote:

In D84526#2178235, @jhenderson wrote:

What's the motivation for testing every Optional<T> type? It seems to me like this code is a common piece of code where a single example would be sufficient.

OK, probably you're right. What about having a full set for a section instead?
E.g I'll compare:

- Name:         .bar
  Type:         SHT_PROGBITS
  Offset:       [[TEST=<none>]]
  Address:      [[TEST=<none>]]
  Content:      [[TEST=<none>]]
  Size:         [[TEST=<none>]]
  ContentArray: [[TEST=<none>]]
  Info:         [[TEST=<none>]]
  EntSize:      [[TEST=<none>]]
  ShName:       [[TEST=<none>]]
  ShOffset:     [[TEST=<none>]]
  ShSize:       [[TEST=<none>]]
  ShFlags:      [[TEST=<none>]]

vs

- Name:         .bar
  Type:         SHT_PROGBITS

The first set contains some possible interactions between Content, Size, ContentArray and ShSize,
between Name and ShName, Offset and ShOffset etc. I.e. probably demonstrates
the having =<none> really does nothing.

Yeah, that seems reasonable to me.

[grimar]

• Addressed review comments.

[jhenderson]

LGTM, with a couple of nits. I'm also happy with other strings/syntax etc, as long as it's fairly readable.

llvm/test/tools/yaml2obj/ELF/none-value.yaml

7

all of -> all

8

a resonable choice -> reasonable

llvm/include/llvm/Support/YAMLTraits.h

void mapOptionalWithContext(const char *Key, T &Val, const DefaultT &Default,
     this->processKeyWithDefault(Key, Val, static_cast<const T &>(Default),
                                 false, Ctx);
   }
 
 private:
   template <typename T, typename Context>
   void processKeyWithDefault(const char *Key, Optional<T> &Val,
                              const Optional<T> &DefaultValue, bool Required,
-                             Context &Ctx) {
-    assert(DefaultValue.hasValue() == false &&
-           "Optional<T> shouldn't have a value!");
-    void *SaveInfo;
-    bool UseDefault = true;
-    const bool sameAsDefault = outputting() && !Val.hasValue();
-    if (!outputting() && !Val.hasValue())
-      Val = T();
-    if (Val.hasValue() &&
-        this->preflightKey(Key, Required, sameAsDefault, UseDefault,
-                           SaveInfo)) {
-      yamlize(*this, Val.getValue(), Required, Ctx);
-      this->postflightKey(SaveInfo);
-    } else {
-      if (UseDefault)
-        Val = DefaultValue;
-    }
-  }
+                             Context &Ctx);
 
   template <typename T, typename Context>
   void processKeyWithDefault(const char *Key, T &Val, const T &DefaultValue,
                              bool Required, Context &Ctx) {
     void *SaveInfo;
     bool UseDefault;
     const bool sameAsDefault = outputting() && Val == DefaultValue;
     if ( this->preflightKey(Key, Required, sameAsDefault, UseDefault,
private:
   bool NeedBitValueComma = false;
   bool NeedFlowSequenceComma = false;
   bool EnumerationMatchFound = false;
   bool WriteDefaultValues = false;
   StringRef Padding;
   StringRef PaddingBeforeContainer;
 };
 
+template <typename T, typename Context>
+void IO::processKeyWithDefault(const char *Key, Optional<T> &Val,
+                               const Optional<T> &DefaultValue, bool Required,
+                               Context &Ctx) {
+  assert(DefaultValue.hasValue() == false &&
+         "Optional<T> shouldn't have a value!");
+  void *SaveInfo;
+  bool UseDefault = true;
+  const bool sameAsDefault = outputting() && !Val.hasValue();
+  if (!outputting() && !Val.hasValue())
+    Val = T();
+  if (Val.hasValue() &&
+      this->preflightKey(Key, Required, sameAsDefault, UseDefault, SaveInfo)) {
+
+    // When reading an Optional<X> key from a YAML description, we allow the
+    // special "<none>" value, which can be used to specify that no value was
+    // requested, i.e. the DefaultValue will be assigned. The DefaultValue is
+    // usually None.
+    bool IsNone = false;
+    if (!outputting())
+      if (auto *Node = dyn_cast<ScalarNode>(((Input *)this)->getCurrentNode()))
+        IsNone = Node->getRawValue() == "<none>";
+
+    if (IsNone)
+      Val = DefaultValue;
+    else
+      yamlize(*this, Val.getValue(), Required, Ctx);
+    this->postflightKey(SaveInfo);
+  } else {
+    if (UseDefault)
+      Val = DefaultValue;
+  }
+}
+
 /// YAML I/O does conversion based on types. But often native data types
 /// are just a typedef of built in intergral types (e.g. int).  But the C++
 /// type matching system sees through the typedef and all the typedefed types
 /// look like a built in type. This will cause the generic YAML I/O conversion
 /// to be used. To provide better control over the YAML conversion, you can
 /// use this macro instead of typedef.  It will create a class with one field
 /// and automatic conversion operators to and from the base type.
 /// Based on BOOST_STRONG_TYPEDEF

llvm/test/tools/yaml2obj/ELF/none-value.yaml

+## We have a special "<none>" value for all keys that are implemented
+## as Optional<> in the code. Setting a key to "<none>" means no-op and
+## works in the same way as when a field was not specified at all.
+
+## Test a few keys for which the "<none>" value is supported.
+## We do not test all possible keys, because it would be too verbose.
+## It reasonable to test all keys for a section, because normally many
+## of them would conflict or intersect when specified together.
+# RUN: yaml2obj %s --docnum=1 -o %t-none
+# RUN: yaml2obj %s --docnum=2 -o %t-base
+# RUN: cmp %t-none %t-base
+
+## We do not use the TEST macro. It exists to
+## demonstrate the expected use case for the <none> word.
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name:         .bar
+    Type:         SHT_PROGBITS
+    Offset:       [[TEST=<none>]]
+    Address:      [[TEST=<none>]]
+    Content:      [[TEST=<none>]]
+    Size:         [[TEST=<none>]]
+    ContentArray: [[TEST=<none>]]
+    Info:         [[TEST=<none>]]
+    EntSize:      [[TEST=<none>]]
+    ShName:       [[TEST=<none>]]
+    ShOffset:     [[TEST=<none>]]
+    ShSize:       [[TEST=<none>]]
+    ShFlags:      [[TEST=<none>]]
+
+## The same document, but all fields that were set to <none> are removed.
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .bar
+    Type: SHT_PROGBITS