[Clang] strengthen checks for 'main' function to meet [basic.start.main] p3 requirements

[a-tarasyuk]

Fixes #101512

[llvmbot]

@llvm/pr-subscribers-clang

Author: Oleksandr T. (a-tarasyuk)

Changes

Fixes #101512

---

Full diff: https://github.com/llvm/llvm-project/pull/101853.diff

8 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/include/clang/AST/DeclBase.h (+2)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2)
• (modified) clang/include/clang/Sema/Sema.h (+2-1)
• (modified) clang/lib/AST/Decl.cpp (+3-5)
• (modified) clang/lib/Sema/SemaDecl.cpp (+31-11)
• (added) clang/test/SemaCXX/linkage1.cpp (+22)
• (added) clang/test/SemaCXX/linkage3.cpp (+5)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4c7bd099420ab..3303db5a87ace 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -145,6 +145,8 @@ Improvements to Clang's diagnostics
 
 - -Wdangling-assignment-gsl is enabled by default.
 
+- Clang now diagnoses the use of `main` in `extern` context as invalid according to [basic.start.main] p2. Fixes #GH101512.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index 40f01abf384e9..e28626eabe8e8 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -2156,6 +2156,8 @@ class DeclContext {
     return getDeclKind() == Decl::TranslationUnit;
   }
 
+  bool isLinkageSpec() const { return getDeclKind() == Decl::LinkageSpec; }
+
   bool isRecord() const {
     return getDeclKind() >= Decl::firstRecord &&
            getDeclKind() <= Decl::lastRecord;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 581434d33c5c9..e86b391264d2a 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -990,6 +990,8 @@ def warn_main_redefined : Warning<"variable named 'main' with external linkage "
     "has undefined behavior">, InGroup<Main>;
 def ext_main_used : Extension<
     "referring to 'main' within an expression is a Clang extension">, InGroup<Main>;
+def err_main_invalid_linkage_specification : ExtWarn<
+  "'main' cannot have linkage specification 'extern \"C\"'">, InGroup<Main>;
 
 /// parser diagnostics
 def ext_no_declarators : ExtWarn<"declaration does not declare anything">,
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 2ec6367eccea0..d791991fcfd8b 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3585,8 +3585,9 @@ class Sema final : public SemaBase {
   /// \param OldT The portion of the type of the old declaration to check.
   bool canFullyTypeCheckRedeclaration(ValueDecl *NewD, ValueDecl *OldD,
                                       QualType NewT, QualType OldT);
-  void CheckMain(FunctionDecl *FD, const DeclSpec &D);
+  void CheckMain(FunctionDecl *FD, DeclContext *DC, const DeclSpec &D);
   void CheckMSVCRTEntryPoint(FunctionDecl *FD);
+  bool CheckLinkageSpecification(DeclContext *DC, Decl *D);
 
   /// Returns an implicit CodeSegAttr if a __declspec(code_seg) is found on a
   /// containing class. Otherwise it will return implicit SectionAttr if the
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 490c4a2fc525c..aa2ad1752cc5c 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -3292,11 +3292,9 @@ bool FunctionDecl::isImmediateFunction() const {
 }
 
 bool FunctionDecl::isMain() const {
-  const TranslationUnitDecl *tunit =
-    dyn_cast<TranslationUnitDecl>(getDeclContext()->getRedeclContext());
-  return tunit &&
-         !tunit->getASTContext().getLangOpts().Freestanding &&
-         isNamed(this, "main");
+  const DeclContext *DC = getDeclContext();
+  return isNamed(this, "main") && !getLangOpts().Freestanding &&
+         (DC->getRedeclContext()->isTranslationUnit() || DC->isLinkageSpec());
 }
 
 bool FunctionDecl::isMSVCRTEntryPoint() const {
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 4fea38d1b02a9..832082d547bf4 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -7353,6 +7353,15 @@ void emitReadOnlyPlacementAttrWarning(Sema &S, const VarDecl *VD) {
   }
 }
 
+static bool isMainVar(DeclarationName Name, VarDecl *VD) {
+  if (Name.getAsIdentifierInfo() && Name.getAsIdentifierInfo()->isStr("main") &&
+      !VD->getDescribedVarTemplate()) {
+    const DeclContext *DC = VD->getDeclContext();
+    return DC->getRedeclContext()->isTranslationUnit() || DC->isLinkageSpec();
+  }
+  return false;
+}
+
 NamedDecl *Sema::ActOnVariableDeclarator(
     Scope *S, Declarator &D, DeclContext *DC, TypeSourceInfo *TInfo,
     LookupResult &Previous, MultiTemplateParamsArg TemplateParamLists,
@@ -8052,15 +8061,13 @@ NamedDecl *Sema::ActOnVariableDeclarator(
   }
 
   // Special handling of variable named 'main'.
-  if (Name.getAsIdentifierInfo() && Name.getAsIdentifierInfo()->isStr("main") &&
-      NewVD->getDeclContext()->getRedeclContext()->isTranslationUnit() &&
-      !getLangOpts().Freestanding && !NewVD->getDescribedVarTemplate()) {
-
+  if (isMainVar(Name, NewVD) && !getLangOpts().Freestanding) {
     // C++ [basic.start.main]p3
     // A program that declares a variable main at global scope is ill-formed.
-    if (getLangOpts().CPlusPlus)
-      Diag(D.getBeginLoc(), diag::err_main_global_variable);
-
+    if (getLangOpts().CPlusPlus) {
+      if (!CheckLinkageSpecification(DC, NewVD))
+        Diag(D.getBeginLoc(), diag::err_main_global_variable);
+    }
     // In C, and external-linkage variable named main results in undefined
     // behavior.
     else if (NewVD->hasExternalFormalLinkage())
@@ -10308,7 +10315,7 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
   if (!getLangOpts().CPlusPlus) {
     // Perform semantic checking on the function declaration.
     if (!NewFD->isInvalidDecl() && NewFD->isMain())
-      CheckMain(NewFD, D.getDeclSpec());
+      CheckMain(NewFD, DC, D.getDeclSpec());
 
     if (!NewFD->isInvalidDecl() && NewFD->isMSVCRTEntryPoint())
       CheckMSVCRTEntryPoint(NewFD);
@@ -10473,7 +10480,7 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
 
     // Perform semantic checking on the function declaration.
     if (!NewFD->isInvalidDecl() && NewFD->isMain())
-      CheckMain(NewFD, D.getDeclSpec());
+      CheckMain(NewFD, DC, D.getDeclSpec());
 
     if (!NewFD->isInvalidDecl() && NewFD->isMSVCRTEntryPoint())
       CheckMSVCRTEntryPoint(NewFD);
@@ -12210,7 +12217,10 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD,
   return Redeclaration;
 }
 
-void Sema::CheckMain(FunctionDecl* FD, const DeclSpec& DS) {
+void Sema::CheckMain(FunctionDecl *FD, DeclContext *DC, const DeclSpec &DS) {
+  if (CheckLinkageSpecification(DC, FD))
+    return;
+
   // C++11 [basic.start.main]p3:
   //   A program that [...] declares main to be inline, static or
   //   constexpr is ill-formed.
@@ -12238,7 +12248,6 @@ void Sema::CheckMain(FunctionDecl* FD, const DeclSpec& DS) {
         << FixItHint::CreateRemoval(DS.getConstexprSpecLoc());
     FD->setConstexprKind(ConstexprSpecKind::Unspecified);
   }
-
   if (getLangOpts().OpenCL) {
     Diag(FD->getLocation(), diag::err_opencl_no_main)
         << FD->hasAttr<OpenCLKernelAttr>();
@@ -12370,6 +12379,17 @@ void Sema::CheckMain(FunctionDecl* FD, const DeclSpec& DS) {
   }
 }
 
+bool Sema::CheckLinkageSpecification(DeclContext *DC, Decl *D) {
+  // [basic.start.main] p2
+  //   The main function shall not be declared with a linkage-specification.
+  if (DC->isExternCContext()) {
+    Diag(D->getLocation(), diag::err_main_invalid_linkage_specification);
+    D->setInvalidDecl();
+    return true;
+  }
+  return false;
+}
+
 static bool isDefaultStdCall(FunctionDecl *FD, Sema &S) {
 
   // Default calling convention for main and wmain is __cdecl
diff --git a/clang/test/SemaCXX/linkage1.cpp b/clang/test/SemaCXX/linkage1.cpp
new file mode 100644
index 0000000000000..3446e85af44be
--- /dev/null
+++ b/clang/test/SemaCXX/linkage1.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -pedantic-errors %s
+
+namespace c {
+  extern "C" void main(); // expected-error {{'main' cannot have linkage specification 'extern "C"'}}
+}
+extern "C" {
+  int main(); // expected-error {{'main' cannot have linkage specification 'extern "C"'}}
+}
+
+extern "C" int main(); // expected-error {{'main' cannot have linkage specification 'extern "C"'}}
+extern "C" struct A { int main(); }; // ok
+
+namespace ns {
+  extern "C" int main;  // expected-error {{'main' cannot have linkage specification 'extern "C"'}}
+  extern "C" struct A {
+    int main; // ok
+  };
+
+  extern "C" struct B {
+    int main(); // ok
+  };
+}
diff --git a/clang/test/SemaCXX/linkage3.cpp b/clang/test/SemaCXX/linkage3.cpp
new file mode 100644
index 0000000000000..f80d76121d4af
--- /dev/null
+++ b/clang/test/SemaCXX/linkage3.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -pedantic-errors %s
+
+extern "C" {
+  void* main; // expected-error {{'main' cannot have linkage specification 'extern "C"'}}
+}

[MitalAshok]

You can put some of the tests in clang/test/CXX/basic/basic.start/basic.start.main/p3.cpp

You lost the check for extern "C++" int main() {} at global scope. To recap:

// These are disallowed by [basic.start.main]p3
//   The main function shall not be declared with a linkage-specification.
// We only want a pedantic warning for these
extern "C++" {
int main();
}
extern "C" {
int main();
}
// These are disallowed by [basic.start.main]p(3.4)
// We want a hard error
namespace X { extern "C" int main; }
extern "C" {
namespace Y {
  int main;
}
namespace Z {
  void main();
}
}
// These are allowed
namespace W {
  extern "C++" int main();
  extern "C" {
    extern "C++" {
      int main(void*);
    }
  }
}

(You should add a test for extern "C" { namespace namespace_name { int main; } } too)

Did you try doing it like #101512 (comment) , checking Decl->getLanguageLinkage() rather than checking the DeclContext it's declared in?

[Sirraide]

This needs some cleanup I think, but it’s going in the right direction.

[Sirraide]

A few minor things, but other than that this is pretty much done I’d say.

[Sirraide]

Two more minor changes but LGTM otherwise.

[MitalAshok]

Diagnostic changes are a minor nit, LGTM otherwise

[Sirraide]

LGTM.

I’ll wait a bit to see if anyone else wants to comment on this since we’ve gone back and forth over a few things w/ this pr, but if not, I’ll merge this sometime later.

[a-tarasyuk]

@Sirraide thanks

[alexfh]

Hi @a-tarasyuk, it looks like this patch introduces a couple of problems. I guess, this boils down to marking the declaration of main() invalid in case it's declared with extern "C", but I may be wrong here.

The first problem is a clang crash: https://gcc.godbolt.org/z/dWETErhzM

The second one is a change of behavior of code that declares main() extern "C" (see my comment 37ec6e5#r145380228). This one still needs some work to create a standalone repro for, but I suspect that both issues are related to marking the declaration invalid.

The problem with both cases is that in some codebases the non-trivial amount of cleanup is necessary for this, but Clang doesn't allow to postpone the cleanup and retain the old behavior by just ignoring the diagnostic (-Wno-main). Even if the diagnostic is not issued, the issues above still manifest.

Please fix this ASAP or revert the patch, if it requires non-trivial / controversial changes or further discussion.

Thanks!