This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj][test] - Move platform specific test cases and their inputs to separate folders.
ClosedPublic

Authored by grimar on Dec 9 2019, 6:28 AM.

Details

Summary

This creates the next subfolders in the test directory:
"COFF", "ELF", "MachO", "WASM".

I've also removed platform specific prefixes, like "coff-*".

One unused binary was removed as well: Inputs/relocs.obj.elf-mips

Diff Detail

Event Timeline

grimar created this revision.Dec 9 2019, 6:28 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar edited the summary of this revision. (Show Details)Dec 9 2019, 6:30 AM
jhenderson accepted this revision.Dec 9 2019, 7:02 AM

This change makes me very happy, LGTM.

I think an obvious follow-up would be to split the multi-format tests up into separate tests.

I wonder how many holes in test coverage for different formats this shows up? That could be an interesting follow-on piece of work (not asking you to do it, just an observation).

llvm/test/tools/llvm-objdump/elf-dynamic-section-machine-specific.test
3

The fact that this test relies on an input in llvm-readobj seems a bit bizarre to me, frankly!

This revision is now accepted and ready to land.Dec 9 2019, 7:02 AM
sbc100 added a comment.Dec 9 2019, 8:43 AM

Would mind calling the WebAssembly subdirectory just "wasm". I know we currently have a bit of a mix of WebAssembly, wasm and Wasm in the codebase. I'm hoping to reduce this to just WebAssembly (for the target arch) and wasm (for the container format).

MaskRay accepted this revision.Dec 9 2019, 10:41 AM

LGTM once the WASM -> wasm rename as suggested by @sbc100 is done.

Would mind calling the WebAssembly subdirectory just "wasm". I know we currently have a bit of a mix of WebAssembly, wasm and Wasm in the codebase. I'm hoping to reduce this to just WebAssembly (for the target arch) and wasm (for the container format).

But there is no "WebAssembly" subdirectory in this patch :) it has the following structure:

test\tools\llvm-readobj:

--> COFF
        |
        ---> Inputs

--> ELF
       |
       ---> AArch64
       |
       ---> ARM
       |
       ---> Inputs

--> MachO
        |
        ---> Inputs

--> WASM
        |
        ---> Inputs

I'll rename WASM to wasm.

grimar marked an inline comment as done.Dec 9 2019, 11:29 PM

This change makes me very happy, LGTM.

I think an obvious follow-up would be to split the multi-format tests up into separate tests.

Yeah, that is my plan.

llvm/test/tools/llvm-objdump/elf-dynamic-section-machine-specific.test
3

At least it is very inconsistent. Doesn't seem we do that in other tests.
(and having many cross references would be a big mess probably!)
It lives in "ELF/Inputs" which is my target for cleanup, so I think it will gone soon.

This revision was automatically updated to reflect the committed changes.
llvm/test/tools/llvm-readobj/Inputs/relocs.obj.elf-mips