This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] - Make the 'Machine' key optional.
ClosedPublic

Authored by grimar on Aug 19 2020, 3:02 AM.

Details

Summary

Currently we have to set Machine to something in our
YAML descriptions. Usually we use EM_X86_64 for 64-bit targets
and EM_386 for 32-bit targets. At the same time, in fact, in most
cases our tests do not need a machine type and we can use
EM_NONE.

This is cleaner, because avoids the need of using a particular machine.

In this patch I've made the Machine key optional (the default value,
when it is not specified is EM_NONE) and removed it (where possible)
from yaml2obj, obj2yaml and llvm-readobj tests.

There are few tests left where I decided not to remove it, because
I didn't want to touch CHECK lines or doing anything more complex
than a removing a "Machine: *" line and formatting lines around.

I also though about possible splitting this into 2 patches:
one could do the code change and adjust yaml2obj/obj2yaml tests and
another could do the job for llvm-readobj test cases.

Diff Detail

Event Timeline

grimar created this revision.Aug 19 2020, 3:02 AM
grimar requested review of this revision.Aug 19 2020, 3:02 AM
grimar edited the summary of this revision. (Show Details)
grimar retitled this revision from [yaml2obj] - Make 'Machine' key optional. to [yaml2obj] - Make the 'Machine' key optional..Aug 19 2020, 3:06 AM
jhenderson accepted this revision.Aug 19 2020, 3:56 AM

LGTM!

llvm/include/llvm/ObjectYAML/ELFYAML.h
557

You can probably just call this getMachine.

llvm/test/tools/obj2yaml/ELF/duplicate-symbol-and-section-names.yaml
30

Should we (in a separate change) only emit the Machine line if we are not EM_NONE?

This revision is now accepted and ready to land.Aug 19 2020, 3:56 AM
grimar updated this revision to Diff 286535.Aug 19 2020, 4:35 AM
grimar marked an inline comment as done.
  • Addressed review comments.
llvm/include/llvm/ObjectYAML/ELFYAML.h
557

Done.

llvm/test/tools/obj2yaml/ELF/duplicate-symbol-and-section-names.yaml
30

I do not expect to see many EM_NONE objects in the wild (I guess it is a very rare case?),
but probably we might want to do it to reduce the possible noise when obj2yaml is used with our objects from test cases,
what is probably will be a much less rare case after this change. So yes, I think it is reasonable.

LGTM!

Thanks, James! I'll hold this a little bit to see there are no objections from others.

This revision was automatically updated to reflect the committed changes.