Page MenuHomePhabricator

Initialize optional members of ELFYAML types.
Needs ReviewPublic

Authored by vzakhari on Apr 26 2021, 9:10 PM.

Details

Summary

This change-set initializes optional memrbers of ELFYAML types to their default values (as defined in ELFYAML.cpp mapping traits).

For example, since Section has a user-defined constructor, one cannot neither default-initialize (e.g. with Section MySection;) nor zero-initilize it (e.g. with Section MySection{};) completely. The optional members will be uninitialized. The same is true for MipsABIFlags.

Moreover, YamlIO documentation says that the native types must have default constructors that must initialize the optional members: https://llvm.org/docs/YamlIO.html#default-values

This change-set is almost no-op, since I have not found uses that output ELFYAML structures as YAML. The new unit test seems to be the first such use. The unit test will fail in an undefined way, if the optional members are not properly initialized.

Diff Detail

Event Timeline

vzakhari created this revision.Apr 26 2021, 9:10 PM
vzakhari requested review of this revision.Apr 26 2021, 9:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2021, 9:10 PM
vzakhari updated this revision to Diff 340954.Apr 27 2021, 12:52 PM
vzakhari edited the summary of this revision. (Show Details)

BBAddrMap-related changes LGTM.

MaskRay added inline comments.Apr 30 2021, 10:05 AM
llvm/include/llvm/ObjectYAML/ELFYAML.h
661

Let strong typedef Hex16/MIPS_AFL_REG initialize the base value

vzakhari added inline comments.Apr 30 2021, 10:14 AM
llvm/include/llvm/ObjectYAML/ELFYAML.h
661

Sorry, I do not understand this.

Are you suggesting to let Hex16's default constructor to initialize the actual value? I believe there is no default constructor that would do that.

#define LLVM_YAML_STRONG_TYPEDEF(_base, _type)                                 \
    struct _type {                                                             \
        _type() = default;                                                     \
        _type(const _base v) : value(v) {}                                     \
        _type(const _type &v) = default;                                       \
        _type &operator=(const _type &rhs) = default;                          \
        _type &operator=(const _base &rhs) { value = rhs; return *this; }      \
        operator const _base & () const { return value; }                      \
        bool operator==(const _type &rhs) const { return value == rhs.value; } \
        bool operator==(const _base &rhs) const { return value == rhs; }       \
        bool operator<(const _type &rhs) const { return value < rhs.value; }   \
        _base value;                                                           \
        using BaseType = _base;                                                \
    };

Default construction will leave value uninitialized for POD _base type. Zero-initializaton will set value to 0.

vzakhari added inline comments.Apr 30 2021, 10:20 AM
llvm/include/llvm/ObjectYAML/ELFYAML.h
661

Just to clarify, I preferred value-initialization over zero-initialization to make all the initializations consistent, and since not all default values may be 0.

MaskRay added inline comments.Apr 30 2021, 10:30 AM
llvm/include/llvm/ObjectYAML/ELFYAML.h
661

You can initialize _base value; , then you don't need to spread {0} everywhere.

vzakhari added inline comments.Apr 30 2021, 11:22 AM
llvm/include/llvm/ObjectYAML/ELFYAML.h
661

Can you please suggest the exact change? Do you mean something like _base value = _base()?

In general, _base type may have a deleted default constructor, so this may not work.

jhenderson added inline comments.May 11 2021, 12:51 AM
llvm/include/llvm/ObjectYAML/ELFYAML.h
661

Are there any actual cases in current practice where the default constructor has been deleted?

vzakhari added inline comments.May 11 2021, 8:40 AM
llvm/include/llvm/ObjectYAML/ELFYAML.h
661

I tried adding the in-class initialization as _base value = _base() and hit the following issue:

llvm/include/llvm/ObjectYAML/WasmYAML.h:86:8: note: ‘llvm::WasmYAML::Import::Import()’ is implicitly deleted because the default definition would be ill-formed:
 struct Import {
        ^~~~~~
llvm/include/llvm/ObjectYAML/WasmYAML.h:92:12: error: union member ‘llvm::WasmYAML::Import::<unnamed union>::GlobalImport’ with non-trivial ‘llvm::WasmYAML::Global::Global()’
     Global GlobalImport;
            ^~~~~~~~~~~~
llvm/include/llvm/ObjectYAML/WasmYAML.h:93:11: error: union member ‘llvm::WasmYAML::Import::<unnamed union>::TableImport’ with non-trivial ‘llvm::WasmYAML::Table::Table()’
     Table TableImport;
           ^~~~~~~~~~~
llvm/include/llvm/ObjectYAML/WasmYAML.h:94:12: error: union member ‘llvm::WasmYAML::Import::<unnamed union>::Memory’ with non-trivial ‘constexpr llvm::WasmYAML::Limits::Limits()’
     Limits Memory;
            ^~~~~~
vzakhari added inline comments.May 11 2021, 10:08 AM
llvm/include/llvm/ObjectYAML/ELFYAML.h
661

I believe this is a reproducer:

struct A {
  int x = int();
};

struct B {
  union {
    A a;
  };
};

void foo() { B b = B(); }
ctr.cpp:11:20: error: call to implicitly-deleted default constructor of 'B'
void foo() { B b = B(); }
                   ^
ctr.cpp:7:7: note: default constructor of 'B' is implicitly deleted because variant field 'a' has a non-trivial default constructor
    A a;
      ^
1 error generated.

A corresponds to the strongdef'ed type.

@vzakhari, what's the motivation for doing this? These data structures are used within yaml2obj/obj2yaml, although I'm not particularly familiar with the plumbing to identify whether it matters. My suspicion is that it doesn't.

llvm/include/llvm/ObjectYAML/ELFYAML.h
661

@MaskRay, do you have any suggestions on how to improve this. It looks to me like you had something in mind, but @vzakhari has been unable to identify it.

MaskRay added inline comments.May 12 2021, 3:10 PM
llvm/include/llvm/ObjectYAML/ELFYAML.h
661

We can change LLVM_YAML_STRONG_TYPEDEF or add a similar macro for integeral types - initialized to 0.

Then we can avoid {0} here and there.

@vzakhari, what's the motivation for doing this? These data structures are used within yaml2obj/obj2yaml, although I'm not particularly familiar with the plumbing to identify whether it matters. My suspicion is that it doesn't.

@jhenderson, there are two ways to create a YAML file using YAML mappings. The first one is to create a YAML file from a string bufffer (see line 38 in llvm/unittests/ObjectYAML/ELFYAMLTest.cpp). The second way is to populate the ELFYAML:: data structures and stream them into yaml::Output. Both ways are valid. The second way is a little bit more convenient for programmatic ELF-YAML creation in my downstream application. When I try to use ELFYAML:: data structures, there is no convenient way to make sure that all optional members in them are initialized. Neither default nor zero-initialization works. This is why I made these changes to make sure that all optional members are initialized properly. Note that the default values are not necessarily zeroes. The default values for this particular case are defined by the mappings in llvm/lib/ObjectYAML/ELFYAML.cpp. So I am not sure that adjusting LLVM_YAML_STRONG_TYPEDEF to use zero values always is the right approach.

@vzakhari, what's the motivation for doing this? These data structures are used within yaml2obj/obj2yaml, although I'm not particularly familiar with the plumbing to identify whether it matters. My suspicion is that it doesn't.

@jhenderson, there are two ways to create a YAML file using YAML mappings. The first one is to create a YAML file from a string bufffer (see line 38 in llvm/unittests/ObjectYAML/ELFYAMLTest.cpp). The second way is to populate the ELFYAML:: data structures and stream them into yaml::Output. Both ways are valid. The second way is a little bit more convenient for programmatic ELF-YAML creation in my downstream application. When I try to use ELFYAML:: data structures, there is no convenient way to make sure that all optional members in them are initialized. Neither default nor zero-initialization works. This is why I made these changes to make sure that all optional members are initialized properly. Note that the default values are not necessarily zeroes.

Which cases don't have zero as their default value? The ones you've modified in this patch all are (even the Mips enums are - the enum values are value 0).

Frankly speaking, I don't like the style of the change: it looks somewhat ugly (= 0 would be better than {0} in my opinion at least), but more importantly there's no guarantee that it will continue to work - more fields could be added at a later date, for example, without this initialisation, especially as it's not clear when it needs to be explicitly initialised (this change for example doesn't change the Size field of the StackSizeEntry struct). It sounds like there's no internal-to-LLVM usage of this approach, so it could easily be considered unnecessary and get dropped at a later date too.

@vzakhari, what's the motivation for doing this? These data structures are used within yaml2obj/obj2yaml, although I'm not particularly familiar with the plumbing to identify whether it matters. My suspicion is that it doesn't.

@jhenderson, there are two ways to create a YAML file using YAML mappings. The first one is to create a YAML file from a string bufffer (see line 38 in llvm/unittests/ObjectYAML/ELFYAMLTest.cpp). The second way is to populate the ELFYAML:: data structures and stream them into yaml::Output. Both ways are valid. The second way is a little bit more convenient for programmatic ELF-YAML creation in my downstream application. When I try to use ELFYAML:: data structures, there is no convenient way to make sure that all optional members in them are initialized. Neither default nor zero-initialization works. This is why I made these changes to make sure that all optional members are initialized properly. Note that the default values are not necessarily zeroes.

Which cases don't have zero as their default value? The ones you've modified in this patch all are (even the Mips enums are - the enum values are value 0).

Sorry, I do not see where I said that there are no default values. I said that neither default nor zero-initialization works - in this phrase I referred to initialization of POD members of objects, when we use default or zero-initialization. For example, look at struct MipsABIFlags: if we declare an object of this type using default initialization/construction, then members like ISARevision will be default constructed, which in turn causes their value to be undefined (because they use the default constructor of the type defined by LLVM_YAML_STRONG_TYPEDEF). Value/zero-initialization will also not work for such objects, because struct MipsABIFlags has a user-defined constructor.

Frankly speaking, I don't like the style of the change: it looks somewhat ugly (= 0 would be better than {0} in my opinion at least), but more importantly there's no guarantee that it will continue to work - more fields could be added at a later date, for example, without this initialisation, especially as it's not clear when it needs to be explicitly initialised (this change for example doesn't change the Size field of the StackSizeEntry struct). It sounds like there's no internal-to-LLVM usage of this approach, so it could easily be considered unnecessary and get dropped at a later date too.

I can try to change {0} to = 0, if this helps getting this merged. Please let me know.
Yes, there is not guarantee that newly added optional fields are properly initialized. There is no way to assert this, just as many other things developers do. Why does it matter?
The added unit test verifies that the existing optional fields are properly initialized, so this should be enough to make sure that the current changes are no dropped accidentally.

Again, I am just following the specification of YamlIO, which says: "That native class must have a default constructor. Whatever value the default constructor initially sets for an optional field will be that field’s value."
I believe this implies that the default constructors of YamlIO objects must set their optional fields to some not undefined value.