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

[grimar]

Summary

Currently we have the Flags property that allows to
set flags for a section. The problem is that it does not
allow us to set an arbitrary values, because of bit fields
validation under the hood. An arbitrary values can be used
to test specific broken cases.

We probably do not want to relax the validation, so this
patch adds a ShSize property that allows to
override the sh_size. It is inline with others Sh* properties
we have already.

[jhenderson]

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).

[grimar]

In 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?

[jhenderson]

Okay, LGTM. Might want to give @MaskRay a chance to chime in before committing.

[MaskRay]

Keeping both ShFlags and Flags is fine if you think doing things the Other way is hard to implement.

I investigated this a bit. The most difficult part is reading. yamlize has to make the decision whether a value is scalar or not, bitset or not early. If we want to parse both an integer (Hex64) and an array of strings, it will be difficult.

template <typename T>
typename std::enable_if<has_ScalarBitSetTraits<T>::value, void>::type
yamlize(IO &io, T &Val, bool, EmptyContext &Ctx) {
  bool DoClear;
  if ( io.beginBitSetScalar(DoClear) ) {
    if ( DoClear )
      Val = T();
    ScalarBitSetTraits<T>::bitset(io, Val);
    io.endBitSetScalar();
  }
}

The problem is that it does not allow us to set an arbitrary values

values->value

llvm/test/tools/yaml2obj/ELF/override-shflags.yaml

73

workaround (noun) -> work around

llvm/include/llvm/ObjectYAML/ELFYAML.h

struct Section : public Chunk {
 
   // This can be used to override the sh_offset field. It does not place the
   // section data at the offset specified.
   Optional<llvm::yaml::Hex64> ShOffset;
 
   // This can be used to override the sh_size field. It does not affect the
   // content written.
   Optional<llvm::yaml::Hex64> ShSize;
+
+  // This can be used to override the sh_flags field.
+  Optional<llvm::yaml::Hex64> ShFlags;
 };
 
 // Fill is a block of data which is placed outside of sections. It is
 // not present in the sections header table, but it might affect the output file
 // size and program headers produced.
 struct Fill : Chunk {
   Optional<yaml::BinaryRef> Pattern;
   llvm::yaml::Hex64 Size;

llvm/lib/ObjectYAML/ELFEmitter.cpp

if (!SymMap.lookup(S, Index) && !to_integer(S, Index)) {
     reportError("unknown symbol referenced: '" + S + "' by YAML section '" +
                 LocSec + "'");
     return 0;
   }
   return Index;
 }
 
 template <class ELFT>
+static void overrideFields(ELFYAML::Section *From, typename ELFT::Shdr &To) {
+  if (!From)
+    return;
+  if (From->ShFlags)
+    To.sh_flags = *From->ShFlags;
+  if (From->ShName)
+    To.sh_name = *From->ShName;
+  if (From->ShOffset)
+    To.sh_offset = *From->ShOffset;
+  if (From->ShSize)
+    To.sh_size = *From->ShSize;
+}
+
+template <class ELFT>
 bool ELFState<ELFT>::initImplicitHeader(ContiguousBlobAccumulator &CBA,
                                         Elf_Shdr &Header, StringRef SecName,
                                         ELFYAML::Section *YAMLSec) {
   // Check if the header was already initialized.
   if (Header.sh_offset)
     return false;
 
   if (SecName == ".symtab")
     initSymtabSectionHeader(Header, SymtabType::Static, CBA, YAMLSec);
   else if (SecName == ".strtab")
     initStrtabSectionHeader(Header, SecName, DotStrtab, CBA, YAMLSec);
   else if (SecName == ".shstrtab")
     initStrtabSectionHeader(Header, SecName, DotShStrtab, CBA, YAMLSec);
   else if (SecName == ".dynsym")
     initSymtabSectionHeader(Header, SymtabType::Dynamic, CBA, YAMLSec);
   else if (SecName == ".dynstr")
     initStrtabSectionHeader(Header, SecName, DotDynstr, CBA, YAMLSec);
   else
     return false;
 
-  // Override the fields if requested.
-  if (YAMLSec) {
-    if (YAMLSec->ShName)
-      Header.sh_name = *YAMLSec->ShName;
-    if (YAMLSec->ShOffset)
-      Header.sh_offset = *YAMLSec->ShOffset;
-    if (YAMLSec->ShSize)
-      Header.sh_size = *YAMLSec->ShSize;
-  }
-
+  // Override section fields if requested.
+  overrideFields<ELFT>(YAMLSec, Header);
   return true;
 }
 
 StringRef llvm::ELFYAML::dropUniqueSuffix(StringRef S) {
   size_t SuffixPos = S.rfind(" [");
   if (SuffixPos == StringRef::npos)
     return S;
   return S.substr(0, SuffixPos);
for (const std::unique_ptr<ELFYAML::Chunk> &D : Doc.Chunks) {
     } else if (auto S = dyn_cast<ELFYAML::GnuHashSection>(Sec)) {
       writeSectionContent(SHeader, *S, CBA);
     } else if (auto S = dyn_cast<ELFYAML::DependentLibrariesSection>(Sec)) {
       writeSectionContent(SHeader, *S, CBA);
     } else {
       llvm_unreachable("Unknown section type");
     }
 
-    // Override the fields if requested.
-    if (Sec) {
-      if (Sec->ShName)
-        SHeader.sh_name = *Sec->ShName;
-      if (Sec->ShOffset)
-        SHeader.sh_offset = *Sec->ShOffset;
-      if (Sec->ShSize)
-        SHeader.sh_size = *Sec->ShSize;
-    }
+    // Override section fields if requested.
+    overrideFields<ELFT>(Sec, SHeader);
   }
 }
 
 static size_t findFirstNonGlobal(ArrayRef<ELFYAML::Symbol> Symbols) {
   for (size_t I = 0; I < Symbols.size(); ++I)
     if (Symbols[I].Binding.value != ELF::STB_LOCAL)
       return I;
   return Symbols.size();

llvm/lib/ObjectYAML/ELFYAML.cpp

static void commonSectionMapping(IO &IO, ELFYAML::Section &Section) {
   IO.mapOptional("Link", Section.Link, StringRef());
   IO.mapOptional("AddressAlign", Section.AddressAlign, Hex64(0));
   IO.mapOptional("EntSize", Section.EntSize);
 
   // obj2yaml does not dump these fields. They are expected to be empty when we
   // are producing YAML, because yaml2obj sets appropriate values for them
   // automatically when they are not explicitly defined.
   assert(!IO.outputting() ||
-         (!Section.ShOffset.hasValue() && !Section.ShSize.hasValue()));
+         (!Section.ShOffset.hasValue() && !Section.ShSize.hasValue() &&
+          !Section.ShName.hasValue() && !Section.ShFlags.hasValue()));
   IO.mapOptional("ShName", Section.ShName);
   IO.mapOptional("ShOffset", Section.ShOffset);
   IO.mapOptional("ShSize", Section.ShSize);
+  IO.mapOptional("ShFlags", Section.ShFlags);
 }
 
 static void sectionMapping(IO &IO, ELFYAML::DynamicSection &Section) {
   commonSectionMapping(IO, Section);
   IO.mapOptional("Entries", Section.Entries);
   IO.mapOptional("Content", Section.Content);
 }
 
if (auto S = dyn_cast<ELFYAML::RawContentSection>(Section.get()))
       sectionMapping(IO, *S);
     else
       sectionMapping(IO, *cast<ELFYAML::StackSizesSection>(Section.get()));
   }
 }
 
 StringRef MappingTraits<std::unique_ptr<ELFYAML::Chunk>>::validate(
     IO &io, std::unique_ptr<ELFYAML::Chunk> &C) {
-  if (const auto *RawSection =
-          dyn_cast<ELFYAML::RawContentSection>(C.get())) {
+  if (const auto *RawSection = dyn_cast<ELFYAML::RawContentSection>(C.get())) {
     if (RawSection->Size && RawSection->Content &&
         (uint64_t)(*RawSection->Size) < RawSection->Content->binary_size())
       return "Section size must be greater than or equal to the content size";
+    if (RawSection->Flags && RawSection->ShFlags)
+      return "ShFlags and Flags cannot be used together";
     return {};
   }
 
   if (const auto *SS = dyn_cast<ELFYAML::StackSizesSection>(C.get())) {
     if (!SS->Entries && !SS->Content && !SS->Size)
       return ".stack_sizes: one of Content, Entries and Size must be specified";
 
     if (SS->Size && SS->Content &&

llvm/test/tools/yaml2obj/ELF/override-shflags.yaml

+## Check we are able to set custom sh_flags field
+## for different sections.
+
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readobj --sections %t1 | FileCheck %s --check-prefix=UPDATE-FLAGS
+
+# UPDATE-FLAGS:      Name: .symtab
+# UPDATE-FLAGS-NEXT: Type: SHT_SYMTAB
+# UPDATE-FLAGS-NEXT: Flags [ (0x1)
+
+# UPDATE-FLAGS:      Name: .strtab
+# UPDATE-FLAGS-NEXT: Type: SHT_STRTAB
+# UPDATE-FLAGS-NEXT: Flags [ (0x2)
+
+# UPDATE-FLAGS:      Name: .shstrtab
+# UPDATE-FLAGS-NEXT: Type: SHT_STRTAB
+# UPDATE-FLAGS-NEXT: Flags [ (0x3)
+
+# UPDATE-FLAGS:      Name: .dynsym
+# UPDATE-FLAGS-NEXT: Type: SHT_DYNSYM
+# UPDATE-FLAGS-NEXT: Flags [ (0x4)
+
+# UPDATE-FLAGS:      Name: .dynstr
+# UPDATE-FLAGS-NEXT: Type: SHT_STRTAB
+# UPDATE-FLAGS-NEXT: Flags [ (0x5)
+
+# UPDATE-FLAGS:      Name: .regular.os
+# UPDATE-FLAGS-NEXT: Type: SHT_PROGBITS
+# UPDATE-FLAGS-NEXT: Flags [ (0xAB00000)
+
+# UPDATE-FLAGS:      Name: .regular.proc
+# UPDATE-FLAGS-NEXT: Type: SHT_PROGBITS 
+# UPDATE-FLAGS-NEXT: Flags [ (0xC0000000)
+
+# UPDATE-FLAGS:      Name: .regular.all
+# UPDATE-FLAGS-NEXT: Type: SHT_PROGBITS
+# UPDATE-FLAGS-NEXT: Flags [ (0xFFFFFFFF)
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+## Check we can override flags for implicit sections.
+  - Name: .symtab
+    Type: SHT_SYMTAB
+    ShFlags: 0x000000001
+  - Name: .strtab
+    Type: SHT_STRTAB
+    ShFlags: 0x000000002
+  - Name: .shstrtab
+    Type: SHT_STRTAB
+    ShFlags: 0x000000003
+  - Name: .dynsym
+    Type: SHT_DYNSYM
+    ShFlags: 0x000000004
+  - Name: .dynstr
+    Type: SHT_STRTAB
+    ShFlags: 0x000000005
+## Check we can override flags for regular sections.
+  - Name: .regular.os
+    Type: SHT_PROGBITS
+    ShFlags: 0x0AB00000
+  - Name: .regular.proc
+    Type: SHT_PROGBITS
+    ShFlags: 0xC0000000
+  - Name: .regular.all
+    Type: SHT_PROGBITS
+    ShFlags: 0xFFFFFFFF
+
+## ShFlags is used when we want to workaround the flag values validation.
+## It does not make sense to allow both ShFlags and Flags in the document.
+
+# RUN: not yaml2obj --docnum=2 %s 2>&1 | FileCheck %s --check-prefix=ERR
+
+# ERR: error: ShFlags and Flags cannot be used together
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .symtab
+    Type: SHT_SYMTAB
+    Flags: [ SHF_ALLOC ]
+    ShFlags: 0x1