[clang][ASTImporter] Fix of unchecked Error object

[balazske]

After a previous fix and commit 30a9cac error handling in function 'importTemplateParameterDefaultArgument' was not correct because std::move was removed from return of an Error object and this caused crash "Error value was Success" in some cases.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)

Changes

After a previous fix and commit 30a9cac error handling in function 'importTemplateParameterDefaultArgument' was not correct because std::move was removed from return of an Error object and this caused crash "Error value was Success" in some cases.

---

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

1 Files Affected:

• (modified) clang/lib/AST/ASTImporter.cpp (+13-13)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 020a2f396b5aa0..e7a6509167f0a0 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -362,24 +362,24 @@ namespace clang {
     template <typename TemplateParmDeclT>
     Error importTemplateParameterDefaultArgument(const TemplateParmDeclT *D,
                                                  TemplateParmDeclT *ToD) {
-      Error Err = Error::success();
       if (D->hasDefaultArgument()) {
         if (D->defaultArgumentWasInherited()) {
-          auto *ToInheritedFrom = const_cast<TemplateParmDeclT *>(
-              importChecked(Err, D->getDefaultArgStorage().getInheritedFrom()));
-          if (Err)
-            return Err;
+          Expected<TemplateParmDeclT *> ToInheritedFromOrErr =
+              import(D->getDefaultArgStorage().getInheritedFrom());
+          if (!ToInheritedFromOrErr)
+            return ToInheritedFromOrErr.takeError();
+          TemplateParmDeclT *ToInheritedFrom = *ToInheritedFromOrErr;
           if (!ToInheritedFrom->hasDefaultArgument()) {
             // Resolve possible circular dependency between default value of the
             // template argument and the template declaration.
-            const auto ToInheritedDefaultArg =
-                importChecked(Err, D->getDefaultArgStorage()
-                                       .getInheritedFrom()
-                                       ->getDefaultArgument());
-            if (Err)
-              return Err;
+            Expected<TemplateArgumentLoc> ToInheritedDefaultArgOrErr =
+                import(D->getDefaultArgStorage()
+                           .getInheritedFrom()
+                           ->getDefaultArgument());
+            if (!ToInheritedDefaultArgOrErr)
+              return ToInheritedDefaultArgOrErr.takeError();
             ToInheritedFrom->setDefaultArgument(Importer.getToContext(),
-                                                ToInheritedDefaultArg);
+                                                *ToInheritedDefaultArgOrErr);
           }
           ToD->setInheritedDefaultArgument(ToD->getASTContext(),
                                            ToInheritedFrom);
@@ -395,7 +395,7 @@ namespace clang {
                                     *ToDefaultArgOrErr);
         }
       }
-      return Err;
+      return Error::success();
     }
 
   public:

[NagyDonat]

The change itself LGTM, but I'm a bit confused by the commit message.

Do I understand it correctly that the import was crashing (more precisely running into an assertion failure) on some input and this commit prevents this crash? If yes, then this is not an NFC change (but a valuable bugfix 🙂 ), so you should not mark it as NFC.

Also, did the apparently NFC removal of std::move influence anything in the behavior of this code? Do you only mention it because it's a recent modification of this code, or did the crash appear when it was merged?

[balazske]

I do not want now to check what is exactly happening, the case is a bit interesting. Normally return Err should work and is used at other functions too. And why did not a similar crash happen at the unit tests? Probably it is caused by different compilation on other platform or when "copy elision" does not happen. It looks difficult to tell if this is a NFC or not, if not there should be a test to reproduce the crash but tests for default template arguments exist already (at least the affected function should be called from some test).