Page MenuHomePhabricator

[SimplifyLibCalls] Keep calling convention when simplifying to libcall
AbandonedPublic

Authored by john.brawn on Oct 14 2020, 9:21 AM.

Details

Summary

Currently when simplifying to a library function for which no declaration exists a new declaration is created which defaults to the C calling convention. This can cause problems if the definition is of a different calling convention, so use the calling convention of the library call that's being replaced.

Fixes PR45524.

Diff Detail

Unit TestsFailed

TimeTest
190 mslinux > AddressSanitizer-Unit._/Asan-x86_64-calls-Dynamic-Test::AddressSanitizer.MAYBE_StrNDupOOBTest
Note: Google Test filter = AddressSanitizer.MAYBE_StrNDupOOBTest [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
40 mslinux > AddressSanitizer-Unit._/Asan-x86_64-calls-Dynamic-Test::AddressSanitizer.StrArgsOverlapTest
Note: Google Test filter = AddressSanitizer.StrArgsOverlapTest [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
270 mslinux > AddressSanitizer-Unit._/Asan-x86_64-calls-Dynamic-Test::AddressSanitizer.StrChrAndIndexOOBTest
Note: Google Test filter = AddressSanitizer.StrChrAndIndexOOBTest [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
50 mslinux > AddressSanitizer-Unit._/Asan-x86_64-calls-Dynamic-Test::AddressSanitizer.StrCpyOOBTest
Note: Google Test filter = AddressSanitizer.StrCpyOOBTest [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
160 mslinux > AddressSanitizer-Unit._/Asan-x86_64-calls-Dynamic-Test::AddressSanitizer.StrDupOOBTest
Note: Google Test filter = AddressSanitizer.StrDupOOBTest [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
View Full Test Results (28 Failed)

Event Timeline

john.brawn created this revision.Oct 14 2020, 9:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2020, 9:21 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
john.brawn requested review of this revision.Oct 14 2020, 9:21 AM

In general, if TargetLibraryInfo says a library function is available, that means it's available with the C calling convention. Copying the calling convention from some other call shouldn't be necessary. In general, there is no call to copy the calling convention from.

I'd recommend taking a closer look at why these markings exist in the first place. My first impression is that clang is doing something wrong: it shouldn't be marking up calls to C library functions with a non-C calling convention.

Clang treats C library calls the same as any other function calls when it comes to the calling convention, so they get a non-C calling convention in the same circumstances that other functions do.

Looking at the ARM target in specific, a function gets a non-C calling convention when CodeGenModule::getTargetCodeGenInfo sets a calling convention (based on target triple and CodeGenOpts.FloatABI) that's different to what ARMABIInfo::getLLVMDefaultCC (which is based solely on the target triple) returns. So if you're using -mtriple=arm-none-eabi -mfloat-abi=hard then functions get the arm_aapcs_vfpcc calling convention, and if you're using -mtriple=arm-none-eabihf -mfloat-abi=soft then functions get the arm_aapcscc calling convention.

Looking at the backend it's ARMTargetLowering::getEffectiveCallingConv that determines the effective calling convention for functions with the C calling convention, which it does based on getTargetMachine().Options.FloatABIType which by default is derived from the target triple.

So if we wanted clang to use the C calling convention always for C library calls we would need to either:

  • Add special handling to make sure C library calls only get the C calling convention and add some mechanism to signal to the backend what the actual calling convention is.
  • Remove the behaviour that the calling-convention is non-C when there's a mismatch between the triple and the float-abi and do something different to make sure we end up with the right calling convention in the end (maybe have -mfloat-abi implicitly modify the triple like -march and -mcpu do).

Remove the behaviour that the calling-convention is non-C when there's a mismatch between the triple and the float-abi and do something different to make sure we end up with the right calling convention in the end (maybe have -mfloat-abi implicitly modify the triple like -march and -mcpu do).

I would lean towards doing this.

The alternative is that we add module metadata or something like that to specify the "C" calling convention in LLVM IR. And I don't really want to go there if we can avoid it. (I can't think of any reason it would be better than messing with the triple on ARM.)

Note that we do currently have TargetOptions::FloatABIType. But that doesn't really work with LTO.

Add special handling to make sure C library calls only get the C calling convention and add some mechanism to signal to the backend what the actual calling convention is.

Special-casing C library functions seems more complicated for no benefit.

D89573 is a fix for this by adjusting the triple. I'll abandon this if that gets accepted.

john.brawn abandoned this revision.Oct 21 2020, 3:24 AM

Abandoning as I've committed D89573.