This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj/elf][test] - Test all core note types properly.
ClosedPublic

Authored by grimar on Sep 10 2020, 7:07 AM.

Details

Summary

Currently we don't test all core note types that are defined in
getCoreNoteTypeName in ELFDumper.cpp.

Also we don't have a test for an unknown core note type.

This patch fixes it.

Diff Detail

Event Timeline

grimar created this revision.Sep 10 2020, 7:07 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar requested review of this revision.Sep 10 2020, 7:07 AM

Emm. I am a bit less sure about the value enumerating every value. It just repeats the getCoreNoteTypeName list in the test file. Two or three values should work, right?

llvm/test/tools/llvm-readobj/ELF/note-core.test
3

Not consistent with other Check where a period is present

Emm. I am a bit less sure about the value enumerating every value. It just repeats the getCoreNoteTypeName list in the test file. Two or three values should work, right?

I believe all values should be tested at least somewhere. We had a situation previously (for file format names) where we tested all values in lib/Object unit test (ELFObjectFileTest.cpp)
and that allowed use to test just a few values in llvm-readelf test. But this case is different - note types are defined in llvm-readelf code.

This is the same what we do for other things. E.g. we normally test all possible relocations, dynamic tags, etc.

jhenderson accepted this revision.Sep 14 2020, 3:18 AM

I think there's probably some value testing every value, as there is a chance that a future refactor could lead to a bug where there is ambiguity between a CORE note value and another variety of note, for example. Having a test for each individual value ensures that any such bug would be picked up. However, I'm not dead set on this idea, and can see counter-arguments to it. LGTM, but happy to defer to others opinion on whether testing each value is atually necessary.

If you haven't already, could you run this test on Windows and make sure it doesn't take an unreasonable amount of time (since there are a lot of process executions, which are expensive on Windows), please. In theory, the test could be written with a single large input testing each individual value. This would make the test check/input bigger, but would at least avoid 150 or so process executions within a single test.

This revision is now accepted and ready to land.Sep 14 2020, 3:18 AM

If you haven't already, could you run this test on Windows and make sure it doesn't take an unreasonable amount of time (since there are a lot of process executions, which are expensive on Windows), please. In theory, the test could be written with a single large input testing each individual value. This would make the test check/input bigger, but would at least avoid 150 or so process executions within a single test.

It takes 1.63s in debug configuration for me. (i7-4790K@4.2 Ghz, windows 8.1 64, 32 Gb Ram, SSD).

-- Testing: 1 tests, 1 workers --
PASS: LLVM :: tools/llvm-readobj/ELF/note-core.test (1 of 1)

Testing Time: 1.63s
  Passed: 1

Is it OK or too much?

Is it OK or too much?

I think it's probably okay.

This revision was automatically updated to reflect the committed changes.
grimar marked an inline comment as done.