This is an archive of the discontinued LLVM Phabricator instance.

[llvm/test/Object] - Cleanup and move out the yaml2obj tests.
ClosedPublic

Authored by grimar on Aug 2 2019, 6:24 AM.

Details

Summary

There are multiple yaml2obj-* tests in llvm/test/Object
folder. This is not correct place to have them and my intention
was to move them out to test\tools\yaml2obj folder. I reviewed
them, made some changes, and my comments are below.

For all tests I:

  • Added comments when needed.
  • Moved them from llvm/test/Object to yaml2obj tests.

Another changes performed:

  1. yaml2obj-invalid.yaml. It was a test for an invalid YAML input.

I just moved it.

  1. yaml2obj-coff-multi-doc.test/yaml2obj-elf-multi-doc.test:

these were a tests for testing --docnum=x functionality,
one was for COFF and one for ELF. I merged them into one.

  1. yaml2obj-elf-bits-endian.test:

I removed its 4 YAML inputs (merged into the main test).

  1. yaml2obj-readobj.test:

This file has a long history. It was added to check the
"parsing of header charactestics" initially. Then was used to test
how yaml2obj writes the relocations. Then was upgraded to check how
yaml2obj handle "-o" option. I think it should be heavily splitted
and refactored in a separate patch. For now I leaved it as is, but restyled
to reduce the changes in a follow-ups.

  1. yaml2obj-elf-alignment.yaml: its intention was to check we

can set sh-addralign field. I moved, renamed (to elf-sh-addralign.yaml)
and updated this test.

  1. yaml2obj-elf-file-headers.yaml: I removed it.

It's intention was to check that
yaml2obj handles OS/ABI and ELF type (e.g Relocatable).
We are testing this already, for example in D64800. We might want
to add a better (more complete) test, but keeping the existent test
does not have much sense I think.

  1. yaml2obj-elf-file-headers-with-e_flags.yaml: I would describe its intention

as "testing MIPS e_flags". It is far from being complete and tests only
a few flags. I leaved it alone for now.

  1. yaml2obj-elf-rel.yaml: its intention is to check the MIPS32 relocations.

We have a version for MIPS64 here: test\Object\Mips\elf-mips64-rel.yaml
Seems them both are incomplete. I leaved them alone for now.

  1. yaml2obj-elf-rel-noref.yaml: was introduced to check the support of arm32

R_ARM_V4BX relocatiion. I leaved it alone for now.

  1. yaml2obj-elf-section-basic.yaml: it just checked that we are able to recognise

trivial fields like section 'Name', 'Type', 'Flags' and others. All of our yaml2obj
tests are heavily using it. I just removed this test.

  1. yaml2obj-elf-section-invalid-size.yaml: its intention was to check the

"Section size must be greater than or equal to the content size" error.
I moved this test to `tools\yaml2obj\section-size-content.yaml'

  1. yaml2obj-elf-symbol-basic.yaml: its intention seems was to support declarations

of the symbols in yaml2obj. I removed it. We use this in almost each test we already have.

  1. yaml2obj-elf-symbol-LocalGlobalWeak.yaml: its intention was to check that we can

declare different symbol bindings. I moved it to tools\yaml2obj\elf-symbol-binding.yaml.

  1. yaml2obj-coff-invalid-alignment.test: check that error is reported for a too large coff

section alignment. Moved it to tools\yaml2obj\coff-invalid-alignment.test

  1. yaml2obj-elf-symbol-visibility.yaml: tests ELF symbols visibility. I improved it and

moved to tools\yaml2obj\elf-symbol-visibility.yaml and tools\obj2yaml\elf-symbol-visibility.yaml

Diff Detail

Event Timeline

grimar created this revision.Aug 2 2019, 6:24 AM

This is not correct place to have them and my intention was to move them out to test\tools\yaml2obj folder.

\ -> / to be consistent :)

yaml2obj-coff-multi-doc.test/yaml2obj-elf-multi-doc.test: these were a tests for testing --docnum=x functionality

were a -> were

test/Object/yaml2obj-readobj.test
1

Have you found --expand-relocs useful? I personally think they are gratuitously verbose but do not improve readability...

78

Align the values

test/tools/obj2yaml/elf-symbol-visibility.yaml
3

As the only test --check-prefix YAML is redundant.

test/tools/yaml2obj/elf-class-endianness.test
1

I think a can be deleted but I'm not very sure..

test/tools/yaml2obj/elf-symbol-binding.yaml
1

the different binding -> different bindings

test/tools/yaml2obj/elf-symbol-visibility.yaml
1

correctly may not convey more information.. (I think correctness is implied, otherwise the tests should be marked FIXME XFAIL etc..)

grimar updated this revision to Diff 213328.Aug 5 2019, 5:01 AM
grimar marked 6 inline comments as done.
  • Addressed review comments.
test/Object/yaml2obj-readobj.test
1

I only moved this test (and inlined the input). If you do not mind, I would not change the flags used here in this patch.

A citation from the description (it mentioned yaml2obj-readelf.test though, not yaml2obj-readobj.test, by mistake):

This file has a long history. It was added to check the
"parsing of header charactestics" initially. Then was used to test
how yaml2obj writes the relocations. Then was upgraded to check how
yaml2obj handle "-o" option. I think it should be heavily splitted
and refactored in a separate patch. For now I leaved it as is, but restyled
to reduce the changes in a follow-ups.

I.e. I think this one should be heavily refactored.

test/tools/yaml2obj/elf-class-endianness.test
1

Sometimes I guess my English teacher isn't sure when I show her the comments from reviews..
I am glad we have at least one guy here around who knows such kind of things.. :]

grimar edited the summary of this revision. (Show Details)Aug 5 2019, 6:49 AM
MaskRay accepted this revision.Aug 5 2019, 6:55 PM

This is mostly file movement, with some fixups. I think it is fine to leave more refactoring into future changes. It would be convenient if Phabricator knew renamed files...

This revision is now accepted and ready to land.Aug 5 2019, 6:55 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 1:01 AM
test/Object/yaml2obj-elf-symbol-basic.yaml