[PATCH] D142384: [C++20] Fix a crash with modules.

usaxena95 added a comment. This does not seem to be a pressing issue as there are no other reported module/cpp20 related crashes which were fixed by this. Also since it has no tests, it should be fine to postpone it to the next release. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D142384: [C++20] Fix a crash with modules.

hans added a comment. I have no opinion about merging this, but I second the worry about not having a test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142384/new/ https://reviews.llvm.org/D142384 ___

[PATCH] D142384: [C++20] Fix a crash with modules.

steakhal added a comment. @usaxena95 @ilya-biryukov, @hans, I wonder if we should backport this change to `release/16.x`. Otherwise, clang-16 won't have this fix. WDYT? I'm also a bit worried that we don't have tests. And that we have "unexpected" sideeffects like what you mentioned @hans. Give

[PATCH] D142384: [C++20] Fix a crash with modules.

rsmith added a comment. In D142384#4102937 , @hans wrote: > I don't know if that's expected or not, but maybe that behavior change could > be used as an inspiration for a test case. At least this suggests a way to test this with a C++ unit test: get a

[PATCH] D142384: [C++20] Fix a crash with modules.

hans added a comment. It looks like this is making one of Chromium's clang plugins traverse (and flag) more code: https://bugs.chromium.org/p/chromium/issues/detail?id=1412769 No modules involved :) I don't know if that's expected or not, but maybe that behavior change could be used as an inspi

[PATCH] D142384: [C++20] Fix a crash with modules.

ilya-biryukov added a comment. In D142384#4101461 , @ChuanqiXu wrote: > It should be good to prevent crashes. But it looks not good that it doesn't > have a test. Do you have plans to add a test case for this soon? It would be great to have a test, but

[PATCH] D142384: [C++20] Fix a crash with modules.

ChuanqiXu added a comment. It should be good to prevent crashes. But it looks not good that it doesn't have a test. Do you have plans to add a test case for this soon? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142384/new/ https://reviews.llvm.

[PATCH] D142384: [C++20] Fix a crash with modules.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb6121432da79: [C++20] Fix a crash with modules. (authored by usaxena95). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D142384: [C++20] Fix a crash with modules.

usaxena95 updated this revision to Diff 494314. usaxena95 marked an inline comment as done. usaxena95 added a comment. Addressed comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142384/new/ https://reviews.llvm.org/D142384 Files: clang/li

[PATCH] D142384: [C++20] Fix a crash with modules.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. In D142384#4098650 , @usaxena95 wrote: > I think this is fine, and we should just use the definition when it is > available without aski

[PATCH] D142384: [C++20] Fix a crash with modules.

usaxena95 added a comment. In D142384#4096935 , @ilya-biryukov wrote: > @usaxena95 could you give an example of the code that fails the assertion? Is > it some of the tests? `assert(getDefinition())` fails about 3.6k tests. `assert(!isa(this) || getDef

[PATCH] D142384: [C++20] Fix a crash with modules.

ilya-biryukov added a comment. I don't see any issues with this, so happy to LGTM. @rsmith any concerns on your side? @usaxena95 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142384/new/ https://reviews.llvm.org/D142384 _

[PATCH] D142384: [C++20] Fix a crash with modules.

usaxena95 updated this revision to Diff 493926. usaxena95 added a comment. Moved to RecordDecl::field_begin. Assertion is no more valid in RecordDecl. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142384/new/ https://reviews.llvm.org/D142384 Files

[PATCH] D142384: [C++20] Fix a crash with modules.

rsmith added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:557-560 + RecordDecl::field_iterator field_begin() const { +assert(hasDefinition() && "Definition not available to get fields."); +return static_cast(getDefinition())->field_begin(); + } ---

[PATCH] D142384: [C++20] Fix a crash with modules.

usaxena95 updated this revision to Diff 493652. usaxena95 added a comment. Use definition based field access only for CXXRecordDecl. Avoid dynamic dispatch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142384/new/ https://reviews.llvm.org/D142384

[PATCH] D142384: [C++20] Fix a crash with modules.

ilya-biryukov added a reviewer: rsmith. ilya-biryukov added a comment. This fix looks very sensible to me and @rsmith confirmed this pattern that we are seeing might happen (seeing members of something that was demoted from declaration to a definition). @rsmith could you confirm the update versi

[PATCH] D142384: [C++20] Fix a crash with modules.

usaxena95 updated this revision to Diff 493538. usaxena95 added a comment. Moved the use of definition where fields are accessed. This now resolves several other C++20/Module related crashes as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142

[PATCH] D142384: [C++20] Fix a crash with modules.

usaxena95 updated this revision to Diff 493489. usaxena95 marked an inline comment as done. usaxena95 added a comment. Add message in the assertion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142384/new/ https://reviews.llvm.org/D142384 Files:

[PATCH] D142384: [C++20] Fix a crash with modules.

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:6233 + assert(RD->hasDefinition()); + RD->getDefinition(); if (RD->getNumVBases()) { I think it would make sense to use the definition of the class as `RD` here, since we're going to b

[PATCH] D142384: [C++20] Fix a crash with modules.

usaxena95 updated this revision to Diff 491661. usaxena95 edited the summary of this revision. usaxena95 added a comment. Use `getDefinition` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142384/new/ https://reviews.llvm.org/D142384 Files: clang

[PATCH] D142384: [C++20] Fix a crash with modules.

usaxena95 created this revision. Herald added a project: All. usaxena95 requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D142384 Files: clang/lib/AST/ExprConstant.cpp Index