This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by grimar on Aug 15 2018, 3:46 AM.

Details

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.

Diff Detail

Event Timeline

grimar created this revision.Aug 15 2018, 3:46 AM

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

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

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

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

grimar marked an inline comment as done.Aug 15 2018, 4:18 AM

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

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 updated this revision to Diff 160765.Aug 15 2018, 4:19 AM
  • Addressed comments.

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 accepted this revision.Aug 15 2018, 4:34 AM

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

test/tools/yaml2obj/elf-comdat-broken.yaml
7–21

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
This revision is now accepted and ready to land.Aug 15 2018, 4:34 AM

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

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 this revision to Diff 160775.Aug 15 2018, 4:38 AM
  • Updated in according to the discussion.
jhenderson accepted this revision.Aug 15 2018, 4:40 AM

LGTM.

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

Maybe you should make this a decimal?

Thanks for the review, James!

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

Yep.

This revision was automatically updated to reflect the committed changes.