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

[Utkarsh Saxena via Phabricator via cfe-commits]

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 ACTION
  https://reviews.llvm.org/D142384/new/

https://reviews.llvm.org/D142384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

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.

Given all these, I would probably vote for not backporting this, but this is 
the chance if we want.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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 
`RecordDecl*` referring to a forward declaration of a struct with a definition, 
and iterate over its fields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

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 inspiration for a test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

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 we didn't manage to come up with a small 
repro (we spent 2 weeks on-and-off trying to come up with one).
This only breaks inside our internal builds with header modules, internal 
version of Bazel, multiple targets with modules and STL.

We'll keep trying to reproduce this, but can't promise any exact dates.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

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.org/D142384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Utkarsh Saxena via Phabricator via cfe-commits]

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://reviews.llvm.org/D142384/new/

https://reviews.llvm.org/D142384

Files:
  clang/lib/AST/Decl.cpp


Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4770,7 +4770,10 @@
 RecordDecl::field_iterator RecordDecl::field_begin() const {
   if (hasExternalLexicalStorage() && !hasLoadedFieldsFromExternalStorage())
 LoadFieldsFromExternalStorage();
-
+  // This is necessary for correctness for C++ with modules.
+  // FIXME: Come up with a test case that breaks without definition.
+  if (RecordDecl *D = getDefinition(); D && D != this)
+return D->field_begin();
   return field_iterator(decl_iterator(FirstDecl));
 }
 


Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4770,7 +4770,10 @@
 RecordDecl::field_iterator RecordDecl::field_begin() const {
   if (hasExternalLexicalStorage() && !hasLoadedFieldsFromExternalStorage())
 LoadFieldsFromExternalStorage();
-
+  // This is necessary for correctness for C++ with modules.
+  // FIXME: Come up with a test case that breaks without definition.
+  if (RecordDecl *D = getDefinition(); D && D != this)
+return D->field_begin();
   return field_iterator(decl_iterator(FirstDecl));
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Utkarsh Saxena via Phabricator via cfe-commits]

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/lib/AST/Decl.cpp


Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4769,7 +4769,10 @@
 RecordDecl::field_iterator RecordDecl::field_begin() const {
   if (hasExternalLexicalStorage() && !hasLoadedFieldsFromExternalStorage())
 LoadFieldsFromExternalStorage();
-
+  // This is necessary for correctness for C++ with modules.
+  // FIXME: Come up with a test case that breaks without definition.
+  if (RecordDecl *D = getDefinition(); D && D != this)
+return D->field_begin();
   return field_iterator(decl_iterator(FirstDecl));
 }
 


Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4769,7 +4769,10 @@
 RecordDecl::field_iterator RecordDecl::field_begin() const {
   if (hasExternalLexicalStorage() && !hasLoadedFieldsFromExternalStorage())
 LoadFieldsFromExternalStorage();
-
+  // This is necessary for correctness for C++ with modules.
+  // FIXME: Come up with a test case that breaks without definition.
+  if (RecordDecl *D = getDefinition(); D && D != this)
+return D->field_begin();
   return field_iterator(decl_iterator(FirstDecl));
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

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 asking the callers to request fields only when definition 
> is available.

Not sure about C, but ObjC stacktrace definitely looks like a potential bug to 
me.
We actually might introduce new failure modes (calling the function on the same 
`RecordDecl` without fields may start retuning different results).

However, we also need to fix this crash in C++, so I suggest to land this and 
see if this will cause any fallout.




Comment at: clang/lib/AST/Decl.cpp:4772
 LoadFieldsFromExternalStorage();
-
+  if (RecordDecl *D = getDefinition(); D && D != this)
+return D->field_begin();

Let's add a comment to make sure we avoid someone deleting this code. (Since we 
don't have a test).
```
// This is necessary for correctness for C++ with modules.
// FIXME: come up with a test case that breaks.
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Utkarsh Saxena via Phabricator via cfe-commits]

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) || getDefinition());` fails a handful 23 
tests in ObjC:

  Failed Tests (23):
Clang :: Analysis/CFContainers.mm
Clang :: Analysis/blocks.m
Clang :: Analysis/dtor-array.cpp
Clang :: Analysis/edges-new.mm
Clang :: Analysis/incorrect-checker-names.mm
Clang :: Analysis/malloc.mm
Clang :: Analysis/mig.mm
Clang :: Analysis/retain-release.m
Clang :: Analysis/retain-release.mm
Clang :: Analysis/stack-capture-leak-arc.mm
Clang :: Analysis/stack-capture-leak-no-arc.mm
Clang :: Analysis/taint-tester.cpp
Clang :: CodeGenObjCXX/encode.mm
Clang-Unit :: AST/./ASTTests/0/100
Clang-Unit :: AST/./ASTTests/1/100
Clang-Unit :: AST/./ASTTests/14/100
Clang-Unit :: AST/./ASTTests/16/100
Clang-Unit :: AST/./ASTTests/76/100
Clang-Unit :: AST/./ASTTests/77/100
Clang-Unit :: AST/./ASTTests/78/100
Clang-Unit :: AST/./ASTTests/79/100
Clang-Unit :: AST/./ASTTests/98/100
Clang-Unit :: AST/./ASTTests/99/100

`bin/clang  $SRC/llvm-project/clang/test/CodeGenObjCXX/encode.mm`
Stacktrace: https://pastebin.pl/view/c6f505a7

`assert((!isa(this) || getDefinition() || !FirstDecl) && "Field 
without a CXX definition ?");` works. So if there is no CXX definition then the 
fields are always empty and current callers are fine with that.

I think this is fine, and we should just use the definition when it is 
available without asking the callers to request fields only when definition is 
available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Utkarsh Saxena via Phabricator via cfe-commits]

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:
  clang/lib/AST/Decl.cpp


Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4769,7 +4769,8 @@
 RecordDecl::field_iterator RecordDecl::field_begin() const {
   if (hasExternalLexicalStorage() && !hasLoadedFieldsFromExternalStorage())
 LoadFieldsFromExternalStorage();
-
+  if (RecordDecl *D = getDefinition(); D && D != this)
+return D->field_begin();
   return field_iterator(decl_iterator(FirstDecl));
 }
 


Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -4769,7 +4769,8 @@
 RecordDecl::field_iterator RecordDecl::field_begin() const {
   if (hasExternalLexicalStorage() && !hasLoadedFieldsFromExternalStorage())
 LoadFieldsFromExternalStorage();
-
+  if (RecordDecl *D = getDefinition(); D && D != this)
+return D->field_begin();
   return field_iterator(decl_iterator(FirstDecl));
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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();
+  }

This change makes me nervous: calling `field_begin` / `fields` on a 
`CXXRecordDecl*` will now work, but calling those same functions on the same 
object that has been cast to `RecordDecl*` will still be broken. Can we change 
the accessors in the base class instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Utkarsh Saxena via Phabricator via cfe-commits]

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

Files:
  clang/include/clang/AST/DeclCXX.h


Index: clang/include/clang/AST/DeclCXX.h
===
--- clang/include/clang/AST/DeclCXX.h
+++ clang/include/clang/AST/DeclCXX.h
@@ -554,6 +554,21 @@
 
   bool hasDefinition() const { return DefinitionData || dataPtr(); }
 
+  RecordDecl::field_iterator field_begin() const {
+assert(hasDefinition() && "Definition not available to get fields.");
+return static_cast(getDefinition())->field_begin();
+  }
+
+  RecordDecl::field_iterator field_end() const {
+return field_iterator(decl_iterator());
+  }
+
+  RecordDecl::field_range fields() const {
+return field_range(field_begin(), field_end());
+  }
+
+  bool field_empty() const { return field_begin() == field_end(); }
+
   static CXXRecordDecl *Create(const ASTContext , TagKind TK, DeclContext 
*DC,
SourceLocation StartLoc, SourceLocation IdLoc,
IdentifierInfo *Id,


Index: clang/include/clang/AST/DeclCXX.h
===
--- clang/include/clang/AST/DeclCXX.h
+++ clang/include/clang/AST/DeclCXX.h
@@ -554,6 +554,21 @@
 
   bool hasDefinition() const { return DefinitionData || dataPtr(); }
 
+  RecordDecl::field_iterator field_begin() const {
+assert(hasDefinition() && "Definition not available to get fields.");
+return static_cast(getDefinition())->field_begin();
+  }
+
+  RecordDecl::field_iterator field_end() const {
+return field_iterator(decl_iterator());
+  }
+
+  RecordDecl::field_range fields() const {
+return field_range(field_begin(), field_end());
+  }
+
+  bool field_empty() const { return field_begin() == field_end(); }
+
   static CXXRecordDecl *Create(const ASTContext , TagKind TK, DeclContext *DC,
SourceLocation StartLoc, SourceLocation IdLoc,
IdentifierInfo *Id,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

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 version looks good to you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Utkarsh Saxena via Phabricator via cfe-commits]

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/D142384/new/

https://reviews.llvm.org/D142384

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclCXX.h


Index: clang/include/clang/AST/DeclCXX.h
===
--- clang/include/clang/AST/DeclCXX.h
+++ clang/include/clang/AST/DeclCXX.h
@@ -554,6 +554,11 @@
 
   bool hasDefinition() const { return DefinitionData || dataPtr(); }
 
+  RecordDecl::field_iterator field_begin() const override {
+assert(hasDefinition() && "Definition not available to get fields.");
+return static_cast(getDefinition())->field_begin();
+  }
+
   static CXXRecordDecl *Create(const ASTContext , TagKind TK, DeclContext 
*DC,
SourceLocation StartLoc, SourceLocation IdLoc,
IdentifierInfo *Id,
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -4220,7 +4220,7 @@
   using field_range = llvm::iterator_range>;
 
   field_range fields() const { return field_range(field_begin(), field_end()); 
}
-  field_iterator field_begin() const;
+  virtual field_iterator field_begin() const;
 
   field_iterator field_end() const {
 return field_iterator(decl_iterator());


Index: clang/include/clang/AST/DeclCXX.h
===
--- clang/include/clang/AST/DeclCXX.h
+++ clang/include/clang/AST/DeclCXX.h
@@ -554,6 +554,11 @@
 
   bool hasDefinition() const { return DefinitionData || dataPtr(); }
 
+  RecordDecl::field_iterator field_begin() const override {
+assert(hasDefinition() && "Definition not available to get fields.");
+return static_cast(getDefinition())->field_begin();
+  }
+
   static CXXRecordDecl *Create(const ASTContext , TagKind TK, DeclContext *DC,
SourceLocation StartLoc, SourceLocation IdLoc,
IdentifierInfo *Id,
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -4220,7 +4220,7 @@
   using field_range = llvm::iterator_range>;
 
   field_range fields() const { return field_range(field_begin(), field_end()); }
-  field_iterator field_begin() const;
+  virtual field_iterator field_begin() const;
 
   field_iterator field_end() const {
 return field_iterator(decl_iterator());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Utkarsh Saxena via Phabricator via cfe-commits]

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:
  clang/lib/AST/ExprConstant.cpp


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -6229,6 +6229,8 @@
 return false;
 
   const CXXRecordDecl *RD = Definition->getParent();
+  assert(RD->hasDefinition() && "Definition is needed to read the fields");
+  RD = RD->getDefinition();
   if (RD->getNumVBases()) {
 Info.FFDiag(CallLoc, diag::note_constexpr_virtual_base) << RD;
 return false;


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -6229,6 +6229,8 @@
 return false;
 
   const CXXRecordDecl *RD = Definition->getParent();
+  assert(RD->hasDefinition() && "Definition is needed to read the fields");
+  RD = RD->getDefinition();
   if (RD->getNumVBases()) {
 Info.FFDiag(CallLoc, diag::note_constexpr_virtual_base) << RD;
 return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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 be iterating through `RD`'s fields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142384

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Utkarsh Saxena via Phabricator via cfe-commits]

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/lib/AST/ExprConstant.cpp


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -6229,6 +6229,8 @@
 return false;
 
   const CXXRecordDecl *RD = Definition->getParent();
+  assert(RD->hasDefinition());
+  RD->getDefinition();
   if (RD->getNumVBases()) {
 Info.FFDiag(CallLoc, diag::note_constexpr_virtual_base) << RD;
 return false;


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -6229,6 +6229,8 @@
 return false;
 
   const CXXRecordDecl *RD = Definition->getParent();
+  assert(RD->hasDefinition());
+  RD->getDefinition();
   if (RD->getNumVBases()) {
 Info.FFDiag(CallLoc, diag::note_constexpr_virtual_base) << RD;
 return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Utkarsh Saxena via Phabricator via cfe-commits]

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: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -6229,6 +6229,8 @@
 return false;
 
   const CXXRecordDecl *RD = Definition->getParent();
+  if (RD->field_empty())
+RD = RD->getMostRecentDecl();
   if (RD->getNumVBases()) {
 Info.FFDiag(CallLoc, diag::note_constexpr_virtual_base) << RD;
 return false;


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -6229,6 +6229,8 @@
 return false;
 
   const CXXRecordDecl *RD = Definition->getParent();
+  if (RD->field_empty())
+RD = RD->getMostRecentDecl();
   if (RD->getNumVBases()) {
 Info.FFDiag(CallLoc, diag::note_constexpr_virtual_base) << RD;
 return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits