[yaml2obj] - Teach tool to produce SHT_GROUP section with a custom type.

[grimar]

Summary

Currently, it is possible to use yaml2obj for producing SHT_GROUP sections
of type GRP_COMDAT. For LLD test case I need to produce an object with
a broken (different from GRP_COMDAT) type.

The patch teaches tool to do such things.

[jhenderson]

Is it normal for yaml2obj to only parse hex values and not decimal for other fields?

test/tools/yaml2obj/elf-comdat-broken.yaml

30 ↗

(On Diff #160761)

Maybe it's worth having two invalid values, so that the second one is interpreted as a section index? What do you think?

tools/yaml2obj/yaml2elf.cpp

541 ↗

(On Diff #160761)

Nit: and about -> and are about
resort - try to parse hex value as section index. -> resort, try to parse as a hex value.

[grimar]

Is it normal for yaml2obj to only parse hex values and not decimal for other fields?

I think usually it allows any numeric values, but I think it is reasonable for this place to allow only hex values.
Here we have a field that expect either section name or named constant. For example, we have the following ones:

enum : unsigned {

GRP_COMDAT = 0x1,
GRP_MASKOS = 0x0ff00000,
GRP_MASKPROC = 0xf0000000

};

(http://www.sco.com/developers/gabi/2001-04-24/ch4.sheader.html#section_group_flags)

So I see why we might want to use section name, the name of the constant and (now) hex value of constant,
but I am not sure it worth to support decimals here. If that field would accept only numeric values, then it would
make much more sense.

test/tools/yaml2obj/elf-comdat-broken.yaml

30 ↗

(On Diff #160761)

It is problematic to have the second value to be invalid because llvm-readobj refuses to parse the Groups then:
"Error reading file: invalid section index".

I added one more valid hex value though.

[grimar]

• Addressed comments.

[grimar]

After one more look, since the field name is SectionOrType, I think it is ok to accept decimals here too for specifying a section index.
It will also simplify the implementation a bit. I'll update the patch.

[jhenderson]

LGTM, with nit. I guess if somebody wants decimal support later, they can add it then.

test/tools/yaml2obj/elf-comdat-broken.yaml

6–20 ↗

(On Diff #160765)

Nit: There's an awful lot of extra (and inconsistent) whitespace between the field names and values. It would be nice to make it more consistent (either only a single space, or for all the blocks to be indented to a minimum level such that they all line up with each other e.g:

Class:   ELFCLASS64
Data:    ELFDATA2LSB
Type:    ET_REL
Machine: EM_X86_64

[jhenderson]

Oops, hang on, didn't see your change as I wrote my review...

[jhenderson]

In D50761#1200428, @grimar wrote:

After one more look, since the field name is SectionOrType, I think it is ok to accept decimals here too for specifying a section index.
It will also simplify the implementation a bit. I'll update the patch.

Okay, I'll wait for your update.

[grimar]

• Updated in according to the discussion.

[jhenderson]

LGTM.

test/tools/yaml2obj/elf-comdat-broken.yaml

17 ↗

(On Diff #160775)

Maybe you should make this a decimal?

[grimar]

Thanks for the review, James!

test/tools/yaml2obj/elf-comdat-broken.yaml

17 ↗

(On Diff #160775)

Yep.

llvm/trunk/test/tools/yaml2obj/elf-comdat-broken.yaml

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj -sections -elf-section-groups %t | FileCheck %s
+
+--- !ELF
+FileHeader:
+  Class:               ELFCLASS64
+  Data:                ELFDATA2LSB
+  Type:                ET_REL
+  Machine:             EM_X86_64
+Sections:
+  - Name:              .group
+    Type:              SHT_GROUP
+    Link:              .symtab
+    Info:              foo
+    Members:
+      - SectionOrType: 0xFF
+      - SectionOrType: 3
+Symbols:
+  Global:
+    - Name:            foo
+
+## Check we are able to produce SHT_GROUP section with a custom Type (0xFF).
+# CHECK:       Groups {
+# CHECK-NEXT:   Group {
+# CHECK-NEXT:     Name: .group
+# CHECK-NEXT:     Index: 1
+# CHECK-NEXT:     Link: 2
+# CHECK-NEXT:     Info: 1
+# CHECK-NEXT:     Type: COMDAT (0xFF)
+# CHECK-NEXT:     Signature: foo
+# CHECK-NEXT:     Section(s) in group [
+# CHECK-NEXT:       .strtab (3)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }

llvm/trunk/tools/yaml2obj/yaml2elf.cpp

bool ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
 
   auto &OS = CBA.getOSAndAlignedOffset(SHeader.sh_offset, SHeader.sh_addralign);
 
   for (auto member : Section.Members) {
     Elf_Word SIdx;
     unsigned int sectionIndex = 0;
     if (member.sectionNameOrType == "GRP_COMDAT")
       sectionIndex = llvm::ELF::GRP_COMDAT;
-    else if (SN2I.lookup(member.sectionNameOrType, sectionIndex)) {
+    else if (SN2I.lookup(member.sectionNameOrType, sectionIndex) &&
+             !to_integer(member.sectionNameOrType, sectionIndex)) {
       WithColor::error() << "Unknown section referenced: '"
                          << member.sectionNameOrType << "' at YAML section' "
                          << Section.Name << "\n";
       return false;
     }
     SIdx = sectionIndex;
     OS.write((const char *)&SIdx, sizeof(SIdx));
   }