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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
