[ASTImporter] Imported FunctionDecls inherit __attribute__((noreturn)) and analyzer_noreturn attrs from existing functions

[martong]

Summary

This way some CTU false positives are eliminated.

[shafik]

clang/lib/AST/ASTImporter.cpp

7842

There are other attributes that apply to functions as well: nodiscard, maybe_unused and deprecated. Is there a reason not to support those as well?

[a_sidorin]

Hello Balasz,
In my opinion, importing and setting the attributes should be handled by the stuff used in InitializeImportedDecl(). Can we extend it or reuse the code? It will allow us not to miss the required actions while importing a new function too.

[a_sidorin]

Ouch! Sorry, Gabor -_\\

[martong]

In D68634#1700591, @a_sidorin wrote:

Hello Balasz,
In my opinion, importing and setting the attributes should be handled by the stuff used in InitializeImportedDecl(). Can we extend it or reuse the code? It will allow us not to miss the required actions while importing a new function too.

I was thinking about that too, but it turned out to be a way more intrusive change.

We have to work with the most recent existing decl (PrevDecl) of the FunctionDecl whose attribute we currently import.
We can get the PrevDecl only with the help of the lookup. We can't get it from the ToD param of InitializeImportedDecl because by the time when we call InitializeImportedDecl the redecl chain is not connected. So, we should pass PrevDecl then into GetImportedOrCreateDecl. It would require to change all call expressions of GetImportedOrCreateDecl (58 occurences). Besides, we would use the PrevDecl only in case of inheritable attributes, only they require this special care. Note that there are a bunch of non-inheritable attributes which are already handled correctly in InitializeImportedDecl.

clang/lib/AST/ASTImporter.cpp

7842

Different attributes require different handling.

E.g. C++11 [[noreturn]] requires diagnostics if the first declaration of a function does not specify a the attribute, but a later does:

void f();
[[noreturn]] void f(); // ERROR

If a function is declared with [[noreturn]] in one translation unit, and the same function is declared without [[noreturn]] in another translation unit, the program is ill-formed; no diagnostic required.

But this is not true to the GNU attribute((noreturn)).

Also, there are non-inheritable attributes, in their case we do not have to copy anything from the existing most-recent-decl.

Thus, I think I am going to change the patch's name to indicate we deal only with __attribute__((noreturn)) and with analyzer_noreturn.

[martong]

In D68634#1701192, @martong wrote:

In D68634#1700591, @a_sidorin wrote:

Hello Balasz,
In my opinion, importing and setting the attributes should be handled by the stuff used in InitializeImportedDecl(). Can we extend it or reuse the code? It will allow us not to miss the required actions while importing a new function too.

I was thinking about that too, but it turned out to be a way more intrusive change.

We have to work with the most recent existing decl (PrevDecl) of the FunctionDecl whose attribute we currently import.
We can get the PrevDecl only with the help of the lookup. We can't get it from the ToD param of InitializeImportedDecl because by the time when we call InitializeImportedDecl the redecl chain is not connected. So, we should pass PrevDecl then into GetImportedOrCreateDecl. It would require to change all call expressions of GetImportedOrCreateDecl (58 occurences). Besides, we would use the PrevDecl only in case of inheritable attributes, only they require this special care. Note that there are a bunch of non-inheritable attributes which are already handled correctly in InitializeImportedDecl.

Actually, giving it some more thought, I realized it would make sense to pass the PrevDecl into GetImportedOrCreateDecl because that way we could connect the redecl chain right after the new node is created, and before importing any other node. That could facilitate structural eq check in the import of subsequent dependent nodes (see https://github.com/Ericsson/clang/issues/506).
Anyway, this change would be quite a big change compared to the original small patch we have here.

clang/lib/AST/ASTImporter.cpp

Error ImportArrayChecked(const InContainerTy &InContainer, OIter Obegin) {
       return ImportArrayChecked(InContainer.begin(), InContainer.end(), Obegin);
     }
 
     Error ImportOverriddenMethods(CXXMethodDecl *ToMethod,
                                   CXXMethodDecl *FromMethod);
 
     Expected<FunctionDecl *> FindFunctionTemplateSpecialization(
         FunctionDecl *FromFD);
+
+    // Copy the inheritable attributes of the FunctionDecl `Old` into `New.
+    void CopyInheritedAttributes(FunctionDecl *Old, FunctionDecl *New);
   };
 
 template <typename InContainerTy>
 Error ASTNodeImporter::ImportTemplateArgumentListInfo(
     SourceLocation FromLAngleLoc, SourceLocation FromRAngleLoc,
     const InContainerTy &Container, TemplateArgumentListInfo &Result) {
   auto ToLAngleLocOrErr = import(FromLAngleLoc);
   if (!ToLAngleLocOrErr)
if (GetImportedOrCreateDecl(
       return ToFunction;
   }
 
   // Connect the redecl chain.
   if (FoundByLookup) {
     auto *Recent = const_cast<FunctionDecl *>(
           FoundByLookup->getMostRecentDecl());
     ToFunction->setPreviousDecl(Recent);
+    CopyInheritedAttributes(Recent, ToFunction);
     // FIXME Probably we should merge exception specifications.  E.g. In the
     // "To" context the existing function may have exception specification with
     // noexcept-unevaluated, while the newly imported function may have an
     // evaluated noexcept.  A call to adjustExceptionSpec() on the imported
     // decl and its redeclarations may be required.
   }
 
   ToFunction->setQualifierInfo(ToQualifierLoc);
if (auto ImportedOrErr = import(FromOverriddenMethod))
           (*ImportedOrErr)->getCanonicalDecl()));
     else
       ImportErrors =
           joinErrors(std::move(ImportErrors), ImportedOrErr.takeError());
   }
   return ImportErrors;
 }
 
+// This implementation is inspired by Sema::mergeDeclAttributes.
+void ASTNodeImporter::CopyInheritedAttributes(FunctionDecl *Old,
+                                              FunctionDecl *New) {
+  assert(&Old->getASTContext() == &New->getASTContext() &&
+         "Should copy attrs only from decl in the same context!");
+  if (AnalyzerNoReturnAttr *OldAttr = Old->getAttr<AnalyzerNoReturnAttr>()) {
+    if (!New->getAttr<AnalyzerNoReturnAttr>()) {
+      AnalyzerNoReturnAttr *NewAttr = OldAttr->clone(Importer.ToContext);
+      NewAttr->setInherited(true);
+      New->addAttr(NewAttr);
+    }
+  }
+  if (Old->isNoReturn() && !New->isNoReturn()) {
+    const FunctionType *NewType = cast<FunctionType>(New->getType());
+    FunctionType::ExtInfo NewTypeInfo = NewType->getExtInfo();
+    NewType = Importer.ToContext.adjustFunctionType(
+        NewType, NewTypeInfo.withNoReturn(true));
+    auto Quals = New->getType().getQualifiers().getAsOpaqueValue();
+    New->setType(QualType(NewType, Quals));
+  }
+}
+
 ASTImporter::ASTImporter(ASTContext &ToContext, FileManager &ToFileManager,
                          ASTContext &FromContext, FileManager &FromFileManager,
                          bool MinimalImport,
                          std::shared_ptr<ASTImporterSharedState> SharedState)
     : SharedState(SharedState), ToContext(ToContext), FromContext(FromContext),
       ToFileManager(ToFileManager), FromFileManager(FromFileManager),
       Minimal(MinimalImport), ODRHandling(ODRHandlingType::Conservative) {
 

clang/test/Analysis/Inputs/ctu-attr-noreturn-other.c

+int cond(void);
+void fatal(void); // Should inherit noreturn when imported.
+
+typedef struct {
+  int a;
+  int b;
+} Data;
+
+void ff(Data* data) {
+  if (cond()) {
+    fatal();
+    return;
+  }
+  data->a = 0;
+  data->b = 0;
+}

clang/test/Analysis/Inputs/ctu-attr-noreturn.externalDefMap.txt

+c:@F@ff ctu-attr-noreturn-other.c.ast

clang/test/Analysis/ctu-attr-noreturn.c

+// Test that imported function decls inherit attributes from existing functions
+// in the "to" ctx and this way false positives are eliminated.
+//
+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir2
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu \
+// RUN:   -emit-pch -o %t/ctudir2/ctu-attr-noreturn-other.c.ast \
+// RUN:   %S/Inputs/ctu-attr-noreturn-other.c
+// RUN: cp %S/Inputs/ctu-attr-noreturn.externalDefMap.txt \
+// RUN:    %t/ctudir2/externalDefMap.txt
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -std=c89 -analyze \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir2 \
+// RUN:   -verify %s
+
+// expected-no-diagnostics
+
+__attribute__((noreturn)) void fatal(void);
+
+typedef struct {
+  int a;
+  int b;
+} Data;
+
+void ff(Data* data);
+
+int caller(void) {
+  Data d;
+  ff(&d);
+  int i = (int)d.b;// If the import of inherited attrs fail then a bug is
+                   // reported here: "Assigned value is garbage or undefined".
+  return i;
+}

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase, ImportDefaultConstructibleLambdas) {
   FunctionDecl *ToF = Import(FromF, Lang_CXX2a);
   EXPECT_TRUE(ToF);
   TranslationUnitDecl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We have two lambda classes after the import.
   EXPECT_EQ(DeclCounter<CXXRecordDecl>().match(ToTU, cxxRecordDecl(isLambda())),
             2u);
 }
 
+struct ImportAttrs : ASTImporterOptionSpecificTestBase {};
+
+TEST_P(ImportAttrs, InheritedAnalyzerNoreturnShouldBeImported) {
+  getToTuDecl(
+      R"(
+      void f();
+      )",
+      Lang_C);
+  Decl *FromTU = getTuDecl(
+      R"(
+      void f();                                    /* From0 */
+      void f() __attribute__((analyzer_noreturn)); /* From1 */
+      void f();                                    /* From2 */
+      )",
+      Lang_C, "input0.c");
+
+  auto *From2 =
+      LastDeclMatcher<FunctionDecl>().match(FromTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(From2->hasAttrs());
+  ASSERT_TRUE(From2->getAttr<AnalyzerNoReturnAttr>());
+  auto *From1 = From2->getPreviousDecl();
+  ASSERT_TRUE(From1->hasAttrs());
+  ASSERT_TRUE(From1->getAttr<AnalyzerNoReturnAttr>());
+  auto *From0 = From1->getPreviousDecl();
+  ASSERT_FALSE(From0->hasAttrs());
+  ASSERT_FALSE(From0->getAttr<AnalyzerNoReturnAttr>());
+
+  auto *Imported = Import(From2, Lang_C);
+  EXPECT_TRUE(Imported->hasAttrs());
+  EXPECT_TRUE(Imported->getAttr<AnalyzerNoReturnAttr>());
+  EXPECT_EQ(Imported->getAttrs().size(), 1u);
+  EXPECT_TRUE(Imported->getPreviousDecl()->hasAttrs());
+  EXPECT_TRUE(Imported->getPreviousDecl()->getAttr<AnalyzerNoReturnAttr>());
+}
+
+TEST_P(ImportAttrs, InheritedNoreturnShouldBeImported) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      void f();
+      )",
+      Lang_C);
+  auto *To =
+      FirstDeclMatcher<FunctionDecl>().match(ToTU, functionDecl(hasName("f")));
+  ASSERT_FALSE(cast<FunctionType>(To->getType())->getNoReturnAttr());
+
+  Decl *FromTU = getTuDecl(
+      R"(
+      void f();                            /* From0 */
+      void f() __attribute__((noreturn));  /* From1 */
+      void f();                            /* From2 */
+      )",
+      Lang_C, "input0.cc");
+
+  auto *From2 =
+      LastDeclMatcher<FunctionDecl>().match(FromTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(cast<FunctionType>(From2->getType())->getNoReturnAttr());
+  auto *From1 = From2->getPreviousDecl();
+  ASSERT_TRUE(cast<FunctionType>(From1->getType())->getNoReturnAttr());
+  auto *From0 = From1->getPreviousDecl();
+  ASSERT_FALSE(cast<FunctionType>(From0->getType())->getNoReturnAttr());
+
+  auto *Imported = Import(From2, Lang_C);
+  EXPECT_TRUE(cast<FunctionType>(Imported->getType())->getNoReturnAttr());
+  EXPECT_TRUE(cast<FunctionType>(Imported->getPreviousDecl()->getType())
+                  ->getNoReturnAttr());
+}
+
+TEST_P(ImportAttrs, ImportShouldInheritExistingInheritableAttr) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      void f() __attribute__((analyzer_noreturn));
+      void f();
+      )",
+      Lang_C);
+  Decl *FromTU = getTuDecl(
+      R"(
+      void f();
+      )",
+      Lang_C, "input0.c");
+
+  auto *From = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("f")));
+
+  auto *To0 =
+      FirstDeclMatcher<FunctionDecl>().match(ToTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(To0->hasAttrs());
+  ASSERT_TRUE(To0->getAttr<AnalyzerNoReturnAttr>());
+  auto *To1 =
+      LastDeclMatcher<FunctionDecl>().match(ToTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(To1->hasAttrs());
+  ASSERT_TRUE(To1->getAttr<AnalyzerNoReturnAttr>());
+  ASSERT_TRUE(To1->getAttr<AnalyzerNoReturnAttr>()->isInherited());
+
+  // Should have an Inherited attribute.
+  auto *Imported = Import(From, Lang_C);
+  EXPECT_TRUE(Imported->hasAttrs());
+  ASSERT_TRUE(Imported->getAttr<AnalyzerNoReturnAttr>());
+  EXPECT_TRUE(Imported->getAttr<AnalyzerNoReturnAttr>()->isInherited());
+}
+
+TEST_P(ImportAttrs, ImportShouldInheritExistingNoreturnTypeSpec) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      void f() __attribute__((noreturn));
+      void f();
+      )",
+      Lang_C);
+  Decl *FromTU = getTuDecl(
+      R"(
+      void f();
+      )",
+      Lang_C, "input0.c");
+
+  auto *From = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("f")));
+
+  auto *To0 =
+      FirstDeclMatcher<FunctionDecl>().match(ToTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(cast<FunctionType>(To0->getType())->getNoReturnAttr());
+  auto *To1 =
+      LastDeclMatcher<FunctionDecl>().match(ToTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(cast<FunctionType>(To1->getType())->getNoReturnAttr());
+
+  // Should have an Inherited attribute as part of the type.
+  auto *Imported = Import(From, Lang_C);
+  EXPECT_TRUE(cast<FunctionType>(Imported->getType())->getNoReturnAttr());
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
                         DefaultTestValuesForRunOptions, );
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportVariables,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, LLDBLookupTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ConflictingDeclsWithLiberalStrategy,
                         DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportAttrs,
+                        DefaultTestValuesForRunOptions, );
+
 } // end namespace ast_matchers
 } // end namespace clang