[PATCH] D132918: [clang] Fix a crash in constant evaluation

kadircet closed this revision. kadircet added a comment. Landed as abd2b1a9d0421f99d3d132dc99af55ae52f3ac3e Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132918/new/ https://rev

[PATCH] D132918: [clang] Fix a crash in constant evaluation

shafik accepted this revision. shafik added a comment. LGTM, I spent some time debugging this and I don't see an obvious alternative. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132918/new/ https://reviews.llvm.org/D132918 __

[PATCH] D132918: [clang] Fix a crash in constant evaluation

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132918/new/ https://reviews.llvm.org/D132918

[PATCH] D132918: [clang] Fix a crash in constant evaluation

ilya-biryukov accepted this revision as: ilya-biryukov. ilya-biryukov added a comment. In D132918#3773237 , @shafik wrote: > This is helpful information but I am not sure this convinces me that > `EvaluateVarDecl` is the correct place to check. Why not i

[PATCH] D132918: [clang] Fix a crash in constant evaluation

kadircet added a comment. > This is helpful information but I am not sure this convinces me that > EvaluateVarDecl is the correct place to check. Why not in EvaluateDecl or > EvaluateStmt? Both locations we could also check. I have done the check inside `EvaluateVarDecl` because the invalid eva

[PATCH] D132918: [clang] Fix a crash in constant evaluation

shafik added a comment. In D132918#3766909 , @ilya-biryukov wrote: > I was just passing by, but wanted to add more context from our investigation > with @kadircet. > If variables with incomplete types appear inside non-template `constexpr` > function t

[PATCH] D132918: [clang] Fix a crash in constant evaluation

ilya-biryukov added a comment. I was just passing by, but wanted to add more context from our investigation with @kadircet. If variables with incomplete types appear inside non-template `constexpr` function this gets detected by a call to `CheckConstexprFunctionDefinition` inside `ActOnFinishFu

[PATCH] D132918: [clang] Fix a crash in constant evaluation

kadircet updated this revision to Diff 457566. kadircet added a comment. Add reproducer. I think the issue is about keeping constexpr functions valid even when their bodies contain invalid decls under certain instantiations, which I believe is the right behaviour. As the function body might be in

[PATCH] D132918: [clang] Fix a crash in constant evaluation

aaron.ballman added reviewers: aaron.ballman, clang-language-wg. aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. Marking this as requesting changes so we don't have another accidental commit. Repository: rG LLVM Gi

[PATCH] D132918: [clang] Fix a crash in constant evaluation

aaron.ballman added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4797 } +// Can't access properties of an incomplete type. +if (!RD->hasDefinition()) { kadircet wrote: > shafik wrote: > > erichkeane wrote: > > > It seems to me that we

[PATCH] D132918: [clang] Fix a crash in constant evaluation

kadircet added a comment. sorry, i missed the other comments around having a lit test. reverting the change. Comment at: clang/lib/AST/ExprConstant.cpp:4797 } +// Can't access properties of an incomplete type. +if (!RD->hasDefinition()) { shafik w

[PATCH] D132918: [clang] Fix a crash in constant evaluation

This revision was automatically updated to reflect the committed changes. Closed by commit rGa5ab650714d0: [clang] Fix a crash in constant evaluation (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132918/new/ https://reviews.l

[PATCH] D132918: [clang] Fix a crash in constant evaluation

shafik added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4797 } +// Can't access properties of an incomplete type. +if (!RD->hasDefinition()) { erichkeane wrote: > It seems to me that we shouldn't GET to this function with an incomple

[PATCH] D132918: [clang] Fix a crash in constant evaluation

erichkeane added a comment. Also, without a reduced example lit-test, we shouldn't be making changes. Use creduce or something, but PLEASE come back with a lit test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132918/new/ https://reviews.llvm.o

[PATCH] D132918: [clang] Fix a crash in constant evaluation

erichkeane added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4797 } +// Can't access properties of an incomplete type. +if (!RD->hasDefinition()) { It seems to me that we shouldn't GET to this function with an incomplete type. I sus

[PATCH] D132918: [clang] Fix a crash in constant evaluation

usaxena95 accepted this revision. usaxena95 added a comment. This revision is now accepted and ready to land. Thanks. This looks reasonable. As discussed offline, this verifiably fixes the crash but it is hard to come up with a reduced reproducer. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D132918: [clang] Fix a crash in constant evaluation

kadircet created this revision. kadircet added reviewers: usaxena95, ilya-biryukov. Herald added a project: All. kadircet requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This was showing up in our internal crash collector. I have no idea ho