[CUDA] [AST] Allow isInlineDefinitionExternallyVisible to be called on functions without bodies.

[jlebar]

Summary

In CUDA compilation, we call isInlineDefinitionExternallyVisible (via
getGVALinkageForFunction) on functions while parsing their definitions.

At the point in time when we call getGVALinkageForFunction, the function
may not have a body, so we trip this assert. But as far as I can tell,
this is harmless.

It is not clear to me whether it's possible to replace this assert with
something slightly weaker.

[jlebar]

Friendly ping. This assert breaks basically all cuda compilation on certain libstdc++ versions. If this change is wrong, I sort of urgently need to figure out the right thing.

[tra]

I'm OK with the change, but the comments suggest that things may be more complicated.
How about disabling assert for CUDA only?

[jlebar]

In D25640#579238, @tra wrote:

I'm OK with the change, but the comments suggest that things may be more complicated.
How about disabling assert for CUDA only?

I don't think that is the right approach. This function has nothing to do with CUDA, so either the assertion is valid and we shouldn't be calling it this way, or it's not valid and we should remove the assertion.

Richard, would appreciate some help with this one.

[rsmith]

I'm not OK with losing this checking entirely. It seems like the problem case is when we're currently processing the definition of the function in question (so it /does/ have a body, it just doesn't have a /complete/ body yet)? I suppose we could track that directly somehow.

[jlebar]

Add WillHaveBody flag.

[jlebar]

OK, I can add new flags with the best of 'em.

I got rid of a super ugly hack I found that was working around the same problem I was trying to work around here. (And I verified that if I don't call setWillHaveBody, a testcase fails.) I can split this out into two patches if you prefer.

[rsmith]

Looks great, thanks.

clang/lib/Sema/SemaDecl.cpp

5762–5768 ↗

(On Diff #76118)

Wow.

cfe/trunk/include/clang/AST/Decl.h

private:
 
   /// \brief Indicates if the function uses __try.
   unsigned UsesSEHTry : 1;
 
   /// \brief Indicates if the function was a definition but its body was
   /// skipped.
   unsigned HasSkippedBody : 1;
 
+  /// Indicates if the function declaration will have a body, once we're done
+  /// parsing it.  (We don't set it to false when we're done parsing, in the
+  /// hopes this is simpler.)
+  unsigned WillHaveBody : 1;
+
   /// \brief End part of this FunctionDecl's source range.
   ///
   /// We could compute the full range in getSourceRange(). However, when we're
   /// dealing with a function definition deserialized from a PCH/AST file,
   /// we can only compute the full range once the function body has been
   /// de-serialized, so it's far better to have the (sometimes-redundant)
   /// EndRangeLoc.
   SourceLocation EndRangeLoc;
private:
   /// member function FD.
   void setInstantiationOfMemberFunction(ASTContext &C, FunctionDecl *FD,
                                         TemplateSpecializationKind TSK);
 
   void setParams(ASTContext &C, ArrayRef<ParmVarDecl *> NewParamInfo);
 
 protected:
   FunctionDecl(Kind DK, ASTContext &C, DeclContext *DC, SourceLocation StartLoc,
-               const DeclarationNameInfo &NameInfo,
-               QualType T, TypeSourceInfo *TInfo,
-               StorageClass S, bool isInlineSpecified,
+               const DeclarationNameInfo &NameInfo, QualType T,
+               TypeSourceInfo *TInfo, StorageClass S, bool isInlineSpecified,
                bool isConstexprSpecified)
       : DeclaratorDecl(DK, DC, NameInfo.getLoc(), NameInfo.getName(), T, TInfo,
                        StartLoc),
-      DeclContext(DK),
-      redeclarable_base(C),
-      ParamInfo(nullptr), Body(),
-      SClass(S),
-      IsInline(isInlineSpecified), IsInlineSpecified(isInlineSpecified),
-      IsVirtualAsWritten(false), IsPure(false), HasInheritedPrototype(false),
-      HasWrittenPrototype(true), IsDeleted(false), IsTrivial(false),
-      IsDefaulted(false), IsExplicitlyDefaulted(false),
-      HasImplicitReturnZero(false), IsLateTemplateParsed(false),
-      IsConstexpr(isConstexprSpecified), UsesSEHTry(false),
-      HasSkippedBody(false), EndRangeLoc(NameInfo.getEndLoc()),
-      TemplateOrSpecialization(),
+        DeclContext(DK), redeclarable_base(C), ParamInfo(nullptr), Body(),
+        SClass(S), IsInline(isInlineSpecified),
+        IsInlineSpecified(isInlineSpecified), IsVirtualAsWritten(false),
+        IsPure(false), HasInheritedPrototype(false), HasWrittenPrototype(true),
+        IsDeleted(false), IsTrivial(false), IsDefaulted(false),
+        IsExplicitlyDefaulted(false), HasImplicitReturnZero(false),
+        IsLateTemplateParsed(false), IsConstexpr(isConstexprSpecified),
+        UsesSEHTry(false), HasSkippedBody(false), WillHaveBody(false),
+        EndRangeLoc(NameInfo.getEndLoc()), TemplateOrSpecialization(),
         DNLoc(NameInfo.getInfo()) {}
 
   typedef Redeclarable<FunctionDecl> redeclarable_base;
   FunctionDecl *getNextRedeclarationImpl() override {
     return getNextRedeclaration();
   }
   FunctionDecl *getPreviousDeclImpl() override {
     return getPreviousDecl();
   }
public:
   /// \brief Determines whether this function is known to be 'noreturn', through
   /// an attribute on its declaration or its type.
   bool isNoReturn() const;
 
   /// \brief True if the function was a definition but its body was skipped.
   bool hasSkippedBody() const { return HasSkippedBody; }
   void setHasSkippedBody(bool Skipped = true) { HasSkippedBody = Skipped; }
 
+  /// True if this function will eventually have a body, once it's fully parsed.
+  bool willHaveBody() const { return WillHaveBody; }
+  void setWillHaveBody(bool V = true) { WillHaveBody = V; }
+
   void setPreviousDeclaration(FunctionDecl * PrevDecl);
 
   FunctionDecl *getCanonicalDecl() override;
   const FunctionDecl *getCanonicalDecl() const {
     return const_cast<FunctionDecl*>(this)->getCanonicalDecl();
   }
 
   unsigned getBuiltinID() const;

cfe/trunk/lib/AST/Decl.cpp

 
 /// \brief For a function declaration in C or C++, determine whether this
 /// declaration causes the definition to be externally visible.
 ///
 /// For instance, this determines if adding the current declaration to the set
 /// of redeclarations of the given functions causes
 /// isInlineDefinitionExternallyVisible to change from false to true.
 bool FunctionDecl::doesDeclarationForceExternallyVisibleDefinition() const {
-  assert(!doesThisDeclarationHaveABody() &&
+  assert(!doesThisDeclarationHaveABody() && !willHaveBody() &&
          "Must have a declaration without a body.");
 
   ASTContext &Context = getASTContext();
 
   if (Context.getLangOpts().MSVCCompat) {
     const FunctionDecl *Definition;
     if (hasBody(Definition) && Definition->isInlined() &&
         redeclForcesDefMSVC(this))

 /// inline definition becomes externally visible (C99 6.7.4p6).
 ///
 /// In GNU89 mode, or if the gnu_inline attribute is attached to the function
 /// definition, we use the GNU semantics for inline, which are nearly the 
 /// opposite of C99 semantics. In particular, "inline" by itself will create 
 /// an externally visible symbol, but "extern inline" will not create an 
 /// externally visible symbol.
 bool FunctionDecl::isInlineDefinitionExternallyVisible() const {
-  assert(doesThisDeclarationHaveABody() && "Must have the function definition");
+  assert(doesThisDeclarationHaveABody() ||
+         willHaveBody() && "Must be a function definition");
   assert(isInlined() && "Function must be inline");
   ASTContext &Context = getASTContext();
   
   if (Context.getLangOpts().GNUInline || hasAttr<GNUInlineAttr>()) {
     // Note: If you change the logic here, please change
     // doesDeclarationForceExternallyVisibleDefinition as well.
     //
     // If it's not the case that both 'inline' and 'extern' are

cfe/trunk/lib/Sema/SemaDecl.cpp

static bool isFunctionDefinitionDiscarded(Sema &S, FunctionDecl *FD) {
   // All cases of this require the 'inline' keyword.
   if (!FD->isInlined()) return false;
 
   // This is only possible in C++ with the gnu_inline attribute.
   if (S.getLangOpts().CPlusPlus && !FD->hasAttr<GNUInlineAttr>())
     return false;
 
   // Okay, go ahead and call the relatively-more-expensive function.
-
-#ifndef NDEBUG
-  // AST quite reasonably asserts that it's working on a function
-  // definition.  We don't really have a way to tell it that we're
-  // currently defining the function, so just lie to it in +Asserts
-  // builds.  This is an awful hack.
-  FD->setLazyBody(1);
-#endif
-
-  bool isC99Inline =
-      S.Context.GetGVALinkageForFunction(FD) == GVA_AvailableExternally;
-
-#ifndef NDEBUG
-  FD->setLazyBody(0);
-#endif
-
-  return isC99Inline;
+  return S.Context.GetGVALinkageForFunction(FD) == GVA_AvailableExternally;
 }
 
 /// Determine whether a variable is extern "C" prior to attaching
 /// an initializer. We can't just call isExternC() here, because that
 /// will also compute and cache whether the declaration is externally
 /// visible, which might change when we attach the initializer.
 ///
 /// This can only be used if the declaration is known to not be a
Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
   if (!FD->isLateTemplateParsed()) {
     CheckForFunctionRedefinition(FD, nullptr, SkipBody);
 
     // If we're skipping the body, we're done. Don't enter the scope.
     if (SkipBody && SkipBody->ShouldSkip)
       return D;
   }
 
+  // Mark this function as "will have a body eventually".  This lets users to
+  // call e.g. isInlineDefinitionExternallyVisible while we're still parsing
+  // this function.
+  FD->setWillHaveBody();
+
   // If we are instantiating a generic lambda call operator, push
   // a LambdaScopeInfo onto the function stack.  But use the information
   // that's already been calculated (ActOnLambdaExpr) to prime the current
   // LambdaScopeInfo.
   // When the template operator is being specialized, the LambdaScopeInfo,
   // has to be properly restored so that tryCaptureVariable doesn't try
   // and capture any new variables. In addition when calculating potential
   // captures during transformation of nested lambdas, it is necessary to

cfe/trunk/test/SemaCUDA/gnu-inline.cu

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#include "Inputs/cuda.h"
+
+// expected-no-diagnostics
+
+// Check that we can handle gnu_inline functions when compiling in CUDA mode.
+
+void foo();
+inline __attribute__((gnu_inline)) void bar() { foo(); }