Page MenuHomePhabricator

ARM: Report error for invalid use of AAPCS_VFP calling convention
Needs ReviewPublic

Authored by charles.baylis on Jun 30 2017, 4:54 AM.

Details

Summary

The AAPCS_VFP calling convention cannot be supported on targets
which do not have VFP, or on Thumb-1 targets, which cannot access
the VFP.

Diff Detail

Event Timeline

charles.baylis created this revision.Jun 30 2017, 4:54 AM
joerg edited edge metadata.Jun 30 2017, 7:37 AM

LGTM, but please cut down the test cases by hand. Most of the attributes and module flags should be unnecessary.

t.p.northover edited edge metadata.Jun 30 2017, 7:46 AM

What's the use case here? For people using __attribute__((pcs(...))) Clang should be producing the diagnostic (and I'm not even entirely convinced it should be an error, if your code only contains integer types there's not actually any problem).

Beyond that the tests don't need to be in separate files and contain an awful lot of Clang-generated cruft (the attributes, metadata, entry label and source_filename).

What's the use case here? For people using __attribute__((pcs(...))) Clang should be producing the diagnostic (and I'm not even entirely convinced it should be an error, if your code only contains integer types there's not actually any problem).

While the main use case is clang, the current behaviour is undesirable. When configured for the hardfp ABI, llvm and clang will (for Thumb-1, and targets without VFP) silently produce code which conforms to the softfloat ABI instead.

Fixing the problem in clang still allows that undesirable behaviour in LLVM. It may also be necessary to fix it in clang for cleaner error reporting, unless there is a more delicate way to report the error here.

Beyond that the tests don't need to be in separate files

I think that they do, because report_fatal_error() causes compilation to terminate so the subsequent tests don't generate a message which can be checked.

and contain an awful lot of Clang-generated cruft (the attributes, metadata, entry label and source_filename).

Ack.

I think that they do, because report_fatal_error() causes compilation to terminate so the subsequent tests don't generate a message which can be checked.

Ah yes. And that's also a really big indicator for why this should be handled in Clang. All you know from this is that at least one function, somewhere in your file (or codebase if doing LTO) has an aapcs_vfp attribute.

I get that silently doing SoftFP isn't great either so I'm not completely opposed to this change, I just think it's a pretty half-baked solution to the actual user-facing problem.

Updates:
. removed cruft from new test cases
. adjusted existing test cases to avoid hardfp/Thumb-1 combination

It does seem like there ought to be a better way to report the error, eg MachineInstr::emitError() has some logic to acquire a LocCookie. I'm not sure if/how I can do something similar here. Any hints?

rovka edited edge metadata.Jul 14 2017, 5:13 AM

Hi, you could try looking into LLVMContext::emitError - it shouldn't be too hard to get hold of an LLVMContext, and that's what we use to report e.g. errors in inline asm in SelectionDAGBuilder.

fhahn edited edge metadata.Jul 31 2017, 6:08 AM

I think that they do, because report_fatal_error() causes compilation to terminate so the subsequent tests don't generate a message which can be checked.

Ah yes. And that's also a really big indicator for why this should be handled in Clang. All you know from this is that at least one function, somewhere in your file (or codebase if doing LTO) has an aapcs_vfp attribute.

I get that silently doing SoftFP isn't great either so I'm not completely opposed to this change, I just think it's a pretty half-baked solution to the actual user-facing problem.

I think TargetInfo in Clang has a checkCallingConvention function, which is implemented by ARMTargetInfo. A user facing error message should probably go there. We have access to the triple there, so we should be able to error on cases where the selected architecture only has soft floats. I am not sure if we have access to target-features there though, so we probably won't be able to handle all cases there.

We probably still need a warning/error in LLVM to cover the LTO case, e.g. when linking modules compiled to use the hardfp ABI into a target module with softfp only. IMO we should report that case, but ideally using LLVMContext::emitError as Diana suggested.

D35826 and D35569 are related changes, adding an error message to Clang when -marm is selected for Thumb-only architectures and arguing for an error message when ARM codegen is requested for a Thumb-only architecture in LLVM. Whatever we decide, I think both cases should be handled consistently.

fhahn resigned from this revision.Dec 15 2017, 12:36 PM

ARM ELF has an additional field that indicates the PCS used. With the frontend validating the input, wouldn't we be sure that each object file is valid. If there is a mismatch in the PCS, that should already be an error from the linker. Wouldn't the LTO case be handled similarly?