This is an archive of the discontinued LLVM Phabricator instance.

[ARM] More rigorous testing of build attribute encoding/decoding
ClosedPublic

Authored by chatur01 on Nov 19 2014, 4:29 AM.

Details

Reviewers
t.p.northover
Summary

Hi,

The current state of the world for build attribute testing is as follows,

  • test/MC/ARM/directive-eabi_attribtues.s -- this file tests that, for a small subset of <Attribute, Value> pairs, attributes are successfully encoded / decoded into / from assembly and object files.
  • test/tools/llvm-readobj/ARM/attributes.s -- this file tests that, for a smaller subset of <Attribute, Value> pairs, attributes are successfully encoded / decoded into / from object files.
  • test/CodeGen/ARM/build-attributes.ll -- Tests that the build attributes are correctly emitted in default cases, and when various command line options are specified (CPU, target attributes, etc)

The test/tools/llvm-readobj/ARM/attributes.s is redundant given that it tests an equally small (though different) subset of build attributes that the test/MC/ARM/directive-eabi_attribtues.s test tests. For ABI conformance, we should be testing every <Attribute, Value> pair, where feasible.

The description fields of tags are not being tested in this patch whereas they were being tested in test/tools/llvm-readobj/ARM/attributes.s (the removed file). I don't think it is useful to test the hard-coded strings in tools/llvm-readobj/ARMAttributeParser.cpp. As an aside, the strings are occasionally cryptic abbreviations for the actual descriptions in the ABI document. If these descriptions are going to be tested, they should be the descriptions verbatim out of the document IMO, not arbitrary abbreviations. I think this would be best left as a separate patch however.

The attached patch contains 163 separate test files. It would have been cleaner to bundle all tests into a single file and use conditional compilation to test each <Attribute, Value> pair, but I was unable to find a way of doing this with the LLVM driver tools. This is required because if you try and encode the same build attribute with two or more different values in the same input file, the object file emitter ignores all but the last specified build attribute.

Thank you for your time,
Charlie.

Diff Detail

Event Timeline

chatur01 retitled this revision from to [ARM] More rigorous testing of build attribute encoding/decoding.
chatur01 updated this object.
chatur01 edited the test plan for this revision. (Show Details)
chatur01 set the repository for this revision to rL LLVM.
chatur01 added a subscriber: Unknown Object (MLST).

Hi Charlie,

The description fields of tags are not being tested in this patch whereas they were being tested in test/tools/llvm-readobj/ARM/attributes.s (the removed file). I don't think it is useful to test the hard-coded strings in tools/llvm-readobj/ARMAttributeParser.cpp.

I disagree with this. Or at least I think that functionality should exist somewhere (I don't want to need the ELF ABI reference open just to tell if the attributes are sane) and if it is present it should be tested. Comments during normal asm emission would be nice.

Other than that, the tests are .s files rather than .ll.

I'm also a bit worried by all the extra processes this will spawn. How about splitting them by value (at least the numeric ones): attributes-0.s, attributes-1.s etc?

Cheers.

Tim.

chatur01 updated this revision to Diff 16437.EditedNov 20 2014, 9:03 AM

Address Tim's comments.

  • I have rearranged the tests into files such as attribute-n.s, which contains tests for attributes which can take on value n.
  • I am now testing the descriptions of these attributes as well. My point was that I feel you still need the ABI document with some of these abbreviations because they can be a tad cryptic. I do concede that's no excuse to ignore testing what we have however :)

This patch depends on D6341 getting being committed first.

Thank you very much for your review Tim!

Hi Charlie,

Just spotted one potential issue looking through the tests:

test/tools/llvm-readobj/ARM/attribute-136.s
4–7

What's doing the capitalisation here? It seems highly suspect (not mentioned in the ABI as far as I can tell, and gets into sticky locale questions).

chatur01 set the repository for this revision to rL LLVM.

Now that D6356 has been committed (stop upper casing string data in object files) this is up for review again.

The tests have not changed apart from those which tested the string values being upper cased. They test that the case is preserved.

This patch also adds tests for D6341 that was recently committed.

t.p.northover accepted this revision.Nov 27 2014, 10:09 AM
t.p.northover added a reviewer: t.p.northover.

Hi Charlie,

I think this looks fine now. Thanks for working on fixing the issues.

Tim.

This revision is now accepted and ready to land.Nov 27 2014, 10:09 AM

I think this looks fine now. Thanks for working on fixing the issues.

Thanks very much for helping me with this patch Tim! Committed as r222917.

test/tools/llvm-readobj/ARM/attribute-R.s