[PATCH] D133886: [clang][RecoveryExpr] Don't perform alignment check if parameter type contains errors

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:5779
         QualType ParamTy = Proto->getParamType(ArgIdx);
+        if (ParamTy->containsErrors())
+          continue;
----------------
ArcsinX wrote:
> hokein wrote:
> > 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).
> 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.
> 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`.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133886/new/

https://reviews.llvm.org/D133886