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

[Aaron Ballman via Phabricator via cfe-commits]

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 shouldn't GET to this function with an incomplete type.  I suspect whoever calls this is doing so incorrectly.
> > Also note we only check in `ExprConstant.cpp` for `hasDefinition()` in one other place in `findCompleteObject` and that is around extern see: https://github.com/llvm/llvm-project/commit/c0d04a2567c22631595bed8092bc042bb91ea4ee#diff-255a21a02a8966766225831836d482547787baf9a770fbf67178ebb7d7347e27
> > It seems to me that we shouldn't GET to this function with an incomplete type. I suspect whoever calls this is doing so incorrectly.
> 
> Agreed, that's also my assumption. but we've been unable to get a minimal crasher. i am not a fan of landing these changes without reproducers but this was clearly fixing the issue we had (moreover, it's happening on invalid code).
> 
> moreover we're checking for recorddecl being invalid up above, so it felt quite likely to hit this code path with incomplete types as well (or there were some changes up the callstack that forgot to update the implementation here).
> Agreed, that's also my assumption. but we've been unable to get a minimal crasher. i am not a fan of landing these changes without reproducers but this was clearly fixing the issue we had (moreover, it's happening on invalid code).

It's hard to say that it actually is fixing the issue instead of papering over the root cause elsewhere in the project. Having test coverage helps us to determine whether the fix is correct or incorrect.

> moreover we're checking for recorddecl being invalid up above, so it felt quite likely to hit this code path with incomplete types as well (or there were some changes up the callstack that forgot to update the implementation here).

If the type is incomplete, why is the record not invalid? This smells like we're possibly missing a call to `RequireCompleteType()` somewhere else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132918