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

[DHowett-MSFT]

Summary

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.

[DHowett-MSFT]

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

[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.

lib/Sema/SemaDeclAttr.cpp

3890

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

[DHowett-MSFT]

Fixed formatting.

[efriedma]

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 https://reviews.llvm.org/D42556 .)

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

[compnerd]

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

test/Sema/ms-forceinline-on-variadic.cpp

2

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

15

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.

[DHowett-MSFT]

In D44646#1042347, @efriedma wrote:

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 https://reviews.llvm.org/D42556 .)

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]

test/Sema/ms-forceinline-on-variadic.cpp

2

I'll do this.

15

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]

test/Sema/ms-forceinline-on-variadic.cpp

15

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

[efriedma]

test/Sema/ms-forceinline-on-variadic.cpp

15

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 https://bugs.llvm.org/show_bug.cgi?id=24320 .)

[DHowett-MSFT]

test/Sema/ms-forceinline-on-variadic.cpp

15

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

[DHowett-MSFT]

@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]

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.

[DHowett-MSFT]

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

[rnk]

lib/Sema/SemaDeclAttr.cpp

3889

Why is this warning dependent on the ABI? GCC has a similar warning:
https://godbolt.org/g/3E4NZ1

3892

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.

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_attribute_ignored : Warning<"%0 attribute ignored">,
   InGroup<IgnoredAttributes>;
 def warn_attribute_ignored_on_inline :
   Warning<"%0 attribute ignored on inline function">,
   InGroup<IgnoredAttributes>;
 def warn_attribute_after_definition_ignored : Warning<
   "attribute %0 after definition is ignored">,
    InGroup<IgnoredAttributes>;
+def warn_always_inline_on_variadic : Warning<
+  "inlining attribute %0 ignored on variadic function">,
+   InGroup<IgnoredAttributes>;
 def warn_unknown_attribute_ignored : Warning<
   "unknown attribute %0 ignored">, InGroup<UnknownAttributes>;
 def warn_cxx11_gnu_attribute_on_type : Warning<
   "attribute %0 ignored, because it cannot be applied to a type">,
   InGroup<IgnoredAttributes>;
 def warn_unhandled_ms_attribute_ignored : Warning<
   "__declspec attribute %0 is not supported">, 
   InGroup<IgnoredAttributes>;

lib/Sema/SemaDeclAttr.cpp

AlwaysInlineAttr *Sema::mergeAlwaysInlineAttr(Decl *D, SourceRange Range,
                                               IdentifierInfo *Ident,
                                               unsigned AttrSpellingListIndex) {
   if (OptimizeNoneAttr *Optnone = D->getAttr<OptimizeNoneAttr>()) {
     Diag(Range.getBegin(), diag::warn_attribute_ignored) << Ident;
     Diag(Optnone->getLocation(), diag::note_conflicting_attribute);
     return nullptr;
   }
 
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+      hasFunctionProto(D) && isFunctionOrMethodVariadic(D)) {
+    Diag(Range.getBegin(), diag::warn_always_inline_on_variadic) << Ident;
+    return nullptr;
+  }
+
   if (D->hasAttr<AlwaysInlineAttr>())
     return nullptr;
 
   return ::new (Context) AlwaysInlineAttr(Range, Context,
                                           AttrSpellingListIndex);
 }
 
 CommonAttr *Sema::mergeCommonAttr(Decl *D, SourceRange Range,

test/Sema/ms-forceinline-on-variadic.cpp

+// RUN: %clang_cc1 -emit-llvm -o - -triple i686-unknown-windows-msvc -verify -fms-extensions %s \
+// RUN:            | FileCheck %s
+//
+// (instruction scheduling crash test)
+// RUN: %clang_cc1 -S -o - -triple i686-unknown-windows-msvc -fms-extensions %s
+
+inline __attribute__((always_inline)) void attribute_variadic(int f, ...) { } // expected-warning {{inlining attribute 'always_inline' ignored on variadic function}}
+
+__forceinline inline void msvc_variadic(int f, ...) { // expected-warning {{inlining attribute '__forceinline' ignored on variadic function}}
+// CHECK-DAG: define {{.*}} void [[MSVC_VARIADIC:@".*msvc_variadic.*"]](
+    __builtin_va_list ap;
+    __builtin_va_start(ap, f);
+    __builtin_va_end(ap);
+}
+
+struct a {
+    // members are, by default, thiscall; enforce it for the purposes of the test
+    void __thiscall dispatcher();
+};
+
+void __thiscall a::dispatcher() {
+    msvc_variadic(1, 2, 3);
+// CHECK-DAG: define dso_local x86_thiscallcc void @"{{.*dispatcher.*}}"(
+// CHECK-DAG: call void {{.*}} [[MSVC_VARIADIC]]
+}
+
+void t() {
+    a{}.dispatcher();
+}