Page MenuHomePhabricator

[ARM] Fix bug 39982 - pcs("aapcs-vfp") is not consistent
ClosedPublic

Authored by carwil on Mar 7 2019, 7:16 AM.

Details

Summary

See: https://bugs.llvm.org/show_bug.cgi?id=39982

When considering how to classify homogeneous aggregates as return/argument types, the ABI of the function (specified by attribute pcs) wasn't being taken into account. This resulted in some weird and unexpected hybrid assembly when compiling with softfp.

Diff Detail

Repository
rC Clang

Event Timeline

carwil created this revision.Mar 7 2019, 7:16 AM

It looks like every call to getABIKind() is doing the wrong thing; I'm worried that's going to lead to some sort of subtle miscompile without some sort of centralized fix to compute the correct calling convention for every place that checks it. Not sure how to write a testcase off the top of my head... have you tried homogeneous aggregates with more than two elements?

clang/test/CodeGenCXX/arm-pcs.cpp
4 ↗(On Diff #189714)

This test needs "// REQUIRES: arm-registered-target" so it's automatically disabled if the ARM target isn't available.

carwil added a comment.EditedMar 8 2019, 2:58 AM

Not sure how to write a testcase off the top of my head... have you tried homogeneous aggregates with more than two elements?

Good catch! Seems like if we surpass "isHomogeneousAggregateSmallEnough" (4), it's no longer classified as an aggregate and so doesn't benefit from this fix. I'll see if I can work out a way to catch that as well.

It looks like every call to getABIKind() is doing the wrong thing; I'm worried that's going to lead to some sort of subtle miscompile without some sort of centralized fix to compute the correct calling convention for every place that checks it.

Agreed. It would be much better if this determined the ABI on context. That said, it doesn't look like that's going to be trivial to achieve...

carwil updated this revision to Diff 190653.Mar 14 2019, 9:48 AM
carwil marked an inline comment as done.

I got bit a confused earlier. That does actually seem like correct behaviour (once we're no longer able to treat the struct as a homogeneous aggregate).
I've tightened up the conditional a bit as it wouldn't have previously accounted for the case where you had mfloat-abi=hard with an AAPCS (non VFP) attribute. Although in this instance, the backend does seem to be correcting/ignoring that, it's still best we don't blindly rely on such behaviour.

I've also re-structured the conditional and added some comments so it's (hopefully) a little bit clearer what's going on.

I believe this fixes the bug at hand. There's some other calls to getABIKind referencing AAPCS16_VFP, but that doesn't have a direct translation to an LLVM/FI CallingConvention (that I can see). Seems there is a lot of different ways to check various ABI/CC's and none of them seem able to give you the full picture at any point.

Looking more carefully, I guess none of the other places actually distinguish between ARMABIInfo::AAPCS_VFP and ARMABIInfo::AAPCS, so I guess the current change is sufficient given the calling-convention attributes that are likely to be used in practice.

Please pull the IsEffectivelyAAPCS_VFP out into a separate helper function on ARMABIInfo.

carwil updated this revision to Diff 190798.Mar 15 2019, 3:31 AM

Done. I've added an extra parameter over what you might expect as classifyArgumentTypes doesn't seem to consider AAPCS16, whereas classifyReturnTypes does.
I've also renamed the variable to make the distinction between it and the function clearer.

efriedma accepted this revision.Mar 15 2019, 12:18 PM
efriedma added a reviewer: t.p.northover.

LGTM

clang/lib/CodeGen/TargetInfo.cpp
6023 ↗(On Diff #190798)

Variable name should be capitalized.

This revision is now accepted and ready to land.Mar 15 2019, 12:18 PM
This revision was automatically updated to reflect the committed changes.
carwil marked an inline comment as done.Mar 22 2019, 9:20 AM