[PATCH] D100667: [clang] Fix assert() crash when checking undeduced arg alignment
Adam Czachorowski via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 26 07:49:00 PDT 2021
adamcz added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:4543
// getTypeAlignInChars requires complete types
- if (ParamTy->isIncompleteType() || ArgTy->isIncompleteType() ||
- ParamTy->isUndeducedType() || ArgTy->isUndeducedType())
+ auto CheckTypeOK = [](QualType Ty) {
+ if (Ty->isIncompleteType() || Ty->isUndeducedType())
----------------
hokein wrote:
> The current fix is on top of kadir's fix, now the code becomes more add-hoc, it looks like a smell to me.
>
>
> It looks like `CheckArgAlignment` has an implicit invariant (the arg type should never be undeduced), and this invariant was enforced by dropping the invalid AST. Now with `RecoveryExpr`, we might break the invariant -- when the RecoveryExpr preserves an `undeduced` type, like the test case in this patch (which is done via the [code](https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaExprCXX.cpp#L1420)).
>
>
> I think a right fix is to fix in the `RecoveryExpr` side, `RecoveryExpr` should not preserve an undeduced type, we did it for function return type already (https://reviews.llvm.org/D87350), but it didn't cover all places. I think a general fixing place is `Sema::CreateRecoveryExpr`, if the passing `T` is undeduced, fallback to dependent type.
>
Ooh, interesting. Thanks for the info.
I updated the change to catch this in CreateRecoveryExpr as well. I kept my original changes too, they shouldn't harm anything and it's better than crashing.
Let me know what you think about this now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100667/new/
https://reviews.llvm.org/D100667
More information about the cfe-commits
mailing list