Attribute inline

[zahiraam]

Summary

Added support for attribute inline for a __declspec declaration.

[aaron.ballman]

include/clang/Basic/Attr.td

869

I cannot see any documentation on MSDN for this spelling of the attribute, and MSVC 2015 rejects it. What is the motivating use case for this spelling?

[zahiraam]

ksh-3.2$ cat test3.c
static void __declspec(inline) foo() {
}

int main()
{

foo();

}

ksh-3.2$ cl test3.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

test3.c
Microsoft (R) Incremental Linker Version 14.00.24215.1
Copyright (C) Microsoft Corporation. All rights reserved.

/out:test3.exe
test3.obj
ksh-3.2$

This is for an application that we are running here that is failing.

[aaron.ballman]

In D32092#727507, @zahiraam wrote:

ksh-3.2$ cat test3.c
static void __declspec(inline) foo() {
}

int main()
{

foo();

}

ksh-3.2$ cl test3.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

test3.c
Microsoft (R) Incremental Linker Version 14.00.24215.1
Copyright (C) Microsoft Corporation. All rights reserved.

/out:test3.exe
test3.obj
ksh-3.2$

This is for an application that we are running here that is failing.

I just tested this on 19.10.25017, 19.10.25206.0 and 14.0.25431.01 and none of them accept your code.

https://godbolt.org/g/xk1ciG

[zahiraam]

Forgot to mention that this is for C only. CL doesn't compile this declaration in a C++ context.
Please see:
https://godbolt.org/g/WRhyjL

[aaron.ballman]

In D32092#727518, @zahiraam wrote:

Forgot to mention that this is for C only. CL doesn't compile this declaration in a C++ context.
Please see:
https://godbolt.org/g/WRhyjL

Oh, that's fun.

It's not at all clear that this isn't simply an MSVC bug. Can you work around this in your project rather than introduce an undocumented MSVC attribute?

[rsmith]

include/clang/Basic/Attr.td

869

Also, it seems rather unlikely that MSVC would implement the GNU inline semantics. Have you investigated the actual semantic effects of this __declspec attribute and checked they match the GNU semantics?

[zahiraam]

Yes it behaves the same way than the Gnu inline.

[zahiraam]

Pushed the submit too fast ...
Before I submitted this review, I have done some experiments and inline and declspec(inline) have the same behavior. Compiling with /Ob0 disables inlining. With -O1 or -O2, inline happens.

[rsmith]

In D32092#727543, @zahiraam wrote:

Pushed the submit too fast ...
Before I submitted this review, I have done some experiments and inline and declspec(inline) have the same behavior. Compiling with /Ob0 disables inlining. With -O1 or -O2, inline happens.

What do you mean, "inline and declspec(inline) have the same behavior"? What exactly did you test? The inline keyword means quite different things in different language modes, and none of them are the same as the meaning of __attribute__((gnu_inline)) (which is what you aliased this to). Some of these differences are quite subtle (for instance, the precise behavior of extern inline and static inline, and what happens if you redeclare a function with a different set of specifiers).

[zahiraam]

Yes I did compare "inline" with "declspec(inline)" and not with "__attribute((gnu_inline))" and didn't test it whit different specifiers. I can do that.
If behavior is different we probably have to add an additional attribute?

[aaron.ballman]

In D32092#727559, @zahiraam wrote:

Yes I did compare "inline" with "declspec(inline)" and not with "__attribute((gnu_inline))" and didn't test it whit different specifiers. I can do that.
If behavior is different we probably have to add an additional attribute?

It's not that it needs an additional attribute, but that your patch will require additional work. However, my question remains unanswered: can you fix your project? Is this in a header file of some well-used library? We typically do not implement undocumented compiler features of other compilers unless there's a compelling reason to do so.

[rsmith]

From some very superficial testing, it looks like CL treats __declspec(inline) exactly like a synonym for inline (it even rejects them both appearing on the same declaration, complaining about a duplicate inline specifier). So modeling this as GNUInlineAttr is definitely wrong, and this should probably be setting the inline flag on the function. But I agree with Aaron: we need some evidence that implementing this in Clang is worthwhile (rather than changing the codebase to use the inline keyword instead). We aim to be CL-compatible for common code patterns, not to be 100% compatible with all code that CL accepts.

include/clang/Basic/Attr.td

def FormatArg : InheritableAttr {
   let Spellings = [GCC<"format_arg">];
   let Args = [IntArgument<"FormatIdx">];
   let Subjects = SubjectList<[ObjCMethod, HasFunctionProto], WarnDiag,
                              "ExpectedFunctionWithProtoType">;
   let Documentation = [Undocumented];
 }
 
 def GNUInline : InheritableAttr {
-  let Spellings = [GCC<"gnu_inline">];
+  let Spellings = [GCC<"gnu_inline">, Declspec<"inline">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [Undocumented];
 }
 
 def Hot : InheritableAttr {
   let Spellings = [GCC<"hot">];
   let Subjects = SubjectList<[Function]>;
   // An AST node is created for this attribute, but not actually used beyond

lib/Parse/ParseDecl.cpp

while (Tok.isNot(tok::r_paren)) {
       // Attribute not present.
       if (TryConsumeToken(tok::comma))
         continue;
 
       // We expect either a well-known identifier or a generic string.  Anything
       // else is a malformed declspec.
       bool IsString = Tok.getKind() == tok::string_literal;
       if (!IsString && Tok.getKind() != tok::identifier &&
-          Tok.getKind() != tok::kw_restrict) {
+          Tok.getKind() != tok::kw_restrict &&
+        (Tok.getKind() != tok::kw_inline ||
+       (Tok.getKind() == tok::kw_inline  &&
+      getLangOpts().CPlusPlus) ) ){
         Diag(Tok, diag::err_ms_declspec_type);
         T.skipToEnd();
         return;
       }
 
       IdentifierInfo *AttrName;
       SourceLocation AttrNameLoc;
       if (IsString) {

lib/Sema/SemaDeclAttr.cpp

static void handleGlobalAttr(Sema &S, Decl *D, const AttributeList &Attr) {
 
   D->addAttr(::new (S.Context)
               CUDAGlobalAttr(Attr.getRange(), S.Context,
                              Attr.getAttributeSpellingListIndex()));
 }
 
 static void handleGNUInlineAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   FunctionDecl *Fn = cast<FunctionDecl>(D);
-  if (!Fn->isInlineSpecified()) {
+   if (!Fn->isInlineSpecified() && !Attr.isDeclspecAttribute()) {
     S.Diag(Attr.getLoc(), diag::warn_gnu_inline_attribute_requires_inline);
     return;
   }
 
   D->addAttr(::new (S.Context)
              GNUInlineAttr(Attr.getRange(), S.Context,
                            Attr.getAttributeSpellingListIndex()));
 }

test/Sema/declspec-inline.c

+// RUN: %clang_cc1 -verify -fms-compatibility %s
+// expected-no-diagnostics
+
+static int __declspec(inline) foo(int i) {
+  int k;
+  int a[10000];
+  for (k=0; k<=i; k++)
+    {
+      a[k] = a[k]*k;
+    }
+  return a[k];
+}
+
+int main()
+{
+  int i=20;
+  return foo(2000);
+}