[clang][RecoveryExpr] Don't perform alignment check if parameter type is dependent

[ArcsinX]

Summary

This patch fixes a crash which appears because of getTypeAlignInChars() call with depentent type.

[ArcsinX]

This looks similar to the problem fixed in D103825

[hokein]

clang/lib/Sema/SemaChecking.cpp

5787

It looks like for the failure case the ParamTy for the parameter is a dependent array type, and it violates the "non-dependent" assumption of clang::ASTContext::getTypeInfoImpl which is called by getTypeAlignInChars in CheckArgAlignment.

so I'd suggest moving the fix to CheckArgAlignment line 5685 (adding a ParamTy->isDependentType() to the if condition).

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

171

we can simplify it further:

void m(int (&)[undefined()]) {}
...


S.m(1);

[ArcsinX]

clang/lib/Sema/SemaChecking.cpp

5787

When I found this problem I fixed it like you are suggesting. But after that I replaced it with this check, because it seems there is no reason to try to check something on code with errors.

I mean that even if we are not crashing, results from CheckArgAlignment() can't be trusted if we are passing something with errors to it.

[hokein]

clang/lib/Sema/SemaChecking.cpp

5787

because it seems there is no reason to try to check something on code with errors.

I think this is case-by-case (for this case, it may be true) -- in some cases (see test7 and test8 for example in recovery-expr-type.cpp), we do want to check something even on the code with errors to emit useful secondary diagnostics. In general we want to emit a full list of diagnostics and at the same time avoid any suspicious diagnostics, however achieving both goals is hard.

Depending on how fatal the error is

• if the error is fatal, RecoveryError is just a dependent-type wrapper, we should not call check* to emit diagnostics (we're less certain about the quality of these diagnostics), this is mostly done by leveraging on the existing template dependent mechanism;
• if the error is minor, RecoveryError preserves a concrete type, we might want call check* to emit diagnostics as we're more confident;

The current solution makes sense to fix the crash, but I think the main reason is that CheckArgAlignment now can be invoked with a dependent-type parameter in non-template context (after we extended the "dependent" concept from depending on template parameters to depending on template parameters and errors), so we should fix CheckArgAlignment.

[ArcsinX]

• Check for dependent type inside CheckArgAlignment()
• Simplify test

[ArcsinX]

clang/lib/Sema/SemaChecking.cpp

5787

Thanks for clarification. Moved check inside CheckArgAlignment() as you suggested.

[hokein]

Thanks!

clang/lib/Sema/SemaChecking.cpp

void Sema::CheckArgAlignment(SourceLocation Loc, NamedDecl *FDecl,
   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 (ArgTy.isNull() || ParamTy->isIncompleteType() ||
-      ArgTy->isIncompleteType() || ParamTy->isUndeducedType() ||
-      ArgTy->isUndeducedType())
+  if (ArgTy.isNull() || ParamTy->isDependentType() ||
+      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)
for (unsigned ArgIdx = 0; ArgIdx < N; ++ArgIdx) {
 
         if (Context.getTargetInfo().getTriple().isOSAIX() && FDecl && Arg &&
             FDecl->hasLinkage() &&
             FDecl->getFormalLinkage() != InternalLinkage &&
             CallType == VariadicDoesNotApply)
           checkAIXMemberAlignment((Arg->getExprLoc()), Arg);
 
         QualType ParamTy = Proto->getParamType(ArgIdx);
         QualType ArgTy = Arg->getType();
         CheckArgAlignment(Arg->getExprLoc(), FDecl, std::to_string(ArgIdx + 1),
                           ArgTy, ParamTy);
       }
     }
   }
 
   if (FDecl && FDecl->hasAttr<AllocAlignAttr>()) {
     auto *AA = FDecl->getAttr<AllocAlignAttr>();

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

 extern "C" void *memset(void *, int b, unsigned long) {
   int * const c(undef()); // expected-error {{undeclared identifier}}
   // Verify we do not crash on evaluating *c whose initializer is a NULL-type ParenListExpr!
   memset(c, 0, *c); // crash1
 
   b = __builtin_object_size(c, 0); // crash2
 }
 }
+
+namespace test15 {
+void f() {
+  struct {
+    void m(int (&)[undefined()]) {} // expected-error {{undeclared identifier}}
+  } S;
+  S.m(1); // no crash
+}
+}