For MS ABI, emit dllexport friend functions defined inline in class

[sberg]

Summary

...as that is apparently what MSVC does

[rnk]

include/clang/AST/ASTConsumer.h

57–62

I'm pretty sure we can relax HandleInlineMethodDefinition to take a FunctionDecl and then we don't need the extra AST consumer callback.

lib/Parse/ParseCXXInlineMethods.cpp

567–568

I'd check for the friend specification here rather than asserting later. There probably are or will eventually be ways to sneak a non-friend, non-method FunctionDecl into a class context.

[sberg]

include/clang/AST/ASTConsumer.h

57–62

...and then also rename it from HandleInlineMethodDefinition to, say,HandleInlineFunctionDefinition? Or better stick with the (then no longer accurate) name?

[rnk]

include/clang/AST/ASTConsumer.h

57–62

Renaming would be good if you're up for it. I have a feeling that nobody outside of Clang is overriding this ASTConsumer callback. It's purpose is very specific to dllexport.

[sberg]

updated as discussed in the comments

[sberg]

friendly ping

[rnk]

lgtm

[sberg]

Can you please push this, I do not have commit access. Thanks

include/clang/AST/ASTConsumer.h

public:
   /// ASTContext.
   virtual void Initialize(ASTContext &Context) {}
 
   /// HandleTopLevelDecl - Handle the specified top-level declaration.  This is
   /// called by the parser to process every top-level Decl*.
   ///
   /// \returns true to continue parsing, or false to abort parsing.
   virtual bool HandleTopLevelDecl(DeclGroupRef D);
 
-  /// \brief This callback is invoked each time an inline method definition is
-  /// completed.
-  virtual void HandleInlineMethodDefinition(CXXMethodDecl *D) {}
+  /// \brief This callback is invoked each time an inline (method or friend)
+  /// function definition in a class is completed.
+  virtual void HandleInlineFunctionDefinition(FunctionDecl *D) {}
 
   /// HandleInterestingDecl - Handle the specified interesting declaration. This
   /// is called by the AST reader when deserializing things that might interest
   /// the consumer. The default implementation forwards to HandleTopLevelDecl.
   virtual void HandleInterestingDecl(DeclGroupRef D);
 
   /// HandleTranslationUnit - This method is called when the ASTs for entire
   /// translation unit have been parsed.
   virtual void HandleTranslationUnit(ASTContext &Ctx) {}
 

include/clang/Frontend/MultiplexConsumer.h

public:
   // Takes ownership of the pointers in C.
   MultiplexConsumer(std::vector<std::unique_ptr<ASTConsumer>> C);
   ~MultiplexConsumer() override;
 
   // ASTConsumer
   void Initialize(ASTContext &Context) override;
   void HandleCXXStaticMemberVarInstantiation(VarDecl *VD) override;
   bool HandleTopLevelDecl(DeclGroupRef D) override;
-  void HandleInlineMethodDefinition(CXXMethodDecl *D) override;
+  void HandleInlineFunctionDefinition(FunctionDecl *D) override;
   void HandleInterestingDecl(DeclGroupRef D) override;
   void HandleTranslationUnit(ASTContext &Ctx) override;
   void HandleTagDeclDefinition(TagDecl *D) override;
   void HandleTagDeclRequiredDefinition(const TagDecl *D) override;
   void HandleCXXImplicitFunctionInstantiation(FunctionDecl *D) override;
   void HandleTopLevelDeclInObjCContainer(DeclGroupRef D) override;
   void HandleImplicitImportDecl(ImportDecl *D) override;
   void HandleLinkerOption(llvm::StringRef Opts) override;

include/clang/Sema/Sema.h

public:
   /// order to parse the rest of the program (for instance, if it is
   /// \c constexpr in C++11 or has an 'auto' return type in C++14).
   bool canSkipFunctionBody(Decl *D);
 
   void computeNRVO(Stmt *Body, sema::FunctionScopeInfo *Scope);
   Decl *ActOnFinishFunctionBody(Decl *Decl, Stmt *Body);
   Decl *ActOnFinishFunctionBody(Decl *Decl, Stmt *Body, bool IsInstantiation);
   Decl *ActOnSkippedFunctionBody(Decl *Decl);
-  void ActOnFinishInlineMethodDef(CXXMethodDecl *D);
+  void ActOnFinishInlineFunctionDef(FunctionDecl *D);
 
   /// ActOnFinishDelayedAttribute - Invoked when we have finished parsing an
   /// attribute for which parsing is delayed.
   void ActOnFinishDelayedAttribute(Scope *S, Decl *D, ParsedAttributes &Attrs);
 
   /// \brief Diagnose any unused parameters in the given sequence of
   /// ParmVarDecl pointers.
   void DiagnoseUnusedParameters(ParmVarDecl * const *Begin,

lib/CodeGen/CodeGenAction.cpp

bool HandleTopLevelDecl(DeclGroupRef D) override {
       Gen->HandleTopLevelDecl(D);
 
       if (llvm::TimePassesIsEnabled)
         LLVMIRGeneration.stopTimer();
 
       return true;
     }
 
-    void HandleInlineMethodDefinition(CXXMethodDecl *D) override {
+    void HandleInlineFunctionDefinition(FunctionDecl *D) override {
       PrettyStackTraceDecl CrashInfo(D, SourceLocation(),
                                      Context->getSourceManager(),
-                                     "LLVM IR generation of inline method");
+                                     "LLVM IR generation of inline function");
       if (llvm::TimePassesIsEnabled)
         LLVMIRGeneration.startTimer();
 
-      Gen->HandleInlineMethodDefinition(D);
+      Gen->HandleInlineFunctionDefinition(D);
 
       if (llvm::TimePassesIsEnabled)
         LLVMIRGeneration.stopTimer();
     }
 
     void HandleTranslationUnit(ASTContext &C) override {
       {
         PrettyStackTraceString CrashInfo("Per-file LLVM IR generation");

lib/CodeGen/ModuleBuilder.cpp

void EmitDeferredDecls() {
       // methods may be added during this loop, since ASTConsumer callbacks
       // can be invoked if AST inspection results in declarations being added.
       HandlingTopLevelDeclRAII HandlingDecl(*this);
       for (unsigned I = 0; I != DeferredInlineMethodDefinitions.size(); ++I)
         Builder->EmitTopLevelDecl(DeferredInlineMethodDefinitions[I]);
       DeferredInlineMethodDefinitions.clear();
     }
 
-    void HandleInlineMethodDefinition(CXXMethodDecl *D) override {
+    void HandleInlineFunctionDefinition(FunctionDecl *D) override {
       if (Diags.hasErrorOccurred())
         return;
 
       assert(D->doesThisDeclarationHaveABody());
 
+      // Handle friend functions.
+      if (D->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend)) {
+        if (Ctx->getTargetInfo().getCXXABI().isMicrosoft())
+          Builder->EmitTopLevelDecl(D);
+        return;
+      }
+
+      // Otherwise, must be a method.
+      auto MD = cast<CXXMethodDecl>(D);
+
       // We may want to emit this definition. However, that decision might be
       // based on computing the linkage, and we have to defer that in case we
       // are inside of something that will change the method's final linkage,
       // e.g.
       //   typedef struct {
       //     void bar();
       //     void foo() { bar(); }
       //   } A;
-      DeferredInlineMethodDefinitions.push_back(D);
+      DeferredInlineMethodDefinitions.push_back(MD);
 
       // Provide some coverage mapping even for methods that aren't emitted.
       // Don't do this for templated classes though, as they may not be
       // instantiable.
-      if (!D->getParent()->getDescribedClassTemplate())
-        Builder->AddDeferredUnusedCoverageMapping(D);
+      if (!MD->getParent()->getDescribedClassTemplate())
+        Builder->AddDeferredUnusedCoverageMapping(MD);
     }
 
     /// HandleTagDeclDefinition - This callback is invoked each time a TagDecl
     /// to (e.g. struct, union, enum, class) is completed. This allows the
     /// client hack on the type, which can occur at any point in the file
     /// (because these can be defined in declspecs).
     void HandleTagDeclDefinition(TagDecl *D) override {
       if (Diags.hasErrorOccurred())

lib/Frontend/MultiplexConsumer.cpp

 
 bool MultiplexConsumer::HandleTopLevelDecl(DeclGroupRef D) {
   bool Continue = true;
   for (auto &Consumer : Consumers)
     Continue = Continue && Consumer->HandleTopLevelDecl(D);
   return Continue;
 }
 
-void MultiplexConsumer::HandleInlineMethodDefinition(CXXMethodDecl *D) {
+void MultiplexConsumer::HandleInlineFunctionDefinition(FunctionDecl *D) {
   for (auto &Consumer : Consumers)
-    Consumer->HandleInlineMethodDefinition(D);
+    Consumer->HandleInlineFunctionDefinition(D);
 }
 
 void MultiplexConsumer::HandleCXXStaticMemberVarInstantiation(VarDecl *VD) {
   for (auto &Consumer : Consumers)
     Consumer->HandleCXXStaticMemberVarInstantiation(VD);
 }
 
 void MultiplexConsumer::HandleInterestingDecl(DeclGroupRef D) {

lib/Parse/ParseCXXInlineMethods.cpp

if (LM.D)
     LM.D->getAsFunction()->setLateTemplateParsed(false);
 
   while (Tok.isNot(tok::eof))
     ConsumeAnyToken();
 
   if (Tok.is(tok::eof) && Tok.getEofData() == LM.D)
     ConsumeAnyToken();
 
-  if (CXXMethodDecl *MD = dyn_cast_or_null<CXXMethodDecl>(LM.D))
-    Actions.ActOnFinishInlineMethodDef(MD);
+  if (auto *FD = dyn_cast_or_null<FunctionDecl>(LM.D))
+    if (isa<CXXMethodDecl>(FD) ||
+        FD->isInIdentifierNamespace(Decl::IDNS_OrdinaryFriend))
+      Actions.ActOnFinishInlineFunctionDef(FD);
 }
 
 /// ParseLexedMemberInitializers - We finished parsing the member specification
 /// of a top (non-nested) C++ class. Now go over the stack of lexed data member
 /// initializers that were collected during its parsing and parse them all.
 void Parser::ParseLexedMemberInitializers(ParsingClass &Class) {
   bool HasTemplateScope = !Class.TopLevelClass && Class.TemplateScope;
   ParseScope ClassTemplateScope(this, Scope::TemplateParamScope,

lib/Sema/SemaDecl.cpp

Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Declarator &D,
   assert(D.isFunctionDeclarator() && "Not a function declarator!");
   Scope *ParentScope = FnBodyScope->getParent();
 
   D.setFunctionDefinitionKind(FDK_Definition);
   Decl *DP = HandleDeclarator(ParentScope, D, TemplateParameterLists);
   return ActOnStartOfFunctionDef(FnBodyScope, DP, SkipBody);
 }
 
-void Sema::ActOnFinishInlineMethodDef(CXXMethodDecl *D) {
-  Consumer.HandleInlineMethodDefinition(D);
+void Sema::ActOnFinishInlineFunctionDef(FunctionDecl *D) {
+  Consumer.HandleInlineFunctionDefinition(D);
 }
 
 static bool ShouldWarnAboutMissingPrototype(const FunctionDecl *FD, 
                              const FunctionDecl*& PossibleZeroParamPrototype) {
   // Don't warn about invalid declarations.
   if (FD->isInvalidDecl())
     return false;
 

test/CodeGenCXX/dllexport.cpp

 __declspec(dllexport) void redecl2();
                       void redecl2() {}
 
 // Friend functions
 // MSC-DAG: define dllexport void @"\01?friend1@@YAXXZ"()
 // GNU-DAG: define dllexport void @_Z7friend1v()
 // MSC-DAG: define dllexport void @"\01?friend2@@YAXXZ"()
 // GNU-DAG: define dllexport void @_Z7friend2v()
+// MSC-DAG: define weak_odr dllexport void @"\01?friend3@@YAXXZ"()
 struct FuncFriend {
   friend __declspec(dllexport) void friend1();
   friend __declspec(dllexport) void friend2();
+  friend __declspec(dllexport) void friend3() {}
 };
 __declspec(dllexport) void friend1() {}
                       void friend2() {}
 
 // Implicit declarations can be redeclared with dllexport.
 // MSC-DAG: define dllexport noalias i8* @"\01??2@{{YAPAXI|YAPEAX_K}}@Z"(
 // GNU-DAG: define dllexport noalias i8* @_Znw{{[yj]}}(
 void* alloc(__SIZE_TYPE__ n);