Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Sema: in msvc compatibility mode, don't allow forceinline on variadics

Authored by DHowett-MSFT on Mar 19 2018, 1:06 PM.



The MSVC compiler rejects __forceinline on variadic functions with the following warning (at /W4):

C4714: function 'void msvc_variadic(int,...)' marked as __forceinline not inlined

This fixes a bug in LLVM where a variadic was getting inlined into a function of calling convention x86_thiscallcc. The LLVM lowering passes cannot consume an @llvm.va_start intrinsic call in a thiscall function without emitting an assertion.

Inlining variadics should almost certainly be possible; however, this is a fix to bring Clang in line with MSVC.

Diff Detail

rC Clang

Event Timeline

DHowett-MSFT created this revision.Mar 19 2018, 1:06 PM

Apologies if I've chosen the wrong set of reviewers.

smeenai added a subscriber: smeenai.

@DHowett-MSFT the reviewers look fine to me. Reid is the code owner for clang's MSVC compat support. David doesn't work on this stuff directly anymore, I think, but he's still pretty active in code reviews for it. I'm adding Saleem, who's also pretty active on Windows stuff.


The formatting here looks off (it doesn't look clang-formatted).

Fixed formatting.

The compiler shouldn't inline functions which call va_start, whether or not they're marked always_inline. That should work correctly, I think, at least on trunk. (See .)

If you want to warn anyway, that's okay.

What happens in the case that you have a variadic in C code marked with __forceinline? Does that also cause a warning with MSVC?


Personally, I prefer the fully canonicalized triple name i686-unknown-windows-msvc.


Would be nice to have a second test that uses the Microsoft definitions (char * and the offsetting handling for the va_list since when building against the Windows SDK, that is the way that va_list and the va_* family of functions will get handled.

The compiler shouldn't inline functions which call va_start, whether or not they're marked always_inline. That should work correctly, I think, at least on trunk. (See .)

If you want to warn anyway, that's okay.

Interesting. I certainly hit the lowering/instruction scheduling crash after that change landed. Perhaps it's a valuable thing for me to file a bug on.

DHowett-MSFT added inline comments.Mar 20 2018, 10:29 AM

I'll do this.


Should/do those still result in the intrinsic being emitted? If not, LLVM shouldn't have trouble during instruction scheduling, but inlining may still be broken. Regardless, though, this test exists only to make sure the function doesn't end up truly inlined, regardless of its contents.

compnerd added inline comments.Mar 20 2018, 3:56 PM

That would still be lowered with the intrinsics. The intent is to make sure that the different implementation does get lowered appropriately.

efriedma added inline comments.Mar 20 2018, 4:19 PM

To get correct lowering in LLVM, the va_start macro *must* be translated to __builtin_va_start(); otherwise, the generated IR is nonsense and you'll miscompile. (See also .)

DHowett-MSFT added inline comments.Mar 20 2018, 5:39 PM

This sounds like an argument for not including a test of the Microsoft definition of va_start.

DHowett-MSFT marked 5 inline comments as done.Mar 23 2018, 10:26 AM

@compnerd CL warns for __forceinline variadics in C code as well:

test.c(1): warning C4714: function 'void hello(int,...)' marked as __forceinline not inlined
DHowett-MSFT planned changes to this revision.Apr 4 2018, 2:19 PM
DHowett-MSFT marked 2 inline comments as done.

Switched from i686-windows to i686-unknown-windows-msvc as per @compnerd's suggestion.
Based on the prior discussion around MSVC's definition of va_{start,end,arg} and how it is not a valid implementation that we do not pattern-match and convert into the intrinsid, I do not see the need in implementing an MSVC va_start test.

Regardless of that, the behaviour under test is that the function is _not inlined_, not that the intrinsic is emitted properly.

compnerd accepted this revision.Apr 4 2018, 8:06 PM
This revision is now accepted and ready to land.Apr 4 2018, 8:06 PM

Thanks! I don't have the means to check this in myself.

rnk added inline comments.Apr 10 2018, 3:04 PM

Why is this warning dependent on the ABI? GCC has a similar warning:


I'd actually rather not ignore the attribute. Clang shouldn't be making decisions about what LLVM is and isn't capable of inlining. It's not too crazy to imagine eventually implementing inlining of variadic function calls.