[clang-cl] Handle some pragma alloc_text corner cases handled by MSVC

[steplong]

Summary

MSVC's pragma alloc_text accepts a function that was redeclared in
a non extern-C context if the previous declaration was in an extern-C
context. i.e.

extern "C" { static void f(); }
static void f();

MSVC's pragma alloc_text also rejects non-functions.

[steplong]

This does make us accept, but MSVC doesn't:

static void f();
extern "C" {
  static void f();
}
#pragma alloc_text(c, f);
static void f() {}

MSVC and GCC fail with an error for conflicting declaration.
GCC: https://godbolt.org/z/Tavvx88E4
MSVC: https://godbolt.org/z/vj41TWdYh

[hans]

clang/lib/Sema/SemaAttr.cpp

811

Since the pragma only applies to functions, maybe we should error here if the decl is not a FunctionDecl?

824

Instead of walking the decls, would checking isExternC() on the getCanonicalDecl() be enough?

[steplong]

• Use getCanonicalDecl(). This does make us reject (MSVC rejects it already because of conflicting declarations):

static void f();
extern "C" { static void f(); }
static void f() {}

• Error if not function decl

I think we have to use isInExternCContext() to accept the following (MSVC accepts this):

extern "C" { static void f(); }
static void f() {}

[hans]

In D128649#3612626, @steplong wrote:

I think we have to use isInExternCContext() to accept the following (MSVC accepts this):

extern "C" { static void f(); }
static void f() {}

I'm probably missing something. Is the "static" important in this example? Since f has internal linkage, I don't see why a user would care if it's "extern C" or not, and also not in which section it would go (in this case it will not be emitted at all, because it's not used).

Isn't the question whether f is considered "extern C" in the end or not? I thought isExternC() checks that? Are you saying it would return false for f in your example?

clang/lib/Sema/SemaAttr.cpp

830

Since it's inside if (getLangOpts().CPlusPlus) { it will only error in C++ mode, but it should probably also error in C mode.

[steplong]

Isn't the question whether f is considered "extern C" in the end or not? I thought isExternC() checks that? Are you saying it would return false for f in your example?

Yup, isExternC() is returning false for that case because there's "static".

I'm probably missing something. Is the "static" important in this example? Since f has internal linkage, I don't see why a user would care if it's "extern C" or not, and also not in which section it would go (in this case it will not be emitted at all, because it's not used).

Hmm, good point. This is the disassembly I see for:

extern "C" { static void f(); }
static void f();
#pragma alloc_text(s, f)
static void f() {}

void bar() {
  f();
}

Disassembly of section .text$mn:
?bar@@YAXXZ:
       0:       fd 7b bf a9     stp     x29, x30, [sp, #-16]!
       4:       fd 03 00 91     mov     x29, sp
       8:       00 00 00 94     bl      #0 <?bar@@YAXXZ+0x8>
       c:       fd 7b c1 a8     ldp     x29, x30, [sp], #16
      10:       c0 03 5f d6     ret
Disassembly of section s:
s:
       0:       c0 03 5f d6     ret

[steplong]

• Error for c also

[hans]

In D128649#3612875, @steplong wrote:

Isn't the question whether f is considered "extern C" in the end or not? I thought isExternC() checks that? Are you saying it would return false for f in your example?

Yup, isExternC() is returning false for that case because there's "static".

I see. It seems there's already some code that gives special treatment to "static extern c" functions in CodeGenModule::MaybeHandleStaticInExternC(): https://github.com/llvm/llvm-project/blob/llvmorg-14.0.6/clang/lib/CodeGen/CodeGenModule.cpp#L4420

Okay, I think this is almost ready. I just had one more comment about the if-statements above.

clang/lib/Sema/SemaAttr.cpp

832

Instead of checking this in two places, how about doing something like:

FunctionDecl *FD = dyn_cast<FunctionDecl>(ND->getCanonicalDecl());
if (!FD) {
  // error
}

above right after the if (!ND) check? Then the C++ specific code just becomes

if (!FD->isInExternCContext()) {
  // error
}

[steplong]

• Rework logic for rejecting non-functions and non-extern C functions when C++

[hans]

lgtm

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_invalid_super_scope : Error<"invalid use of '__super', "
   "this keyword can only be used inside class or member function scope">;
 def err_super_in_lambda_unsupported : Error<
   "use of '__super' inside a lambda is unsupported">;
 
 def err_pragma_expected_file_scope : Error<
   "'#pragma %0' can only appear at file scope">;
 def err_pragma_alloc_text_c_linkage: Error<
   "'#pragma alloc_text' is applicable only to functions with C linkage">;
+def err_pragma_alloc_text_not_function: Error<
+  "'#pragma alloc_text' is applicable only to functions">;
 
 def warn_pragma_unused_undeclared_var : Warning<
   "undeclared variable %0 used as an argument for '#pragma unused'">,
   InGroup<IgnoredPragmas>;
 def warn_atl_uuid_deprecated : Warning<
   "specifying 'uuid' as an ATL attribute is deprecated; use __declspec instead">,
   InGroup<DeprecatedDeclarations>;
 def warn_pragma_unused_expected_var_arg : Warning<

clang/lib/Sema/SemaAttr.cpp

for (auto &Function : Functions) {
     SourceLocation Loc;
     std::tie(II, Loc) = Function;
 
     DeclarationName DN(II);
     NamedDecl *ND = LookupSingleName(TUScope, DN, Loc, LookupOrdinaryName);
     if (!ND) {
       Diag(Loc, diag::err_undeclared_use) << II->getName();
       return;
     }
 
-    DeclContext *DC = ND->getDeclContext();
-    if (getLangOpts().CPlusPlus && !DC->isExternCContext()) {
+    auto *FD = dyn_cast<FunctionDecl>(ND->getCanonicalDecl());
+    if (!FD) {
+      Diag(Loc, diag::err_pragma_alloc_text_not_function);
+      return;
+    }
+
+    if (getLangOpts().CPlusPlus && !FD->isInExternCContext()) {
       Diag(Loc, diag::err_pragma_alloc_text_c_linkage);
       return;
     }
 
     FunctionToSectionMap[II->getName()] = std::make_tuple(Section, Loc);
   }
 }
 
 void Sema::ActOnPragmaUnused(const Token &IdTok, Scope *curScope,
                              SourceLocation PragmaLoc) {
 
   IdentifierInfo *Name = IdTok.getIdentifierInfo();
   LookupResult Lookup(*this, Name, IdTok.getLocation(), LookupOrdinaryName);
   LookupParsedName(Lookup, curScope, nullptr, true);
 
   if (Lookup.empty()) {
     Diag(PragmaLoc, diag::warn_pragma_unused_undeclared_var)
       << Name << SourceRange(IdTok.getLocation());
     return;
   }
 

clang/test/Sema/pragma-ms-alloc-text.c

 // RUN: %clang_cc1 -fms-extensions -fsyntax-only -verify %s
 
 void foo();
-#pragma alloc_text("hello", foo) // expected-no-diagnostics
+#pragma alloc_text("hello", foo) // no-error
 void foo() {}
 
 static void foo1();
-#pragma alloc_text("hello", foo1) // expected-no-diagnostics
+#pragma alloc_text("hello", foo1) // no-error
 void foo1() {}
+
+int foo2;
+#pragma alloc_text(c, foo2) // expected-error {{'#pragma alloc_text' is applicable only to functions}}

clang/test/Sema/pragma-ms-alloc-text.cpp

 #pragma alloc_text(c, foo5) // expected-error {{'#pragma alloc_text' is applicable only to functions with C linkage}}
 extern "C" void foo5() {}   // expected-error {{declaration of 'foo5' has a different language linkage}}
 
 extern "C" {
 static void foo6();
 #pragma alloc_text(c, foo6) // no-warning
 void foo6() {}
 }
+
+extern "C" {
+static void foo7();
+}
+static void foo7();
+#pragma alloc_text(c, foo7) // no-warning
+void foo7() {}
+
+static void foo8();
+extern "C" {
+static void foo8();
+}
+#pragma alloc_text(c, foo8) // expected-error {{'#pragma alloc_text' is applicable only to functions with C linkage}}
+void foo8() {}
+
+enum foo9 { A, B, C };
+#pragma alloc_text(c, foo9) // expected-error {{'#pragma alloc_text' is applicable only to functions}}