This is an archive of the discontinued LLVM Phabricator instance.

[mips] [IAS] Add missing constraints and improve testing for the .module directive.
ClosedPublic

Authored by tomatabacu on Jan 23 2015, 3:34 AM.

Details

Summary

None of the .set directives can be used before the .module directives. The .set mips0/pop/push were not triggering this constraint.
Also added testing for all the other implemented directives which are supposed to trigger this constraint.

Diff Detail

Event Timeline

tomatabacu updated this revision to Diff 18655.Jan 23 2015, 3:34 AM
tomatabacu retitled this revision from to [mips] Forbid the usage of .module after emitting .set mips0,pop,push..
tomatabacu updated this object.
tomatabacu edited the test plan for this revision. (Show Details)
tomatabacu added a reviewer: dsanders.
tomatabacu added a subscriber: Unknown Object (MLST).
tomatabacu updated this revision to Diff 18661.Jan 23 2015, 5:24 AM

Moved tests to mips-abi-bad.s.
Added test for .set pop.

tomatabacu updated this revision to Diff 19469.Feb 6 2015, 3:02 AM

Add more comprehensive test.

tomatabacu retitled this revision from [mips] Forbid the usage of .module after emitting .set mips0,pop,push. to [mips] [IAS] Add missing constraints and improve testing for the .module directive..Feb 18 2015, 5:37 AM
tomatabacu updated this object.
dsanders edited edge metadata.Feb 26 2015, 3:35 AM

The code change looks correct to me but I think there's a problem with the test.

test/MC/Mips/module-set-directives-bad.s
1–134 ↗(On Diff #19469)

I don't think this test covers what you think it does. It looks like it's only testing that '.set mips0' triggers the errors. All the subsequent errors will appear as a consequence of '.set mips0' regardless of whether those directives do the right thing.

I think we need a special directive for testing purposes to reset the flag in between each test (.llvm_internal_reallow_module_directive?). That way we can reset the flag between each subtest. The alternative is to have a separate file (and associated process overhead) for each subtest which seems unreasonable.

tomatabacu updated this revision to Diff 21091.Mar 3 2015, 3:07 AM
tomatabacu edited edge metadata.

Added internal directive for resetting .module, as suggested in the review.
Added missing call to forbidModuleDirective in emitDirectiveSetFp.
Removed CHECKs for .set macro/nomacro because they don't get emitted at the moment.
Changed the name of the test file to a more appropriate one.

dsanders accepted this revision.Mar 4 2015, 5:03 AM
dsanders edited edge metadata.

LGTM with the CHECK-NOT's added

test/MC/Mips/module-directive-bad.s
9 ↗(On Diff #21091)

You ought to have a CHECK-NOT on each of these to ensure the error doesn't trigger.

This revision is now accepted and ready to land.Mar 4 2015, 5:03 AM
tomatabacu updated this revision to Diff 21340.Mar 6 2015, 3:15 AM
tomatabacu edited edge metadata.

Added CHECK-NOTs.

tomatabacu closed this revision.Mar 6 2015, 4:17 AM