This is an archive of the discontinued LLVM Phabricator instance.

[Object/invalid.test] - Convert most of the sub tests to YAML.
ClosedPublic

Authored by grimar on Jun 25 2019, 5:03 AM.

Details

Summary

Object/invalid.test is a test case that is used to check the behavior of tools
when broken inputs are used.

The most often tool tested there is llvm-readobj. I think we might want to move
such tests to test\tools\llvm-readobj. For now this patch converts
many sub-tests to use YAML and removes 12 binaries from the inputs.

I think there is no clean way to convert other sub-tests at this moment
(yaml2obj isn't ready for that yet)

Diff Detail

Event Timeline

grimar created this revision.Jun 25 2019, 5:03 AM

FWIW, I think this test is generally testing the behaviour of libObject code, so arguably belongs exactly where it is.

test/Object/invalid.test
36

Comment?

39

relocations -> relocation

55

I think this requires a comment explaining the contents. As things stand, it's about as opaque as a canned binary.

63

Double blank-line?

65

when tries -> when it tries

same below

126

How about INVALID-DYNSYM-SIZE?

189

Comments for these test cases?

209

if a relocation contains an

grimar updated this revision to Diff 206624.Jun 26 2019, 3:52 AM
grimar marked 8 inline comments as done.
grimar edited the summary of this revision. (Show Details)
  • Addressed review comments.
  • Converted 3 more test cases.
  • Now depends on D63771.
jhenderson added inline comments.Jun 26 2019, 4:51 AM
test/Object/invalid.test
36–37

I'd rephrase this slightly:

"... has an invalid size that goes past the end of the file."

60

relocation, idea -> relocation. The idea

61–62

on a non-SHT_RELA

149

has an invalid

168

tries -> trying

225

if the e_phentsize

240

if a relocation

241

has a broken sh_offset (past the end of the file)

(I think that would be clearer)

258

tries -> trying

259

It's probably worth explaining how it is broken.

274

You missed the "a" between "if" and "relocation": should be "if a relocation..."

grimar updated this revision to Diff 206631.Jun 26 2019, 5:11 AM
grimar marked 11 inline comments as done.
  • Addressed review comments.

Something odd has happened with the diff in this patch. It looks like you might have created it against the wrong thing?

test/Object/invalid.test
129

"has an invalid" here and everywhere use using "has invalid"

259

than the actual

test/tools/yaml2obj/elf-header-sh-fields.yaml
4

I think "First we..." was better.

grimar updated this revision to Diff 206639.EditedJun 26 2019, 5:33 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.

Something odd has happened with the diff in this patch. It looks like you might have created it against the wrong thing?

Right.. It seems I forgot to rebase it on top of the newest D63771 and created it against the old version of it. Fixed.

This revision is now accepted and ready to land.Jun 26 2019, 5:59 AM
MaskRay accepted this revision.Jun 26 2019, 6:24 AM
MaskRay added inline comments.
test/Object/invalid.test
43

during relocation resolution when instead of expected

This sentence seems ill-formed.

grimar marked an inline comment as done.Jun 26 2019, 6:32 AM
grimar added inline comments.
test/Object/invalid.test
43

I do not see a problem with it honestly. It sounds OK to me. Any suggestions how to change it?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2019, 4:33 AM