Page MenuHomePhabricator

[ARM] Fix recent breakage of -mfpu=none.

Authored by simon_tatham on May 31 2019, 3:54 AM.



The recent change D60691 introduced a bug in clang when handling
option combinations such as -mcpu=cortex-m4 -mfpu=none. Those
options together should select Cortex-M4 but disable all use of
hardware FP, but in fact, now hardware FP instructions can still be
generated in that mode.

The reason is because the handling of FPUVersion::NONE disables all
the same feature names it used to, of which the base one is vfp2.
But now there are further features below that, like vfp2d16fp and
(following D60694) fpregs, which also need to be turned off to
disable hardware FP completely.

Added a tiny test which double-checks that compiling a simple FP
function doesn't access the FP registers.

Diff Detail


Event Timeline

simon_tatham created this revision.May 31 2019, 3:54 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 31 2019, 3:54 AM
lebedev.ri added inline comments.
2 ↗(On Diff #202413)

Generally clang codegen tests should test -emit-llvm -S output

dmgreen added inline comments.May 31 2019, 4:08 AM
2 ↗(On Diff #202413)

Perhaps add to the tests in clang/test/Driver/arm-mfpu.c instead?

simon_tatham marked an inline comment as done.May 31 2019, 4:16 AM
simon_tatham added inline comments.
2 ↗(On Diff #202413)

@lebedev.ri : looking at the IR doesn't show up what I'm trying to test in this case. And I modelled this on existing tests in test/CodeGen, such as arm-eabi.c, which also need to test the real assembler output in order to do their job.

@dmgreen: it's true that I could also make test/Driver/arm-mfpu.c check that the extra feature name is appearing in the cc1 command line where I currently think it should be, but I think that wouldn't replace this test. The real point of this test is that the currently right feature names might not stay right, if the feature set is reorganised again in future – and if that happens, then whoever does it will probably rewrite most of arm-mfpu.c anyway.

So this is an end-to-end check that whatever detailed command line is generated by the driver in these circumstances, it is sufficient to ensure that the final generated code really doesn't include FP instructions.

dmgreen added inline comments.May 31 2019, 6:21 AM
2 ↗(On Diff #202413)

I believe that the idea is that if we test each step, so make sure we have tests from clang driver -> clang-cl and clang -> llvm-ir and llvm -> asm, then it should all be tested. And because each has a well defined interfaces along each step of the way it should keep working. Any integration tests are often left to external tests like the test suite.

But I do see some precedent for this in other places. If you add the extra checks in test/Driver/arm-mfpu.c, then I think this is fine. (And I guess if someone objects we can always take it out :) )

simon_tatham marked an inline comment as done.Jun 3 2019, 2:02 AM
simon_tatham added inline comments.
2 ↗(On Diff #202413)

I think the shortcomings of that step-by-step testing strategy are exactly what caused the breakage in the first place.

If you know you're changing the mapping from driver arguments to cc1 feature options, then you change the test that obviously goes with the change you just made; but you leave unchanged the test that says the previous set of cc1 options caused the behaviour you wanted – because nothing will remind you that that needs changing as well (or make you aware that that test even exists to be updated, if you didn't already know).

An end-to-end test that ensures that this user input continues to cause that ultimate output code, regardless of the intervening details, would have prevented this bug arising last week, where the other strategy didn't. (Indeed, an end-to-end test of exactly that kind in our downstream tests is how we spotted it shortly after the change landed.)

So I still think I'd prefer not to remove this end-to-end test (though I will if people absolutely insist). But I'll add those extra checks you mention in arm-mfpu.c.

Added the extra checks in arm-mfpu.c.

dmgreen accepted this revision.Jun 3 2019, 3:47 AM

LGTM. Thanks for adding the new test. I agree with keeping both tests ( I wasn't very clear).

This revision is now accepted and ready to land.Jun 3 2019, 3:47 AM
Closed by commit rL362380: [ARM] Fix recent breakage of -mfpu=none. (authored by statham, committed by ). · Explain WhyJun 3 2019, 4:04 AM
This revision was automatically updated to reflect the committed changes.