[PATCH] D71411: [yaml2obj] - Add a way to override sh_flags section field.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D71411#1781744 <https://reviews.llvm.org/D71411#1781744>, @jhenderson wrote:

> This looks fine to me. I do have one question though: is it possible to allow Flags to take two different kinds of value (i.e. a list or a number)? If so, we don't need an extra field - if a number is specified, then use that and do no validation, otherwise try to map the list values to the flags and do validation. The other fields make sense because they allow overriding a value which is also used for something else (e.g. ShOffset allows overriding the section's offset, but the actual offset is still used for writing data).


Currently we map Flags as `ScalarBitSetTraits<ELFYAML::ELF_SHF>`, it is defined as `Optional<ELF_SHF> Flags;`,
so seems we will need to add a new type to perform mapping.

I guess a correct way to implement that would be to do something like we did for `st_other`, where we needed to support both flags
and numeric values, i.e. to use the `MappingNormalization` and add something like `NormalizedOther` and `StOtherPiece`:
https://github.com/llvm-mirror/llvm/blob/master/lib/ObjectYAML/ELFYAML.cpp#L868
https://github.com/llvm-mirror/llvm/blob/master/lib/ObjectYAML/ELFYAML.cpp#L974
https://github.com/llvm-mirror/llvm/blob/master/lib/ObjectYAML/ELFYAML.cpp#L852

Perhaps it is possible to write a bit less code, since "a list or a number" case sounds simpler,
though it is not clear yet. Perphaps easier would be to stick to format of `st_other` and allow to mix flags
and values, like: `[SHF_ALLOC, 0xFEFEFEFE]` then.

It is not that hard to implement probably, but my concern is the next:
For `st_other` we knew it adds a complexity to the code, but we had no choice probably as the format
of the  `st_other` was kind of special.

But does it worth adding such a complexity to the code to implement the same for `Flags`?


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

https://reviews.llvm.org/D71411