[clang] Do not crash when ArgTy is null in CheckArgAlignment

[adamcz]

Summary

No repro case just yet. I only observed a crash like this and noticed
ArgTy-Value = 0 in coredump.

[kadircet]

clang/lib/Sema/SemaChecking.cpp

4567

i think ArgTy becomes null after this operation. e.g. the function requires a pointer param, user provides a non-pointer arg in a context where recovery exprs would keep the ast call attached.

I've also spent quite some time trying to reproduce but no luck.

4657

i think before diving in here we should ensure this Expr doesn't have type or error dependence.

[adamcz]

added a test

[adamcz]

update commit description

[adamcz]

Added some containsErrors() calls, not sure if that's the right thing to do

[adamcz]

FYI The ArgTy.isNull() check is sufficient to fix this. The Arg->containsErrors() is not - it's false in this case, since it seems CXXDefaultArgExpr with RecoveryExpr inside seems to not contains errors, according to containsError(). I can't tell if that's by design, or a bug.

Haojian, please let me know if you think Arg->containsError() should be true or if we should be testing somewhere else, or just drop it and accept that CheckArgAlignment may be called with RecoveryExpr and have it check for ArgTy->isNull().

Basically, let me know what you think about this. Thanks!

[kadircet]

as discussed offline LG from my side.

regarding the default arg of a parameter containing an error, it seems to be effecting type information of the parameter even though the type is explicitly specified. so it feels like either:

• one cannot perform any semantic checks on a parmvardecl with an errornous default arg, hence error dependence bit should be propagated from default arg to the parameter, or
• there's an issue with the type checking logic itself and it shouldn't be making type of the var decl depend on the default argument (when it is explicitly specified in the declaration).

but i am not an expert in either, so these are just feelings :). But it'd be great if we got rid of the additional default arg containing error check in here one way or the other.

[hokein]

In D103825#2805760, @adamcz wrote:

FYI The ArgTy.isNull() check is sufficient to fix this. The Arg->containsErrors() is not - it's false in this case, since it seems CXXDefaultArgExpr with RecoveryExpr inside seems to not contains errors, according to containsError(). I can't tell if that's by design, or a bug.

Haojian, please let me know if you think Arg->containsError() should be true or if we should be testing somewhere else, or just drop it and accept that CheckArgAlignment may be called with RecoveryExpr and have it check for ArgTy->isNull().

Basically, let me know what you think about this. Thanks!

Thanks for the analysis.

Yeah, I think this is a bug in clang when computing the dependence of CXXDefaultArgExpr -- the CXXDefaultArgExpr should respect the dependence of its actual argument (getExpr()). I think we should fix it first.

Regarding to test it, the best way is to dump the AST node and verify the containsError is in the dump string.

[adamcz]

Thanks Haojian!
I sent out https://reviews.llvm.org/D103982 for the CXXDefaultArgExpr. I'll update this change after I submit that one.

[adamcz]

updated after the CXXDefaultArgExpr containsErrors() change.

[hokein]

Thanks! Really hope this is the last time to fix this crash.

clang/lib/Sema/SemaChecking.cpp

4574–4576

looks like the isNull is not needed to fix the crash, but let's be more defensive this time.

clang/lib/Sema/SemaChecking.cpp

void Sema::CheckArgAlignment(SourceLocation Loc, NamedDecl *FDecl,
   // If a function accepts a pointer or reference type
   if (!ParamTy->isPointerType() && !ParamTy->isReferenceType())
     return;
 
   // If the parameter is a pointer type, get the pointee type for the
   // argument too. If the parameter is a reference type, don't try to get
   // the pointee type for the argument.
   if (ParamTy->isPointerType())
     ArgTy = ArgTy->getPointeeType();
 
   // Remove reference or pointer
   ParamTy = ParamTy->getPointeeType();
 
   // Find expected alignment, and the actual alignment of the passed object.
   // getTypeAlignInChars requires complete types
-  if (ParamTy->isIncompleteType() || ArgTy->isIncompleteType() ||
-      ParamTy->isUndeducedType() || ArgTy->isUndeducedType())
+  if (ArgTy.isNull() || ParamTy->isIncompleteType() ||
+      ArgTy->isIncompleteType() || ParamTy->isUndeducedType() ||
+      ArgTy->isUndeducedType())
     return;
 
   CharUnits ParamAlign = Context.getTypeAlignInChars(ParamTy);
   CharUnits ArgAlign = Context.getTypeAlignInChars(ArgTy);
 
   // If the argument is less aligned than the parameter, there is a
   // potential alignment issue.
   if (ArgAlign < ParamAlign)
if (isa_and_nonnull<FunctionProtoType>(FT))
       Proto = cast<FunctionProtoType>(FDecl->getFunctionType());
   }
   if (Proto) {
     // For variadic functions, we may have more args than parameters.
     // For some K&R functions, we may have less args than parameters.
     const auto N = std::min<unsigned>(Proto->getNumParams(), Args.size());
     for (unsigned ArgIdx = 0; ArgIdx < N; ++ArgIdx) {
       // Args[ArgIdx] can be null in malformed code.
       if (const Expr *Arg = Args[ArgIdx]) {
         QualType ParamTy = Proto->getParamType(ArgIdx);
         QualType ArgTy = Arg->getType();
         CheckArgAlignment(Arg->getExprLoc(), FDecl, std::to_string(ArgIdx + 1),
                           ArgTy, ParamTy);
       }
     }
   }
 

clang/test/SemaCXX/recovery-expr-type.cpp

struct S { // expected-note {{candidate}}
   ~S();
 };
 template <typename T> S(T t) -> S<void *>;
 
 void baz() {
   bar(S(123)); // expected-error {{no matching conversion}}
 }
 } // namespace test11
+
+namespace test12 {
+// Verify we do not crash.
+void fun(int *foo = no_such_function()); // expected-error {{undeclared identifier}}
+void baz() { fun(); }
+} // namespace test12
+