from:"Michael Jabbour via cfe\-commits"

[clang] [clang] Fix crash after merging named enums (PR #114240)

2024-10-30 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Clean up enumerators when merging named enums (PR #114240)

2024-10-30 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource created 
https://github.com/llvm/llvm-project/pull/114240

Clang already has a mechanism to cleanup enumerators for typedefs to anonymous 
enums. So the following example code used to be handled correctly while 
merging, and ASTWriter behaves as expected:

```c
typedef enum { Val } AnonEnum;
```
However, the mentioned mechanism didn't handle named enums. This lead to stale 
declarations in `IdResolver`, causing an assertion violation in ASTWriter 
`Assertion `DeclIDs.contains(D) && "Declaration not emitted!"' failed.` when a 
module is being serialized with the following merged enums:

```c
typedef enum Enum1 { Val_A } Enum1;
enum Enum2 { Val_B };
```

The PR applies the same mechanism in the named enums case. Additionally, I 
dropped the call to `getLexicalDeclContext()->removeDecl` as it was causing a 
wrong odr-violation diagnostic with anonymous enums.

Might be easier to to review commit by commit. Any feedback is appreciated.

### Context

This fixes frontend crashes that were encountered when certain Objective-C 
modules are included on Xcode 16. For example, by running `CC=/path/to/clang-19 
xcodebuild clean build` on a project that contains the following Objective-C 
file:

```c
#include 

int main() {
  return memory_order_relaxed;
}
```
This crashes the parser in release, when ASTReader tried to load the enumerator 
declaration.

>From cc3cf25da67c9f8b9edabb318c6011cad9bd2f58 Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 11:16:09 +0100
Subject: [PATCH 1/4] [NFC] Factor out RetireNodesFromMergedDecl

---
 clang/include/clang/Sema/Sema.h |  6 ++
 clang/lib/Sema/SemaDecl.cpp | 27 +++
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 93d98e1cbb9c81..70bbb882eb2b13 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4034,6 +4034,12 @@ class Sema final : public SemaBase {
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 LookupResult &OldDecls);
 
+  /// RetireNodesFromMergedDecl - We have just merged the decl 'New' by making
+  /// another definition visible.
+  /// This method performs any necessary cleanup on the parser state to discard
+  /// child nodes from newly parsed decl we are retiring.
+  void RetireNodesFromMergedDecl(Scope *S, Decl *New);
+
   /// MergeFunctionDecl - We just parsed a function 'New' from
   /// declarator D which has the same name and scope as a previous
   /// declaration 'Old'.  Figure out how to resolve this situation,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index f8e5f3c6d309d6..54c4c5f4e5395e 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);
-}
-  }
+  RetireNodesFromMergedDecl(S, NewTag);
 }
   }
 
@@ -2639,6 +2628,20 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   notePreviousDefinition(Old, New->getLocation());
 }
 
+void Sema::RetireNodesFromMergedDecl(Scope *S, Decl *New) {
+  // If this was an unscoped enumeration, yank all of its enumerators
+  // out of the scope.
+  if (auto *ED = dyn_cast(New); ED) {
+Scope *EnumScope = getNonFieldDeclScope(S);
+for (auto *ECD : ED->enumerators()) {
+  assert(EnumScope->isDeclScope(ECD));
+  EnumScope->RemoveDecl(ECD);
+  IdResolver.RemoveDecl(ECD);
+  ECD->getLexicalDeclContext()->removeDecl(ECD);
+}
+  }
+}
+
 /// DeclhasAttr - returns true if decl Declaration already has the target
 /// attribute.
 static bool DeclHasAttr(const Decl *D, const Attr *A) {

>From f07904ae5468658a3e7e4a649cd183734479d6cc Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 12:07:29 +0100
Subject: [PATCH 2/4] Drop enumerators when merging named enums

This fixes a crash in ASTWriter when the stale IdResolver entries are
used when writing the identifier table.

  assert(DeclIDs.contains(D) && "Declaration not emitted!");
---
 clang/include/clang/Sema/Sema.h |  2 +-
 clang/lib/Parse/ParseDecl.cpp   |  2 +-
 clang/lib/Parse/ParseDeclCXX.cpp|  3 +-
 clang/lib/Sema/SemaDecl.cpp |  6 ++-
 clang/test/Modules/modules-merge-enum.m | 52 +
 5 files changed, 60 insertions(+), 5 deletion

[clang] [clang] Clean up enumerators when merging named enums (PR #114240)

2024-10-30 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Clean up enumerators when merging named enums (PR #114240)

2024-10-30 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Clean up enumerators when merging named enums (PR #114240)

2024-10-30 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Clean up enumerators when merging named enums (PR #114240)

2024-10-30 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Clean up enumerators when merging named enums (PR #114240)

2024-10-30 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2024-10-31 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2024-10-31 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2024-10-31 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2024-11-06 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Ping?

Note that this PR fixes a crash in ASTWriter on valid Objective-C modules code; 
The initial PR title didn't accurately reflect that.

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-06 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Gentle ping, and I wish you all a happy new year. :smile: 

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-02-05 Thread Michael Jabbour via cfe-commits

@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);

-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);

michael-jabbour-sonarsource wrote:

Thank you all for looking into this :pray:

> Can we solve this issue by reusing some level of modules-specific declaration 
> shadowing?

I am new to modules, and I only learned how this part of the code works while 
debugging the crash I am sharing here. I am not entirely sure I understand what 
this means, could you provide some pointers that I can start from (e.g. 
function names, commits, ...etc)?

I can try to dig into this at the end of this week or next week if needed...

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-02-05 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-02-05 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-02-05 Thread Michael Jabbour via cfe-commits

@@ -2639,6 +2628,19 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   notePreviousDefinition(Old, New->getLocation());
 }

+void Sema::CleanupMergedEnum(Scope *S, Decl *New) {

michael-jabbour-sonarsource wrote:

> Is MergeTypedefNameDecl not called for the motivating example?

Unfortunately, it seems to me that `MergeTypedefNameDecl` is only called in the 
typedef to anonymous enum case (`MyEnum3` in the test case I am adding). For 
the rest of the enum cases, the closest I found was 
`Sema::ActOnDuplicateDefinition`, which is called when merging all tags (and 
this is where I am adding the new call to `CleanupMergedEnum`).

> Can we find a common path when merging definitions to put that logic there?

I could only see that merging enums for C and Obj-C crashes in this case (C++ 
works differently, see 
[here](https://github.com/llvm/llvm-project/pull/114240#issuecomment-2614544626)),
 and I found `Sema::ActOnDuplicateDefinition` during my investigation to be the 
function that handles merging tags.

I am not aware of a central place for merging definitions in general. Could you 
provide some hints on what to look for?

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2024-12-16 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Thanks for the update @vsapsai. Sounds good. Let me know if there are any 
changes that can make reviewing the PR easier!

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2024-11-17 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Gentle ping :smile:

The aim of the PR is to fix an ASTWriter crash that currently happens when 
serializing merged enums. Such a case was encountered while parsing Objective-C 
code that uses the XCode 16 SDK. If any of the changes look risky, I am happy 
to rework them and/or add more tests to increase confidence.

Thanks in advance.

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2024-12-03 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Gentle ping :smile:

I would love to hear your feedback, and if any changes are needed, I am happy 
to address them promptly.

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-21 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource updated 
https://github.com/llvm/llvm-project/pull/114240

>From cc3cf25da67c9f8b9edabb318c6011cad9bd2f58 Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 11:16:09 +0100
Subject: [PATCH 1/6] [NFC] Factor out RetireNodesFromMergedDecl

---
 clang/include/clang/Sema/Sema.h |  6 ++
 clang/lib/Sema/SemaDecl.cpp | 27 +++
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 93d98e1cbb9c81..70bbb882eb2b13 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4034,6 +4034,12 @@ class Sema final : public SemaBase {
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 LookupResult &OldDecls);
 
+  /// RetireNodesFromMergedDecl - We have just merged the decl 'New' by making
+  /// another definition visible.
+  /// This method performs any necessary cleanup on the parser state to discard
+  /// child nodes from newly parsed decl we are retiring.
+  void RetireNodesFromMergedDecl(Scope *S, Decl *New);
+
   /// MergeFunctionDecl - We just parsed a function 'New' from
   /// declarator D which has the same name and scope as a previous
   /// declaration 'Old'.  Figure out how to resolve this situation,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index f8e5f3c6d309d6..54c4c5f4e5395e 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);
-}
-  }
+  RetireNodesFromMergedDecl(S, NewTag);
 }
   }
 
@@ -2639,6 +2628,20 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   notePreviousDefinition(Old, New->getLocation());
 }
 
+void Sema::RetireNodesFromMergedDecl(Scope *S, Decl *New) {
+  // If this was an unscoped enumeration, yank all of its enumerators
+  // out of the scope.
+  if (auto *ED = dyn_cast(New); ED) {
+Scope *EnumScope = getNonFieldDeclScope(S);
+for (auto *ECD : ED->enumerators()) {
+  assert(EnumScope->isDeclScope(ECD));
+  EnumScope->RemoveDecl(ECD);
+  IdResolver.RemoveDecl(ECD);
+  ECD->getLexicalDeclContext()->removeDecl(ECD);
+}
+  }
+}
+
 /// DeclhasAttr - returns true if decl Declaration already has the target
 /// attribute.
 static bool DeclHasAttr(const Decl *D, const Attr *A) {

>From f07904ae5468658a3e7e4a649cd183734479d6cc Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 12:07:29 +0100
Subject: [PATCH 2/6] Drop enumerators when merging named enums

This fixes a crash in ASTWriter when the stale IdResolver entries are
used when writing the identifier table.

  assert(DeclIDs.contains(D) && "Declaration not emitted!");
---
 clang/include/clang/Sema/Sema.h |  2 +-
 clang/lib/Parse/ParseDecl.cpp   |  2 +-
 clang/lib/Parse/ParseDeclCXX.cpp|  3 +-
 clang/lib/Sema/SemaDecl.cpp |  6 ++-
 clang/test/Modules/modules-merge-enum.m | 52 +
 5 files changed, 60 insertions(+), 5 deletions(-)
 create mode 100644 clang/test/Modules/modules-merge-enum.m

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 70bbb882eb2b13..d53bcf63dd1bd9 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3910,7 +3910,7 @@ class Sema final : public SemaBase {
   /// Perform ODR-like check for C/ObjC when merging tag types from modules.
   /// Differently from C++, actually parse the body and reject / error out
   /// in case of a structural mismatch.
-  bool ActOnDuplicateDefinition(Decl *Prev, SkipBodyInfo &SkipBody);
+  bool ActOnDuplicateDefinition(Scope *S, Decl *Prev, SkipBodyInfo &SkipBody);
 
   typedef void *SkippedDefinitionContext;
 
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 31984453487aef..750301a9155d79 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -5645,7 +5645,7 @@ void Parser::ParseEnumSpecifier(SourceLocation StartLoc, 
DeclSpec &DS,
 Decl *D = SkipBody.CheckSameAsPrevious ? SkipBody.New : TagDecl;
 ParseEnumBody(StartLoc, D);
 if (SkipBody.CheckSameAsPrevious &&
-!Actions.ActOnDuplicateDefinition(TagDecl, SkipBody)) {
+!Actions.ActOnDuplicateDefinition(getCurScope(), TagDecl, SkipBody)) {
   DS.SetT

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-21 Thread Michael Jabbour via cfe-commits

michael-jabbour-sonarsource wrote:

@vsapsai Thank you very much for the input. :pray: 

> Can you trigger the crash without -ast-dump-all?

> Can you use in the test %clang_cc1 instead of %clang?

I addressed both of these in f6f0887d22172c9db4f602b7f272630a97a708bd. I added 
a separate invocation without `-ast-dump-all` that reproduces the crash prior 
to this PR. I also switched to `%clang_cc1` instead of `%clang`.

For convenience, you can also find the crashing example here on CE: 
[link](https://godbolt.org/#z:OYLghAFBqd5QCxAFwE4FN0BoCWIIDGAtgIYDW6AgqsAM4gDkAtACIDCAspQNICiA%2BgCEAqgEkAMi34AVAJoAFXgFIAzCxboARgFdgDLAQD2RAA44ANulTiSAO2DaSwdKIAmIAkoBMg74IPatMjGAPLayCbhAGIW6LYkROgo6EFYAGax9LDorjjBqG4gXumxhSollvGJILQIJBiuAHQIWKAM%2Bji0orYE5tquOYwADB20HCQ4tgDKhtqoBEkMAIyjISZxw/rmdsCF7QCUWBA5eYYF7uUZloUALBVxCUnoAB4JJpaNBCYmrYyj3b1%2BoMGCNcGMJtNZvNFqDOmsNiCtjs9gxDsdcvlCkt7oUAGz3KpJIiGVzaD7E1ykH5tf49PoDdyIsHjSYzOYLRgrMHw2ybLDbeyFTw%2BPxok6Y9x3K4udwAdgJjxAHBJlBiliWzV%2B7TBAPpwNh4NZUI5y1W615TIFu3cwt8PjFGLOhQArDj3AAOBXVZWuVWxLyamk6ulAxkGlmQ9mLLlw818q1Cvyio7ip3ufHSwoATi9SR9gjV6EDf2DgIZfM6EbZ0M5ZoRoITNqT9sOnU45HQ8lQhgAVugCMha1geyBsbZ0AB3QtYoaHcyMZ36C2gwyMaQYdAAam8Ki5g8RaKQRgGvEdqFw%2BGlhJqxqLCG%2B6UMtmQUwIJEso6lT%2BQwloneefIjsUVoeM2/gmHY6AAHKKrU9Q5Jqf6WAOOBPvgcSuGwhh9EQvLYhh4iTNB2hEJoVggK6JiGLQeSobYWE4XhWBUTRyB0YR45QSRZGoBRWBIf2bFPlMyD1MgDEkUxAkocJomoMgHHEaR5GukEYkSbho78XJClEVxym8c6hy0LejAAPRmZu0gIFu47PMgm7IBOhibi8byWLQjmoDgwDOKgnnIDZm4EKgJC1JuGTPDkm6TI5CCdJu8gAEpKEMlDIAAnusAxpK5tgkZuHAZbw%2BVEEs26yoIhUZQAau%2B/CUBVLDVSVJFLKovhpXEBVFa1RBeBVVVFXV5hCE1HWpelWU5OguXdUQg3VSN/BsE1LWlSoE1daVi3DfVzVKLKLBbZQqL8guS58qucENJq%2B4jIeHgkugp6nOeeAQFeiqkJMnwPmk36vu%2BSRLF%2Bz6/v%2BgEgOUIG2n4zGQTB1RuaYHxfD80lCbyxy2Jh2GSVpBF6dx5F3CxtFPhpTHk1jin6TxIB3JjdEiep%2BOadizOyWJdMk7xTM6VTWlqfJvMGYzxmmQwO6TGWW7eF4%2BaFs0CuTZNkwOT9tgQPsi0YMgcy2Et9WrX4xujcdKhVYdx1pWd84MIuWDLvoq4o%2B8Rbo1g91nRAR7Pa9%2BQXp9sTXqaj7PkDH6gxHP5/vIAFMiO2Kw2BCOcd9JJkkWFJUvx6DIVj6G40L%2BG42LDNLPiNN0aXzHURTtgV%2BRVf54XLOC%2BzUkF4JHc88T4utyL4ld8LOnN7xVeS1GjAUtnhUqoNk2bpuc%2BWAvvqFuVh2dY1K%2BbjZJADKg25eIrKpbyrZ/L/vLxUfJm4AFQ3zbN9r1uPp%2BpYA07zfK%2BH8fU%2B59N7%2Bivl4P%2Brlnj3wcs/NKK9X5pQQZQSa78N7W0qjfABVggFK1iGAm%2Bd8zgwLVkdSa9sLrOyuiAd%2BjRc4kB%2BD7R6x4XpnmDl9ao4cAaRzfNHMGcdIZJ0ZvyHYoERQ%2BHTtBRUn9L4tC5tjDCdciacT5kUeurFa6j2xDXJ8E9VFyNZvJOu%2Bjx4DwZsUYeRiTHKPFl4aeNZpZeBULLPUQCboIQQKrO2c4KEuywKuaRsQNQtEYUcf2J5WEfXYdGfQXCXw8JBnwiGCcoaulTmI8CiMpEX1AbInuMl5El00VgJRSkzFqMbnXbRTdTHkXMXkrGBiR6MWFvUvuosam8XMZ3ZpnMrGlNqXYk0FlNy8FMJlCKsQyHeMdpdJk/jsnfzupsJhAcImXlDoqThgN4mfljkkxOoIRz4jSXaDJGdvQkgLHg3J7c0I4zxj04p5cOl8SqZUhutMXmqVadzQxRTjH92sQzVS3SCa9MBf0wygzFjDNwevAY5pXCeSfBvL%2B6AAwIE3E%2BcwGVGhq0cc4oEOCFnovwWlYZup%2BiTGAJuNxTRMU2QwMFEggQUibimOgUgjkXKJBoFuQKW5KBTGkJuWwz1PJpG7AtOlzRNwTjyJigVB8cCuAGEbJ8bLJXGFRXixBBKQwDFcXUW6Hjr5ePOjMyhcylSXOVsE5ZhwsG8R1lgMgFEhgWpuPoIgo4hget8aubU9BsQhLgLAGAiAnqmFiO9fARho2WF4r0HY/Awp/nknRWg/A0DaFsGQCoyArD0AgJoPkmhJj1AykOct8RUAZRCJoPsA4hzxsSM%2BEItgcV8gGDoYAbB3zmHoOHAYpB7AfiZLkDAKEABuKQ%2BQvH7OEaJuBnwF3jDgTQoU61YQGHyNAOAfXh1nagTQ1F0AaFHcAcw1LlnpG2HQGqOBJw8iHLEqOCS9nxwOfoZOwj7CiNOfyDdWQXWGBMFjIdTBnhYsbb3WdTACCbiYGkd%2BnlkPvyYFSZDky1A0LoSYJDaRaAZWfCQZ4TBsUZVdse7yAwQPGR%2BQUh5YKnmuF0Voj5GjHlVPY23XuvymksYBe0oFLdtJs0ecPdjhw0CYFHNMp2AbGCbmeO6XETBcQ3GClaTcEA01WAg15XNZBdYQHXJgU%2Bu59j6EYY69AR9yIuodl6rAPqVDukaI42UuJZRLFlGp507ohg3BUK6JTQaQAetDRGv2IBZgRHCMHeNZhE1I0WKp9TmntM7E3EsLMjQNSgjfTsmOsT9lQxDRuFVWkJyhRMLWBTsyVyMDCAlhyhhcoZY01p5N9hdP6YzU%2BAKqBjOmawgm7BO4ljWe9g6z13qQC4g1N5xxXh3ReCWCoG4eXnT4nC/oegUW5tuudB6h2KgmuuxLLZi1XhLt%2BOu3N49NE0I3CAA%3D%3D).

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-21 Thread Michael Jabbour via cfe-commits



@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);

michael-jabbour-sonarsource wrote:

Thanks for raising the concern. I am also not entirely sure, but I chose to 
remove it for two reasons:

 1. It seems to me that the presence of this `removeDecl()` call causes an 
incorrect ODR violation diagnostic when `-ast-dump-all` is provided (which I am 
using in my regression test to expect the AST nodes after merging):

```
In module 'ModB' imported from :1:
In module 'ModA' imported from ./ModBFile.h:2:
./shared.h:6:9: error: 'MyEnum3' has different definitions in different 
modules; definition in module 
'ModA.ModAFile1' first difference is enum with 1 element
6 | typedef enum { MyVal_C } MyEnum3;
  | ^~~~
./shared.h:6:9: note: but in 'ModB' found enum with 0 elements
6 | typedef enum { MyVal_C } MyEnum3;
  | ^~~~
1 error generated.
```

This incorrect ODR violation can be observed before this patch. CE link: 
[here](https://godbolt.org/#z:OYLghAFBqd5QCxAFwE4FN0BoCWIIDGAtgIYDW6AgqsAM4gDkAtACIDCAspQNICiA%2BgCEAqgEkAMi34AVAJoAFXgFIAzCxboARgFdgDLAQD2RAA44ANulTiSAO2DaSwdKIAmIAkoBMg74IPatMjGAPLayCbhAGIW6LYkROgo6EFYAGax9LDorjjBqG4gXumxhSollvGJILQIJBiuAHQIWKAM%2Bji0orYE5tquOYwADB20HCQ4tgDKhtqoBEkMAIyjISZxw/rmdsCF7QCUWBA5eYYF7uUZloUALBVxCUnoAB4JJpaNBCYmrYyj3b1%2BoMGCNcGMJtNZvNFqDOmsNiCtjs9gxDsdcvlCkt7oUAGz3KpJIiGVzaD7E1ykH5tf49PoDdyIsHjSYzOYLRgrMHw2ybLDbeyFTw%2BPxok6Y9x3K4udwAdgJjxAHBJlBiliWzV%2B7TBAPpwNh4NZUI5y1W615TIFu3cwt8PjFGLOhQArDj3AAOBXVZWuVWxLyamk6ulAxkGlmQ9mLLlw818q1Cvyio7ip3ufHSwoATi9SR9gjV6EDf2DgIZfM6EbZ0M5ZoRoITNqT9sOnU45HQ8lQhgAVugCMha1geyBsbZ0AB3QtYoaHcyMZ36C2gwyMaQYdAAam8Ki5g8RaKQRgGvEdqFw%2BGlhJqxqLCG%2B6UMtmQUwIJEso6lT%2BQwloneefIjsUVoeM2/gmHY6AAHKKrU9Q5Jqf6WAOOBPvgcSuGwhh9EQvIqHcGHiJM0HaEQmhWCAromIYtB5KhthYTheF3NRtHIPRRHjlBpHkaglFYEh/bsU%2BUzIPUyCMaRzECegyHCdMYmoMgnEkWRFGukE4mSbhID4QJinKcR3FqXxzqHLQt6MAA9FZm7CD0xiJM%2BkzAJuyAIFu47PMgbkToYm4vG8li0G5qA4MAzioCF7lbgQqAkLUm4ZM8OSbpMbkIJ0m7yAASkoQyUDZbkAJ7rAMaQBbYpGbhwxW8FVRBLNusqCDVxUAGrvvwlDNSwbX1aRSyqL4BVFXE1W1QNRBeM1rW1Z15hCL1w35ZQq3IKVOToBV41ELNbULfwbC9f1DUqCtBWrbt%2B3zV1fVKLKLAXZQqL8guS58qucENJq%2B4jIeHgkugp6nOeeAQFeiqkJMnwPmk36vu%2BSRLF%2Bz6/v%2BgG6fyOygSKPhYBBXGKoFpgfF8PyCShaHHLYmHYVJo5YIRRk8RRLE0XRT7aby2KsZztgqcZvEgHclPyaJWn0zp2Ji/REtKYLrN8aLBnc4zmkKyzJki%2BZlkMDukxllu3hePmhbNCbq2rZMPnQ7YED7PtGDIHMtgHV1x1%2BO7i1PSorUPU9BWvfODCLlgy76KuJPvEW5NYH9r0QEeQMg/kF4Q7E16mo%2Bz6Ix%2BKM5z%2Bf7yABTIjtiIG2n4BOQTB1QUmSRYUlSMlyfR6G02r2LM1xSujvifPyV3BMc/Jiva0s%2BKyyJqtSzzrdCXLBnj8Lk/6ZLTHq8vWur7iutRowDeWDVKqzatm6bkfW4%2Bn66pnwVF8Xx5JADKg25eKbKqFhqCCWw/j8vGokpTcAAqc%2By1/5XxPr6QsM0HojR6o/Z%2Br936fxgf6C2H9wEX0AWcHyYD/4Byto9K2BUoH5nvog5BVhUFm1iJgrw4DcHAIIWtEhQc5zvXDp9EAV9GjNxID8BOANjzAzPOnSG1Rs7w1zm%2BfOqMi4YzLiLbG9hcZ2nArXRUN9v6IVkovamGFh491UsLYog96LDwsU%2BFeFFijTwUhvBmMt9FU0cZrXu2t7Gz03jLbenizH7xrPrLwKhDZ6lQd9BCv8sEcLeqHD6TJVw6NiD/eOmwREp3EeDSR0Z9AyJfHI5GCj0Yl0xq6SuYEa5E29F/DBLQHEdzpr4pmtNbF8XMaPSxc9GbWIFjvOxC83Hywkj0lxbcZ7iXaUUdeSlh4a0MgEuxQSTRFV4KYDaSVYirWDlwiOWBkl1MsAGFowijjJxPNky8mdFTSIRkUz8hdSml1BCOfElS8aaJqXmEkBZ6ENNcfJJpxi2kDNMiPNi3SWl9OmRpQFS8nHSyGeLfxpj1KzNGS0hZsKVmLCKnQ4%2BAxzSuBCk%2BaBt90AnM3E%2BcwxVGhW1CeEoEtCjmUoYatIqup%2BguU3FEpoCBNweQwJuN8gQUibimOgUgbl/KJBoFuGKm5KBTGkJuWwQMQppG7HtPlzRNwTjyAKxVmVXADDdk%2BcVWrjDkvpZdRlIYBiRLqD9GJjC4khzDvsw5rg/kfFORkw41C%2BIOywGQSiQx4k3H0EQUcQwI1epLPQbEZy4CwBgIgQGphYhg3wEYLNlg%2BK9B2PwBKf4lL0VoPwNA2hbBkAqMgKw9AICaD5JoSY9RipDjbfEVAxUQiaD7AOIceanLIBCLYWlfIBg6GAGwd85h6DZwGKQewH4mS5AwChAAbikPkLx%2BzhDybgZ8sl4w4E0PFXtWEBh8jQDgGN2cd2oE0DRdAGgV3AHMC5DJ6Rth0HajgScPIhwFLzsUp5xcXn6HLqo0AVd8Zfs0FkENhgTDyUXUwZ41KB2Lx3UwAgm4mBpCviFIjV8mBUiI9stQfCBEmEI2kWgxVnwkGeEwGlxVCMAA0i32EIwlZATBSSmCYPOyOT6woDGQ%2BZeFhjO5jNaa4aZvMulcwUzCsF6tZPuMxc45FCKPFor4n4xF89sWaaWIcNAmBRycISdwpJjBNzPHdLiJguIbgiqtJuCAparDodCjWsgjsIDrkwO/Xc%2Bx9DCMDegF%2BFEQ0hyjVgGNKh3SNFCbKXEsoliylc86d0QwbgqFdAm7U9AI0pvTUnEAswIjhHTnmswBa66LBc25jzXmdibiWFmRoGpQSgYeQXApzzMbJo3Dgdw2IJzxRMLWOznqeEMDCPVnyhgKrtfc553jrlfO0DLQF6ttaQtYXzTQnclnosBsjdGkAuINRZdCV4d0Xglj4V686fEZX9AVfSQeQ4YbnQRpDioRJK4SwxfiV4cHkdIc3afbRNCNwgA).

2. I am also not entirely sure if performing the call in the named enum case is 
safe. I can understand why it is important to remove the constant from 
IdResolver and the Enum context, but I don't know if removing the constant from 
the enum decl doesn't cause problems

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-21 Thread Michael Jabbour via cfe-commits



@@ -4034,6 +4034,12 @@ class Sema final : public SemaBase {
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 LookupResult &OldDecls);
 
+  /// RetireNodesFromMergedDecl - We have just merged the decl 'New' by making
+  /// another definition visible.
+  /// This method performs any necessary cleanup on the parser state to discard
+  /// child nodes from newly parsed decl we are retiring.
+  void RetireNodesFromMergedDecl(Scope *S, Decl *New);

michael-jabbour-sonarsource wrote:

Thanks for the suggestion, done in f55ca2bdf0e0fabdc672f0f85007d091272a8891.

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-21 Thread Michael Jabbour via cfe-commits



@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);

michael-jabbour-sonarsource wrote:

I can try to dig deeper to answer the points I mentioned (especially point 2) 
if desired, but I would need to allocate some time (maybe at the end of the 
week).

Any pointers/hints/thoughts would be appreciated.

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-21 Thread Michael Jabbour via cfe-commits



@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);

michael-jabbour-sonarsource wrote:

Additionally, as far as I can see, for `EnumDeclConstant`s created by the 
parser, the DeclContext is always the containing `Enum` itself:

https://github.com/llvm/llvm-project/blob/79231a86846b7dff09497fc58ea1e82e892052bd/clang/lib/Sema/SemaDecl.cpp#L19739-L19740

Therefore, after the refactoring I did, I think that the call can be simplified 
to:

```
ED->removeDecl(ECD);
```


https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-26 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource updated 
https://github.com/llvm/llvm-project/pull/114240

>From cc3cf25da67c9f8b9edabb318c6011cad9bd2f58 Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 11:16:09 +0100
Subject: [PATCH 1/8] [NFC] Factor out RetireNodesFromMergedDecl

---
 clang/include/clang/Sema/Sema.h |  6 ++
 clang/lib/Sema/SemaDecl.cpp | 27 +++
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 93d98e1cbb9c81..70bbb882eb2b13 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4034,6 +4034,12 @@ class Sema final : public SemaBase {
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 LookupResult &OldDecls);
 
+  /// RetireNodesFromMergedDecl - We have just merged the decl 'New' by making
+  /// another definition visible.
+  /// This method performs any necessary cleanup on the parser state to discard
+  /// child nodes from newly parsed decl we are retiring.
+  void RetireNodesFromMergedDecl(Scope *S, Decl *New);
+
   /// MergeFunctionDecl - We just parsed a function 'New' from
   /// declarator D which has the same name and scope as a previous
   /// declaration 'Old'.  Figure out how to resolve this situation,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index f8e5f3c6d309d6..54c4c5f4e5395e 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);
-}
-  }
+  RetireNodesFromMergedDecl(S, NewTag);
 }
   }
 
@@ -2639,6 +2628,20 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   notePreviousDefinition(Old, New->getLocation());
 }
 
+void Sema::RetireNodesFromMergedDecl(Scope *S, Decl *New) {
+  // If this was an unscoped enumeration, yank all of its enumerators
+  // out of the scope.
+  if (auto *ED = dyn_cast(New); ED) {
+Scope *EnumScope = getNonFieldDeclScope(S);
+for (auto *ECD : ED->enumerators()) {
+  assert(EnumScope->isDeclScope(ECD));
+  EnumScope->RemoveDecl(ECD);
+  IdResolver.RemoveDecl(ECD);
+  ECD->getLexicalDeclContext()->removeDecl(ECD);
+}
+  }
+}
+
 /// DeclhasAttr - returns true if decl Declaration already has the target
 /// attribute.
 static bool DeclHasAttr(const Decl *D, const Attr *A) {

>From f07904ae5468658a3e7e4a649cd183734479d6cc Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 12:07:29 +0100
Subject: [PATCH 2/8] Drop enumerators when merging named enums

This fixes a crash in ASTWriter when the stale IdResolver entries are
used when writing the identifier table.

  assert(DeclIDs.contains(D) && "Declaration not emitted!");
---
 clang/include/clang/Sema/Sema.h |  2 +-
 clang/lib/Parse/ParseDecl.cpp   |  2 +-
 clang/lib/Parse/ParseDeclCXX.cpp|  3 +-
 clang/lib/Sema/SemaDecl.cpp |  6 ++-
 clang/test/Modules/modules-merge-enum.m | 52 +
 5 files changed, 60 insertions(+), 5 deletions(-)
 create mode 100644 clang/test/Modules/modules-merge-enum.m

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 70bbb882eb2b13..d53bcf63dd1bd9 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3910,7 +3910,7 @@ class Sema final : public SemaBase {
   /// Perform ODR-like check for C/ObjC when merging tag types from modules.
   /// Differently from C++, actually parse the body and reject / error out
   /// in case of a structural mismatch.
-  bool ActOnDuplicateDefinition(Decl *Prev, SkipBodyInfo &SkipBody);
+  bool ActOnDuplicateDefinition(Scope *S, Decl *Prev, SkipBodyInfo &SkipBody);
 
   typedef void *SkippedDefinitionContext;
 
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 31984453487aef..750301a9155d79 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -5645,7 +5645,7 @@ void Parser::ParseEnumSpecifier(SourceLocation StartLoc, 
DeclSpec &DS,
 Decl *D = SkipBody.CheckSameAsPrevious ? SkipBody.New : TagDecl;
 ParseEnumBody(StartLoc, D);
 if (SkipBody.CheckSameAsPrevious &&
-!Actions.ActOnDuplicateDefinition(TagDecl, SkipBody)) {
+!Actions.ActOnDuplicateDefinition(getCurScope(), TagDecl, SkipBody)) {
   DS.SetT

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-26 Thread Michael Jabbour via cfe-commits

michael-jabbour-sonarsource wrote:

@vsapsai Thanks again for the feedback :pray: 

> I've realized that the test doesn't have to be Objective-C, the failure is 
> reproducible in C. Curiously, we aren't hitting the assertion in 
> C++/Objective-C++ but I don't know why.

I dug a bit into this today, and I could find a few differences:

1. ODR checks seem to be implemented on C and ObjC only (see 
df0ee34bc2523f11b907311670c776cb3de108c1). As far as I understand, in the C++ 
case, `Sema::ActOnTag` sets [`SkipBody.ShouldSkip` to 
true](https://github.com/llvm/llvm-project/blob/cd708029e0b2869e80abe31ddb175f7c35361f90/clang/lib/Sema/SemaDecl.cpp#L17616),
 causing `ParseEnumSpecifier` to [skip the body and return 
early](https://github.com/llvm/llvm-project/blob/cd708029e0b2869e80abe31ddb175f7c35361f90/clang/lib/Parse/ParseDecl.cpp#L5553-L5561)
 before it would otherwise [call 
`ParseEnumBody`](https://github.com/llvm/llvm-project/blob/cd708029e0b2869e80abe31ddb175f7c35361f90/clang/lib/Parse/ParseDecl.cpp#L5603)
 and create the `EnumDeclConstant` AST node (that we need to clean up in the 
C/Objective-C case).

2. Another difference, which doesn't seem to contribute directly to the 
divergence in this specific case, is that in C++, `ASTReader` pre-loads a set 
of interesting identifiers, see 33e0f7ef94124a53404555879666cef0e370f8c2.

> I wanted to try defining the enums in different scopes (e.g. in a struct) and 
> see how it works. Consequences for scopes aren't clear, so that's worth 
> taking another look.

I added one more test case for this. I got the same stack trace, and the crash 
seems to be fixed after the PR as well. However, I couldn’t reproduce the stack 
trace with `WriteCUDAPragmas`, so I might be missing something. See the scoped 
example [on 
CE](https://godbolt.org/#z:OYLghAFBqd5QCxAFwE4FN0BoCWIIDGAtgIYDW6AgqsAM4gDkAtACIDCAspQNICiA%2BgCEAqgEkAMi34AVAJoAFXgFIAzCxboARgFdgDLAQD2RAA44ANulTiSAO2DaSwdKIAmIAkoBMg74IPatMjGAPLayCbhAGIW6LYkROgo6EFYAGax9LDorjjBqG4gXumxhSollvGJILQIJBiuAHQIWKAM%2Bji0orYE5tquOYwADB20HCQ4tgDKhtqoBEkMAIyjISZxw/rmdsCF7QCUWBA5eYYF7uUZloUALBVxCUnoAB4JJpaNBCYmrYyj3b1%2BoMGCNcGMJtNZvNFqDOmsNiCtjs9gxDsdcvlCkt7oUAGz3KpJIiGVzaD7E1ykH5tf49PoDdyIsHjSYzOYLRgrMHw2ybLDbeyFTw%2BPxok6Y9x3K4udwAdgJjxAHBJlBiliWzV%2B7TBAPpwNh4NZUI5y1W615TIFu3cwt8PjFGLOhQArDj3AAOBXVZWuVWxLyamk6ulAxkGlmQ9mLLlw818q1Cvyio7ip3ufHSwoATi9SR9gjV6EDf2DgIZfM6EbZ0M5ZoRoITNqT9sOnU45HQ8lQhgAVugCMha1geyBsbZ0AB3QtYoaHcyMZ36C2gwyMaQYdAAam8Ki5g8RaKQRgGvEdqFw%2BGlhJqxqLCG%2B6UMtmQUwIJEso6lT%2BQwloneefIjsUVoeM2/gmHY6AAHKKrU9Q5Jqf6WAOOBPvgcSuGwhh9EQvIqHcGHiJM0HaEQmhWEUWAmIYtB5KhthYTheF3NRtHIPRRHjlBpHkaglFIf27FPlMyD1MgjGkcxWACShwmiagyCcSRZEUcUQRiRJuEgPh0nyYpxHcSpfFeIctC3owQSoNoA6bhwACeIlWTZSiyr4QyUJum5xKRtl2bwtikc626ub5ABq778CwwVRTE6DmK4qhuZQLksIlqL8guS58qucENJq%2B4jIeHgkugp6nOeeAQFeiqkJMnwPmk36vu%2BSRLF%2Bz6/v%2BgHafyOygSKPhUZBMHVC8bwfF8PwyUJvLHLYmHYZJo5YIRBk8RRLE0XRT6aby2KsdtthKYZvEgHc030SJGmLVp2IXXJYnHetfHnXpu3LepClPUZZ2meZDA7pMZZbt4Xj5oWzSg0o7nQ5QkzIJutW2BA%2BzBYIm4YMgcy2GFEWpSo6MpbD6Xzgwi5YMu%2BirmNpgTQ%2BBXpRAR4lWV%2BQXlVsTXqaj7Ps1H5tTzP5/vIAFMiO2IgbafhDVxNUkmSRYUlS0lxYJ9HofN73YqtXHPaO%2BIHTNWtUVtM3fadSz4vd0xvTde0q8hM1XV9a0/ZbunXUxH16ebFGW39UaMBSCu2SqaOw55weWKHvqFks4fuZ5nkIOgJADKg25eGDKpx5DWcR0nLzUQpm4AFQF0TieI/L0c%2Bn6lheAnHlJynadWJn2ex/6edeAXnlF2cCPl1XlfJbKqUw%2B5UdbvmTfJ6n6cd%2BDsQ9wXA8l8PY8T5QJOZRT2UgNPjRKyQPwM0Vx6lWe7PVdU3ONbzb78%2B1QtdWLZ29fY/V2uBw2KnXucWjWw1gtL22t5q%2B2MibNi9FjaGw4q7U6alVayRtp7Jad0UFOx9og1SHsFLG0%2BvpXWP0TLSX%2BoDEMAwO65QQggKG7ld5kyykyVcADYgahaOfI4zMTzX0qrfaM%2BgH4vifq1F%2BnURbdVdJLMCMtoL/xzt3IBWD1ZzVARglaEDcFQPgTtO2y09FHR0fxVRD0CEGMwY7S6OCSFIPweJSxDjIFFADjWBgAB6Dxm5eCmGQHZTc0piZzj3pTLAbClEN3ypsC%2BLN%2BGXk5oqe%2BTUxGfkFpI0WoIRz4lkQNX%2BstvQkgLCvFR1i0LqONjrZSp1XRGLgabBBdiKKumts7RxYCHZq3McQ6pzSHGENsb0vizo3Emi8THYp0cBjmlcLQTcT4Y713QAGBA8zbDmDso0WGlDgZL0ics1e7lxm6n6JMYAm5aFNFWSnDAm43yBBSJuKY6BSCbmCIjKwzg3kp03JQKY0hNy2BKnMtI3YiAXLqHlVZE48irOQD8hAOBXADBxk%2BR5oLjCLK2TDLwKggZ6hoZCuhDCd4hOYfvVhSoikQy4TEw4rd074EOGQEAzohgZQYDcfQRBRxDHZWE1c2p6DYm4XAWAMBEDFVMLECq%2BAjDSssHxXoOx%2BAkFoH%2BBS9FaD8DQNoWwZAKjICsPQCAmg%2BSaEmPUOyQ4LXxFQHZEImg%2BwDiHPKxIz4QjrOtUyAYOhgBsHfOYeg3MBikHsB%2BH1OAMAoQAG4pD5C8fs4QhG4GfHFeMOBNCoCtVhAYfI0A4B5dzONqBNA0XQBoMNwBzBnJiekbYdBQo4EnDyIcIi%2BbiPScLTJ%2Bhxaf1AFLQaNbNBZBRuEkwM1g1MGeHczcTA0jTzmfO6eTAqTztiKoFgR8T4mDnWkWgdlnwkGeEwJ8GyqYltQEi%2BNkBTJmNmhhd6NxXRVJOhRcodSDHPugYdFx5RWm2y9t%2BgDj0TH/sA0tYDgy318RUIcNAmBRxkvJgKxgm5njulxEwXENw7lWk3BANVGrJ1vKsvq1GEB1yYEzrufY%2Bhz70oXhRMdpMuVYB5Sod0jRcWylxLKJYspMPOndEMG4KhXSoaFSAdloqJVMxALMCI4R2byrMIqkaiwMNYZw3hnYm4lhZkaBqUE7bUkCxERk7qIqNxIuWhObNJhazIZYSuRgYQlMI0MGkdDmHsO4eVfYAjRGrAkd1eRgjWEFXtx3EsOjWAGMcrYzy3EGpeO4q8O6LwSx8IGedPiST%2Bh6AybpVgFlbKOUqBc1TEsCXSZeCq%2BEmrJWS20TQjcIAA%3D).

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-27 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-26 Thread Michael Jabbour via cfe-commits



@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);

michael-jabbour-sonarsource wrote:

I had a quick look into this today, and I could see that some other ODR checks 
aren't emitted unless `-ast-dump-all` is provided. For example, in [this CE 
example](https://godbolt.org/#z:OYLghAFBqd5TKALEAXATgU0wGgJYgQDGAtgIYDWmAgusAM4gDkAtACIDCAstQNICiAfQBCAVQCSAGTaCAKgE0ACvwCkAZjZtMAIwCuwJjiIB7EgAc8AG0zpJZAHbBdZYJnEATEERUAmYb%2BEjXXpUUwB5XVQzSIAxK0x7MhJMNEwQnAAzeMZYTHc8UPQPEB9M%2BOK1MutE5JB6JDIsdwA6JBxQJkM8enF7IktddzzmAAYu%2Bi4yPHsAZWNddCIUpgBGcbCzBNHDSwdgYs6AShwIPILjIs9KrOtigBYqhKSUzAAPJLNrZqIzM3bmca9fqDYZMMb4CZTWbzRbLcHdDZbME7PYHJjHU75QrFFaPYoANkeNRSJGM7l0X1J7nIfw6gL6AyGnmREMm0zmCyWzDWEMR9m2OF2jmK3j8AQxZ2xngeNzcngA7ETniAuGTqHFrCtWv9OhCgYzQfDIeyYVzVutNvyWUL9p5Rf4/BKsRdigBWPGeAAcStqqvc6viPm1dL1DJBzKNbOhnOWPIRloFNpFAXFJ0lLs8hNlxQAnD6Un7hBrMMGAaHgUyBd0oxzYdyLUjwUm7SnHcdutxKJhFOhjAArTBEVD1nB9kC4%2ByYADuxZxI2OlmYrsMVvBxmYsiwmAA1L41Dzh8iMSgTEN%2BM70PhCLLiSOMsZ7KgZkQyNZxzKH6hRPRu68BWP3RtLxW0CMwHEwAA5ZU/QDawgzaH9rCHPAH0IBJ3A4YwBhIflcXQyRpkg3QSG0GwShwMxjHoAoUPsTDsNwiiqJoh8CMnCDiNI9ByMQwdUFomZUEaVB6OIxjeOQh9BOEtiiJIsjShCYTRJw8ccCU9BUFkjj5O4nxjnoU1lgAemM7d%2BHMVAAE9t1lFQRmodFBSXFcBXXGDi3gnBDzGY8vDJTBz3OS8CAgG9lUMmNWl%2BTJP2fV8UhWD9H2/X9/3IoD7QCCjwKg2p6kaPJtQk/jUNOewMKwsS1PwwidK48jKOo0q6Kq1TcSalj7G0ziFPUzAkJa6TNJU8SBr4gShM0nrdJ4qaRLa8T5pmhr9PUozmASYjty4Kz%2BHsYifF3eVhB2qyADVXxEY62HUfwHKcxcmGXHBV0MdcCqabUfKciATwCoLCivML4lvchpm%2BGL70feK3yS2KUp/RQ/xZMdKkykCcvY5U3g%2BL4fj%2BEraLQirRpKUpavY3ruMqTqWrJnxSjp2iVrIyoiak%2BaGcU8bJNmZa6upkB2a5xbyfUgWqdmtQDI2pg92mCsd18HxC2LVoVfs6gtemVBt3B%2BwIEOY7TqwVAFnsbcODum6tcelzXrckBcfMfGYp%2BvzT0Ci9gfC2pzQRp8Xzh5KvyRlHwTHXEMbFPwscg5UqQpEsqRpfrBuJ8rKoYmqKtZ7iVkJZmHzJjrmJa/Px0JDn%2BeUsXcRr4atMF2bC4luuc4byW5IawvZZjZgk%2BsHa1RNrXt31slk5H/1ixWMeHIniekEwMghnQXdGY8%2BItSQTXF6Xt5KM07cACpx9tg%2Bh53be4IX6gl%2B3Fe15sTfVbVTyNcZi%2BJ6Pi49fPgfFQ8pboOWAaA7WDlr4z1OsA%2B6D8n6r3Xm/NW8Qv4%2BAvn/E%2BgDtYgLtguB2b0cDrmvs0VOZA/gexOP9M8PtQp%2B1jIYaGQcErvkDqlZG6UHgxwdKBXK0EP472KrzFqJNs7VTwnnFuDVabl1oqXJizUWbSLZunCanMO4SLUXzJulcRaaPau3aaKiab9zrPLBy1lNhDAyNuLaJATZWxuuZO6%2BDnLPVciydygjNTfW2IcY4z916EGOBQEAroRjuLuIYEg4TIlEPXLqRg7oqFwFgDQ72wVfag2VAHZhsNEqhw4RHQwY5CQ8OymBbGvoyRFjQQhERmd0IKMpj3MiRc5El3roorqldq6NI0SNbpjdu71XaUYhancJl9LMWaUyMDizbiGJadw9BtwPhnrBTA8F1n2EsFZZoWsFZhiGCgnx2z0FHLAZYqy1jMC2PsY4jgOBtxsGcaoNQ8D7YeMdl4lUtT1ZtA9oEpBZEjZRJiXE35a4yyMEicCnAYSInuLUJ4mFupUmIDScgfy5h4ghUICYPF1huL9D2IIMg9AfyaVovQQQGBdD2AoFUVANhGAQG0AKbQ0xGhWRHNyxI6ArJhG0AOIcI4iXJEfGEPZfKWRDD0MADgr5LCMADkMcgjg3zyrwFgZCAA3NIAo3iDkiIw/Aj4BqJjwNodAvLMJDAFBgPAsSA6GvQNoKimAtCauAJYaYHRfKZF2Awc6eBpx8jvHFYOhT2Hh3StHPYwFY6BH9doHI4LjBmBamqlgrxtxEG3CwDI181nFuviwGkxb4jqDYKQ8hZgi0ZHoFZR8ZBXgsAfPsotAANMljgi2UtQCwck5gWAqveu69AeAhgZoMgM/kWcWlSKlr3Hp9NunF26iYtSIyDFjQzoM5uq6yJd33bu0ZQsVjHAwNgccBCfkJOYNuV4np8QsHxHcAtNptwQEpdSnN24GVMuNhATc2BN77kOIYBFQSwUPuiTgWJrotTynxJ6NQOYVg%2BHxFhnMOZ8TyndE%2BpJIB4X%2BJOFilA8woiRGBkSiwJK8rLFfe%2Bz9369jbhWDmZoWpwT5JjWw5hxSE3eS3DOtSU47VmHrA%2Bl6JGIi0b1sYWxrGP1fv7cAX9/6bCAeAxQUDmFiWvz3NemD/iEOQruJ6ZoPh9z4hGJ6eUahHMjBGGoHwxGnakfI0eUJUKnqouhe9MsCKno%2BDRSFjFFmcDuuoqhO4QA),
 I have the enum `E` defined with different enumerators, and I don't get a 
diagnostic unless I have `-Xclang -ast-dump-all` on the command line (removing 
it, somehow removes the diagnostic).

While I still don't have a concrete answer for what I mentioned above, it seems 
to me that this is a separate bug in the ODR check implementation, and that 
ideally, we should get the diagnostic without the `-ast-dump-all`.

If my conclusion above is correct, I would argue that dropping this call is 
necessary for these ODR checks to work (once the bug above is fixed).

I may try to dig deeper later into why the diagnostics are not emitted without 
`-ast-dump-all`. I am curious about your thoughts here.

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-26 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-26 Thread Michael Jabbour via cfe-commits

michael-jabbour-sonarsource wrote:

@vsapsai Thank you for sharing the previous patch. This is really interesting.

> For the record, the previous work I've abandoned is 
> https://reviews.llvm.org/D114833 Doesn't seem particularly relevant to this 
> change to be honest.

As far as I can see, it actually seems to introduce the exact same 
`IdResolver.RemoveDecl(ECD)` call in `Sema::ActOnDuplicateDefinition` that I am 
introducing in this PR, and this seems to be sufficient to fix the crash here 
as well.

I tried to understand why that work was abandoned, in order to know if the 
reasoning applies to the changes in my PR.

- Regarding the "ambiguous use of internal linkage declaration" warning in the 
C++ test ([this comment](https://reviews.llvm.org/D114833#3165889)), I could 
see that the currently the warning is emitted before and after my PR on that 
test, so I see no risk here.
- About handling anonymous enums ([this 
comment](https://reviews.llvm.org/D114833#3190487)), do you think any further 
action needs to be taken? I have a test case to show how their AST dump looks 
like...

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-21 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-21 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-01-21 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource updated 
https://github.com/llvm/llvm-project/pull/114240

>From cc3cf25da67c9f8b9edabb318c6011cad9bd2f58 Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 11:16:09 +0100
Subject: [PATCH 1/7] [NFC] Factor out RetireNodesFromMergedDecl

---
 clang/include/clang/Sema/Sema.h |  6 ++
 clang/lib/Sema/SemaDecl.cpp | 27 +++
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 93d98e1cbb9c81..70bbb882eb2b13 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4034,6 +4034,12 @@ class Sema final : public SemaBase {
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 LookupResult &OldDecls);
 
+  /// RetireNodesFromMergedDecl - We have just merged the decl 'New' by making
+  /// another definition visible.
+  /// This method performs any necessary cleanup on the parser state to discard
+  /// child nodes from newly parsed decl we are retiring.
+  void RetireNodesFromMergedDecl(Scope *S, Decl *New);
+
   /// MergeFunctionDecl - We just parsed a function 'New' from
   /// declarator D which has the same name and scope as a previous
   /// declaration 'Old'.  Figure out how to resolve this situation,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index f8e5f3c6d309d6..54c4c5f4e5395e 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);
-}
-  }
+  RetireNodesFromMergedDecl(S, NewTag);
 }
   }
 
@@ -2639,6 +2628,20 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   notePreviousDefinition(Old, New->getLocation());
 }
 
+void Sema::RetireNodesFromMergedDecl(Scope *S, Decl *New) {
+  // If this was an unscoped enumeration, yank all of its enumerators
+  // out of the scope.
+  if (auto *ED = dyn_cast(New); ED) {
+Scope *EnumScope = getNonFieldDeclScope(S);
+for (auto *ECD : ED->enumerators()) {
+  assert(EnumScope->isDeclScope(ECD));
+  EnumScope->RemoveDecl(ECD);
+  IdResolver.RemoveDecl(ECD);
+  ECD->getLexicalDeclContext()->removeDecl(ECD);
+}
+  }
+}
+
 /// DeclhasAttr - returns true if decl Declaration already has the target
 /// attribute.
 static bool DeclHasAttr(const Decl *D, const Attr *A) {

>From f07904ae5468658a3e7e4a649cd183734479d6cc Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 12:07:29 +0100
Subject: [PATCH 2/7] Drop enumerators when merging named enums

This fixes a crash in ASTWriter when the stale IdResolver entries are
used when writing the identifier table.

  assert(DeclIDs.contains(D) && "Declaration not emitted!");
---
 clang/include/clang/Sema/Sema.h |  2 +-
 clang/lib/Parse/ParseDecl.cpp   |  2 +-
 clang/lib/Parse/ParseDeclCXX.cpp|  3 +-
 clang/lib/Sema/SemaDecl.cpp |  6 ++-
 clang/test/Modules/modules-merge-enum.m | 52 +
 5 files changed, 60 insertions(+), 5 deletions(-)
 create mode 100644 clang/test/Modules/modules-merge-enum.m

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 70bbb882eb2b13..d53bcf63dd1bd9 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3910,7 +3910,7 @@ class Sema final : public SemaBase {
   /// Perform ODR-like check for C/ObjC when merging tag types from modules.
   /// Differently from C++, actually parse the body and reject / error out
   /// in case of a structural mismatch.
-  bool ActOnDuplicateDefinition(Decl *Prev, SkipBodyInfo &SkipBody);
+  bool ActOnDuplicateDefinition(Scope *S, Decl *Prev, SkipBodyInfo &SkipBody);
 
   typedef void *SkippedDefinitionContext;
 
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 31984453487aef..750301a9155d79 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -5645,7 +5645,7 @@ void Parser::ParseEnumSpecifier(SourceLocation StartLoc, 
DeclSpec &DS,
 Decl *D = SkipBody.CheckSameAsPrevious ? SkipBody.New : TagDecl;
 ParseEnumBody(StartLoc, D);
 if (SkipBody.CheckSameAsPrevious &&
-!Actions.ActOnDuplicateDefinition(TagDecl, SkipBody)) {
+!Actions.ActOnDuplicateDefinition(getCurScope(), TagDecl, SkipBody)) {
   DS.SetT

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-02-27 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource updated 
https://github.com/llvm/llvm-project/pull/114240

>From cc3cf25da67c9f8b9edabb318c6011cad9bd2f58 Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 11:16:09 +0100
Subject: [PATCH 1/9] [NFC] Factor out RetireNodesFromMergedDecl

---
 clang/include/clang/Sema/Sema.h |  6 ++
 clang/lib/Sema/SemaDecl.cpp | 27 +++
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 93d98e1cbb9c8..70bbb882eb2b1 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4034,6 +4034,12 @@ class Sema final : public SemaBase {
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 LookupResult &OldDecls);
 
+  /// RetireNodesFromMergedDecl - We have just merged the decl 'New' by making
+  /// another definition visible.
+  /// This method performs any necessary cleanup on the parser state to discard
+  /// child nodes from newly parsed decl we are retiring.
+  void RetireNodesFromMergedDecl(Scope *S, Decl *New);
+
   /// MergeFunctionDecl - We just parsed a function 'New' from
   /// declarator D which has the same name and scope as a previous
   /// declaration 'Old'.  Figure out how to resolve this situation,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index f8e5f3c6d309d..54c4c5f4e5395 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);
-}
-  }
+  RetireNodesFromMergedDecl(S, NewTag);
 }
   }
 
@@ -2639,6 +2628,20 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   notePreviousDefinition(Old, New->getLocation());
 }
 
+void Sema::RetireNodesFromMergedDecl(Scope *S, Decl *New) {
+  // If this was an unscoped enumeration, yank all of its enumerators
+  // out of the scope.
+  if (auto *ED = dyn_cast(New); ED) {
+Scope *EnumScope = getNonFieldDeclScope(S);
+for (auto *ECD : ED->enumerators()) {
+  assert(EnumScope->isDeclScope(ECD));
+  EnumScope->RemoveDecl(ECD);
+  IdResolver.RemoveDecl(ECD);
+  ECD->getLexicalDeclContext()->removeDecl(ECD);
+}
+  }
+}
+
 /// DeclhasAttr - returns true if decl Declaration already has the target
 /// attribute.
 static bool DeclHasAttr(const Decl *D, const Attr *A) {

>From f07904ae5468658a3e7e4a649cd183734479d6cc Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 12:07:29 +0100
Subject: [PATCH 2/9] Drop enumerators when merging named enums

This fixes a crash in ASTWriter when the stale IdResolver entries are
used when writing the identifier table.

  assert(DeclIDs.contains(D) && "Declaration not emitted!");
---
 clang/include/clang/Sema/Sema.h |  2 +-
 clang/lib/Parse/ParseDecl.cpp   |  2 +-
 clang/lib/Parse/ParseDeclCXX.cpp|  3 +-
 clang/lib/Sema/SemaDecl.cpp |  6 ++-
 clang/test/Modules/modules-merge-enum.m | 52 +
 5 files changed, 60 insertions(+), 5 deletions(-)
 create mode 100644 clang/test/Modules/modules-merge-enum.m

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 70bbb882eb2b1..d53bcf63dd1bd 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3910,7 +3910,7 @@ class Sema final : public SemaBase {
   /// Perform ODR-like check for C/ObjC when merging tag types from modules.
   /// Differently from C++, actually parse the body and reject / error out
   /// in case of a structural mismatch.
-  bool ActOnDuplicateDefinition(Decl *Prev, SkipBodyInfo &SkipBody);
+  bool ActOnDuplicateDefinition(Scope *S, Decl *Prev, SkipBodyInfo &SkipBody);
 
   typedef void *SkippedDefinitionContext;
 
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 31984453487ae..750301a9155d7 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -5645,7 +5645,7 @@ void Parser::ParseEnumSpecifier(SourceLocation StartLoc, 
DeclSpec &DS,
 Decl *D = SkipBody.CheckSameAsPrevious ? SkipBody.New : TagDecl;
 ParseEnumBody(StartLoc, D);
 if (SkipBody.CheckSameAsPrevious &&
-!Actions.ActOnDuplicateDefinition(TagDecl, SkipBody)) {
+!Actions.ActOnDuplicateDefinition(getCurScope(), TagDecl, SkipBody)) {
   DS.SetTypeSpecE

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-03-07 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource updated 
https://github.com/llvm/llvm-project/pull/114240

>From 9eede4531ad27ce1d77df492076d5fbcd237fd3f Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Fri, 7 Mar 2025 09:32:54 +0100
Subject: [PATCH] [clang] Fix ASTWriter crash after merging named enums

---
 clang/docs/ReleaseNotes.rst |   1 +
 clang/include/clang/Sema/Sema.h |   8 +-
 clang/lib/Parse/ParseDecl.cpp   |   2 +-
 clang/lib/Parse/ParseDeclCXX.cpp|   3 +-
 clang/lib/Sema/SemaDecl.cpp |  30 ---
 clang/test/Modules/modules-merge-enum.m | 111 
 6 files changed, 139 insertions(+), 16 deletions(-)
 create mode 100644 clang/test/Modules/modules-merge-enum.m

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 28856c27317f3..a25808e36bd51 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -248,6 +248,7 @@ Bug Fixes in This Version
 
 - Clang now outputs correct values when #embed data contains bytes with 
negative
   signed char values (#GH102798).
+- Fixed a crash when merging named enumerations in modules (#GH114240).
 - Fixed rejects-valid problem when #embed appears in std::initializer_list or
   when it can affect template argument deduction (#GH122306).
 - Fix crash on code completion of function calls involving partial order of 
function templates
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 08621e633b55c..fdef57e84ee3d 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4008,7 +4008,7 @@ class Sema final : public SemaBase {
   /// Perform ODR-like check for C/ObjC when merging tag types from modules.
   /// Differently from C++, actually parse the body and reject / error out
   /// in case of a structural mismatch.
-  bool ActOnDuplicateDefinition(Decl *Prev, SkipBodyInfo &SkipBody);
+  bool ActOnDuplicateDefinition(Scope *S, Decl *Prev, SkipBodyInfo &SkipBody);
 
   typedef void *SkippedDefinitionContext;
 
@@ -4132,6 +4132,12 @@ class Sema final : public SemaBase {
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 LookupResult &OldDecls);
 
+  /// CleanupMergedEnum - We have just merged the decl 'New' by making another
+  /// definition visible.
+  /// This method performs any necessary cleanup on the parser state to discard
+  /// child nodes from newly parsed decl we are retiring.
+  void CleanupMergedEnum(Scope *S, Decl *New);
+
   /// MergeFunctionDecl - We just parsed a function 'New' from
   /// declarator D which has the same name and scope as a previous
   /// declaration 'Old'.  Figure out how to resolve this situation,
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 7ae136af47391..31cb4a24ad97e 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -5652,7 +5652,7 @@ void Parser::ParseEnumSpecifier(SourceLocation StartLoc, 
DeclSpec &DS,
 Decl *D = SkipBody.CheckSameAsPrevious ? SkipBody.New : TagDecl;
 ParseEnumBody(StartLoc, D);
 if (SkipBody.CheckSameAsPrevious &&
-!Actions.ActOnDuplicateDefinition(TagDecl, SkipBody)) {
+!Actions.ActOnDuplicateDefinition(getCurScope(), TagDecl, SkipBody)) {
   DS.SetTypeSpecError();
   return;
 }
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 43db715ac6d70..e48a35078892d 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -2350,7 +2350,8 @@ void Parser::ParseClassSpecifier(tok::TokenKind 
TagTokKind,
   // Parse the definition body.
   ParseStructUnionBody(StartLoc, TagType, cast(D));
   if (SkipBody.CheckSameAsPrevious &&
-  !Actions.ActOnDuplicateDefinition(TagOrTempResult.get(), SkipBody)) {
+  !Actions.ActOnDuplicateDefinition(getCurScope(),
+TagOrTempResult.get(), SkipBody)) {
 DS.SetTypeSpecError();
 return;
   }
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 714210c3856d7..5716eb61d4ae8 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2564,18 +2564,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);
-}
-  }
+  CleanupMergedEnum(S, NewTag);
 }
   }
 
@@ -2652,6 +2641,19 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
Typ

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-03-07 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource updated 
https://github.com/llvm/llvm-project/pull/114240

>From d5c710e5a16f77b5a44bb842d09674a8b40dbd0d Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Fri, 7 Mar 2025 09:32:54 +0100
Subject: [PATCH] [clang] Fix ASTWriter crash after merging named enums

---
 clang/docs/ReleaseNotes.rst |   1 +
 clang/include/clang/Sema/Sema.h |   8 +-
 clang/lib/Parse/ParseDecl.cpp   |   2 +-
 clang/lib/Parse/ParseDeclCXX.cpp|   3 +-
 clang/lib/Sema/SemaDecl.cpp |  30 ---
 clang/test/Modules/modules-merge-enum.m | 111 
 6 files changed, 139 insertions(+), 16 deletions(-)
 create mode 100644 clang/test/Modules/modules-merge-enum.m

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 28856c27317f3..a25808e36bd51 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -248,6 +248,7 @@ Bug Fixes in This Version
 
 - Clang now outputs correct values when #embed data contains bytes with 
negative
   signed char values (#GH102798).
+- Fixed a crash when merging named enumerations in modules (#GH114240).
 - Fixed rejects-valid problem when #embed appears in std::initializer_list or
   when it can affect template argument deduction (#GH122306).
 - Fix crash on code completion of function calls involving partial order of 
function templates
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 08621e633b55c..fdef57e84ee3d 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4008,7 +4008,7 @@ class Sema final : public SemaBase {
   /// Perform ODR-like check for C/ObjC when merging tag types from modules.
   /// Differently from C++, actually parse the body and reject / error out
   /// in case of a structural mismatch.
-  bool ActOnDuplicateDefinition(Decl *Prev, SkipBodyInfo &SkipBody);
+  bool ActOnDuplicateDefinition(Scope *S, Decl *Prev, SkipBodyInfo &SkipBody);
 
   typedef void *SkippedDefinitionContext;
 
@@ -4132,6 +4132,12 @@ class Sema final : public SemaBase {
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 LookupResult &OldDecls);
 
+  /// CleanupMergedEnum - We have just merged the decl 'New' by making another
+  /// definition visible.
+  /// This method performs any necessary cleanup on the parser state to discard
+  /// child nodes from newly parsed decl we are retiring.
+  void CleanupMergedEnum(Scope *S, Decl *New);
+
   /// MergeFunctionDecl - We just parsed a function 'New' from
   /// declarator D which has the same name and scope as a previous
   /// declaration 'Old'.  Figure out how to resolve this situation,
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 7ae136af47391..31cb4a24ad97e 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -5652,7 +5652,7 @@ void Parser::ParseEnumSpecifier(SourceLocation StartLoc, 
DeclSpec &DS,
 Decl *D = SkipBody.CheckSameAsPrevious ? SkipBody.New : TagDecl;
 ParseEnumBody(StartLoc, D);
 if (SkipBody.CheckSameAsPrevious &&
-!Actions.ActOnDuplicateDefinition(TagDecl, SkipBody)) {
+!Actions.ActOnDuplicateDefinition(getCurScope(), TagDecl, SkipBody)) {
   DS.SetTypeSpecError();
   return;
 }
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 43db715ac6d70..e48a35078892d 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -2350,7 +2350,8 @@ void Parser::ParseClassSpecifier(tok::TokenKind 
TagTokKind,
   // Parse the definition body.
   ParseStructUnionBody(StartLoc, TagType, cast(D));
   if (SkipBody.CheckSameAsPrevious &&
-  !Actions.ActOnDuplicateDefinition(TagOrTempResult.get(), SkipBody)) {
+  !Actions.ActOnDuplicateDefinition(getCurScope(),
+TagOrTempResult.get(), SkipBody)) {
 DS.SetTypeSpecError();
 return;
   }
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 714210c3856d7..5716eb61d4ae8 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2564,18 +2564,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);
-}
-  }
+  CleanupMergedEnum(S, NewTag);
 }
   }
 
@@ -2652,6 +2641,19 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
Typ

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-03-07 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Thank you all for looking into this.

Note that I don't have commit access, and I would appreciate it if someone 
merges this if/when it is ready to merge!

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix quadratic slowdown in AST matcher parent map generation (PR #87824)

2025-03-04 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Hello,
This seems to cause a noticeable increase in memory consumption on some 
examples. See 
https://github.com/llvm/llvm-project/issues/129808#issuecomment-2700015065.

https://github.com/llvm/llvm-project/pull/87824
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-02-27 Thread Michael Jabbour via cfe-commits



@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);

michael-jabbour-sonarsource wrote:

Thank you all very much for checking this. I will be updating the failing test 
in a few hours (or tomorrow).

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-02-27 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-02-23 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Gentle ping :smile:

@vsapsai, @vgvassilev, I think I have responded to your inquiries and added a 
few related questions. Could you have a look?

https://github.com/llvm/llvm-project/pull/114240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-02-26 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource updated 
https://github.com/llvm/llvm-project/pull/114240

>From cc3cf25da67c9f8b9edabb318c6011cad9bd2f58 Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 11:16:09 +0100
Subject: [PATCH 1/8] [NFC] Factor out RetireNodesFromMergedDecl

---
 clang/include/clang/Sema/Sema.h |  6 ++
 clang/lib/Sema/SemaDecl.cpp | 27 +++
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 93d98e1cbb9c8..70bbb882eb2b1 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4034,6 +4034,12 @@ class Sema final : public SemaBase {
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 LookupResult &OldDecls);
 
+  /// RetireNodesFromMergedDecl - We have just merged the decl 'New' by making
+  /// another definition visible.
+  /// This method performs any necessary cleanup on the parser state to discard
+  /// child nodes from newly parsed decl we are retiring.
+  void RetireNodesFromMergedDecl(Scope *S, Decl *New);
+
   /// MergeFunctionDecl - We just parsed a function 'New' from
   /// declarator D which has the same name and scope as a previous
   /// declaration 'Old'.  Figure out how to resolve this situation,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index f8e5f3c6d309d..54c4c5f4e5395 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);
-}
-  }
+  RetireNodesFromMergedDecl(S, NewTag);
 }
   }
 
@@ -2639,6 +2628,20 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   notePreviousDefinition(Old, New->getLocation());
 }
 
+void Sema::RetireNodesFromMergedDecl(Scope *S, Decl *New) {
+  // If this was an unscoped enumeration, yank all of its enumerators
+  // out of the scope.
+  if (auto *ED = dyn_cast(New); ED) {
+Scope *EnumScope = getNonFieldDeclScope(S);
+for (auto *ECD : ED->enumerators()) {
+  assert(EnumScope->isDeclScope(ECD));
+  EnumScope->RemoveDecl(ECD);
+  IdResolver.RemoveDecl(ECD);
+  ECD->getLexicalDeclContext()->removeDecl(ECD);
+}
+  }
+}
+
 /// DeclhasAttr - returns true if decl Declaration already has the target
 /// attribute.
 static bool DeclHasAttr(const Decl *D, const Attr *A) {

>From f07904ae5468658a3e7e4a649cd183734479d6cc Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 12:07:29 +0100
Subject: [PATCH 2/8] Drop enumerators when merging named enums

This fixes a crash in ASTWriter when the stale IdResolver entries are
used when writing the identifier table.

  assert(DeclIDs.contains(D) && "Declaration not emitted!");
---
 clang/include/clang/Sema/Sema.h |  2 +-
 clang/lib/Parse/ParseDecl.cpp   |  2 +-
 clang/lib/Parse/ParseDeclCXX.cpp|  3 +-
 clang/lib/Sema/SemaDecl.cpp |  6 ++-
 clang/test/Modules/modules-merge-enum.m | 52 +
 5 files changed, 60 insertions(+), 5 deletions(-)
 create mode 100644 clang/test/Modules/modules-merge-enum.m

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 70bbb882eb2b1..d53bcf63dd1bd 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3910,7 +3910,7 @@ class Sema final : public SemaBase {
   /// Perform ODR-like check for C/ObjC when merging tag types from modules.
   /// Differently from C++, actually parse the body and reject / error out
   /// in case of a structural mismatch.
-  bool ActOnDuplicateDefinition(Decl *Prev, SkipBodyInfo &SkipBody);
+  bool ActOnDuplicateDefinition(Scope *S, Decl *Prev, SkipBodyInfo &SkipBody);
 
   typedef void *SkippedDefinitionContext;
 
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 31984453487ae..750301a9155d7 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -5645,7 +5645,7 @@ void Parser::ParseEnumSpecifier(SourceLocation StartLoc, 
DeclSpec &DS,
 Decl *D = SkipBody.CheckSameAsPrevious ? SkipBody.New : TagDecl;
 ParseEnumBody(StartLoc, D);
 if (SkipBody.CheckSameAsPrevious &&
-!Actions.ActOnDuplicateDefinition(TagDecl, SkipBody)) {
+!Actions.ActOnDuplicateDefinition(getCurScope(), TagDecl, SkipBody)) {
   DS.SetTypeSpecE

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-02-26 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource updated 
https://github.com/llvm/llvm-project/pull/114240

>From cc3cf25da67c9f8b9edabb318c6011cad9bd2f58 Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 11:16:09 +0100
Subject: [PATCH 1/8] [NFC] Factor out RetireNodesFromMergedDecl

---
 clang/include/clang/Sema/Sema.h |  6 ++
 clang/lib/Sema/SemaDecl.cpp | 27 +++
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 93d98e1cbb9c8..70bbb882eb2b1 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4034,6 +4034,12 @@ class Sema final : public SemaBase {
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 LookupResult &OldDecls);
 
+  /// RetireNodesFromMergedDecl - We have just merged the decl 'New' by making
+  /// another definition visible.
+  /// This method performs any necessary cleanup on the parser state to discard
+  /// child nodes from newly parsed decl we are retiring.
+  void RetireNodesFromMergedDecl(Scope *S, Decl *New);
+
   /// MergeFunctionDecl - We just parsed a function 'New' from
   /// declarator D which has the same name and scope as a previous
   /// declaration 'Old'.  Figure out how to resolve this situation,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index f8e5f3c6d309d..54c4c5f4e5395 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);
-}
-  }
+  RetireNodesFromMergedDecl(S, NewTag);
 }
   }
 
@@ -2639,6 +2628,20 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   notePreviousDefinition(Old, New->getLocation());
 }
 
+void Sema::RetireNodesFromMergedDecl(Scope *S, Decl *New) {
+  // If this was an unscoped enumeration, yank all of its enumerators
+  // out of the scope.
+  if (auto *ED = dyn_cast(New); ED) {
+Scope *EnumScope = getNonFieldDeclScope(S);
+for (auto *ECD : ED->enumerators()) {
+  assert(EnumScope->isDeclScope(ECD));
+  EnumScope->RemoveDecl(ECD);
+  IdResolver.RemoveDecl(ECD);
+  ECD->getLexicalDeclContext()->removeDecl(ECD);
+}
+  }
+}
+
 /// DeclhasAttr - returns true if decl Declaration already has the target
 /// attribute.
 static bool DeclHasAttr(const Decl *D, const Attr *A) {

>From f07904ae5468658a3e7e4a649cd183734479d6cc Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 12:07:29 +0100
Subject: [PATCH 2/8] Drop enumerators when merging named enums

This fixes a crash in ASTWriter when the stale IdResolver entries are
used when writing the identifier table.

  assert(DeclIDs.contains(D) && "Declaration not emitted!");
---
 clang/include/clang/Sema/Sema.h |  2 +-
 clang/lib/Parse/ParseDecl.cpp   |  2 +-
 clang/lib/Parse/ParseDeclCXX.cpp|  3 +-
 clang/lib/Sema/SemaDecl.cpp |  6 ++-
 clang/test/Modules/modules-merge-enum.m | 52 +
 5 files changed, 60 insertions(+), 5 deletions(-)
 create mode 100644 clang/test/Modules/modules-merge-enum.m

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 70bbb882eb2b1..d53bcf63dd1bd 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3910,7 +3910,7 @@ class Sema final : public SemaBase {
   /// Perform ODR-like check for C/ObjC when merging tag types from modules.
   /// Differently from C++, actually parse the body and reject / error out
   /// in case of a structural mismatch.
-  bool ActOnDuplicateDefinition(Decl *Prev, SkipBodyInfo &SkipBody);
+  bool ActOnDuplicateDefinition(Scope *S, Decl *Prev, SkipBodyInfo &SkipBody);
 
   typedef void *SkippedDefinitionContext;
 
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 31984453487ae..750301a9155d7 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -5645,7 +5645,7 @@ void Parser::ParseEnumSpecifier(SourceLocation StartLoc, 
DeclSpec &DS,
 Decl *D = SkipBody.CheckSameAsPrevious ? SkipBody.New : TagDecl;
 ParseEnumBody(StartLoc, D);
 if (SkipBody.CheckSameAsPrevious &&
-!Actions.ActOnDuplicateDefinition(TagDecl, SkipBody)) {
+!Actions.ActOnDuplicateDefinition(getCurScope(), TagDecl, SkipBody)) {
   DS.SetTypeSpecE

[clang] [clang] Fix ASTWriter crash after merging named enums (PR #114240)

2025-02-27 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource updated 
https://github.com/llvm/llvm-project/pull/114240

>From cc3cf25da67c9f8b9edabb318c6011cad9bd2f58 Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 11:16:09 +0100
Subject: [PATCH 1/8] [NFC] Factor out RetireNodesFromMergedDecl

---
 clang/include/clang/Sema/Sema.h |  6 ++
 clang/lib/Sema/SemaDecl.cpp | 27 +++
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 93d98e1cbb9c8..70bbb882eb2b1 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4034,6 +4034,12 @@ class Sema final : public SemaBase {
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
 LookupResult &OldDecls);
 
+  /// RetireNodesFromMergedDecl - We have just merged the decl 'New' by making
+  /// another definition visible.
+  /// This method performs any necessary cleanup on the parser state to discard
+  /// child nodes from newly parsed decl we are retiring.
+  void RetireNodesFromMergedDecl(Scope *S, Decl *New);
+
   /// MergeFunctionDecl - We just parsed a function 'New' from
   /// declarator D which has the same name and scope as a previous
   /// declaration 'Old'.  Figure out how to resolve this situation,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index f8e5f3c6d309d..54c4c5f4e5395 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -2551,18 +2551,7 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   // Make the old tag definition visible.
   makeMergedDefinitionVisible(Hidden);
 
-  // If this was an unscoped enumeration, yank all of its enumerators
-  // out of the scope.
-  if (isa(NewTag)) {
-Scope *EnumScope = getNonFieldDeclScope(S);
-for (auto *D : NewTag->decls()) {
-  auto *ED = cast(D);
-  assert(EnumScope->isDeclScope(ED));
-  EnumScope->RemoveDecl(ED);
-  IdResolver.RemoveDecl(ED);
-  ED->getLexicalDeclContext()->removeDecl(ED);
-}
-  }
+  RetireNodesFromMergedDecl(S, NewTag);
 }
   }
 
@@ -2639,6 +2628,20 @@ void Sema::MergeTypedefNameDecl(Scope *S, 
TypedefNameDecl *New,
   notePreviousDefinition(Old, New->getLocation());
 }
 
+void Sema::RetireNodesFromMergedDecl(Scope *S, Decl *New) {
+  // If this was an unscoped enumeration, yank all of its enumerators
+  // out of the scope.
+  if (auto *ED = dyn_cast(New); ED) {
+Scope *EnumScope = getNonFieldDeclScope(S);
+for (auto *ECD : ED->enumerators()) {
+  assert(EnumScope->isDeclScope(ECD));
+  EnumScope->RemoveDecl(ECD);
+  IdResolver.RemoveDecl(ECD);
+  ECD->getLexicalDeclContext()->removeDecl(ECD);
+}
+  }
+}
+
 /// DeclhasAttr - returns true if decl Declaration already has the target
 /// attribute.
 static bool DeclHasAttr(const Decl *D, const Attr *A) {

>From f07904ae5468658a3e7e4a649cd183734479d6cc Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Tue, 29 Oct 2024 12:07:29 +0100
Subject: [PATCH 2/8] Drop enumerators when merging named enums

This fixes a crash in ASTWriter when the stale IdResolver entries are
used when writing the identifier table.

  assert(DeclIDs.contains(D) && "Declaration not emitted!");
---
 clang/include/clang/Sema/Sema.h |  2 +-
 clang/lib/Parse/ParseDecl.cpp   |  2 +-
 clang/lib/Parse/ParseDeclCXX.cpp|  3 +-
 clang/lib/Sema/SemaDecl.cpp |  6 ++-
 clang/test/Modules/modules-merge-enum.m | 52 +
 5 files changed, 60 insertions(+), 5 deletions(-)
 create mode 100644 clang/test/Modules/modules-merge-enum.m

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 70bbb882eb2b1..d53bcf63dd1bd 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3910,7 +3910,7 @@ class Sema final : public SemaBase {
   /// Perform ODR-like check for C/ObjC when merging tag types from modules.
   /// Differently from C++, actually parse the body and reject / error out
   /// in case of a structural mismatch.
-  bool ActOnDuplicateDefinition(Decl *Prev, SkipBodyInfo &SkipBody);
+  bool ActOnDuplicateDefinition(Scope *S, Decl *Prev, SkipBodyInfo &SkipBody);
 
   typedef void *SkippedDefinitionContext;
 
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 31984453487ae..750301a9155d7 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -5645,7 +5645,7 @@ void Parser::ParseEnumSpecifier(SourceLocation StartLoc, 
DeclSpec &DS,
 Decl *D = SkipBody.CheckSameAsPrevious ? SkipBody.New : TagDecl;
 ParseEnumBody(StartLoc, D);
 if (SkipBody.CheckSameAsPrevious &&
-!Actions.ActOnDuplicateDefinition(TagDecl, SkipBody)) {
+!Actions.ActOnDuplicateDefinition(getCurScope(), TagDecl, SkipBody)) {
   DS.SetTypeSpecE

[clang] [Clang] skip shadow warnings for enum constants in distinct class scopes (PR #115656)

2025-07-02 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Hello,

I think that the following cases used to be valid reports on clang 19, but they 
are not reported in clang 20 after this patch. Is this intended?

```cpp
namespace classEnums {
int globalVar = 0;
class ClassWithEnum {
  enum Enum {
globalVar  // No -Wshadow after this patch.
  };
};

class OuterClass {
  static const int memberData = 0;
  enum Enum {
enumVal
  };
  class InnerClass {
enum Enum {
  memberData,  // No -Wshadow after this patch.
  enumVal  // No -Wshadow after this patch.
};
  };
};
}
```
CE: https://godbolt.org/z/no87EEYxn

@a-tarasyuk, @Fznamznon, @Sirraide

https://github.com/llvm/llvm-project/pull/115656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] Revert "[lit][clang] Avoid realpath on Windows due to MAX_PATH limitations" (PR #139739)

2025-05-22 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Gentle ping :smile:

I would also appreciate any opinions on my previous comment, maybe this can 
help us align on a solution that satisfies all users...

https://github.com/llvm/llvm-project/pull/139739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] Revert "[lit][clang] Avoid realpath on Windows due to MAX_PATH limitations" (PR #139739)

2025-06-16 Thread Michael Jabbour via cfe-commits

michael-jabbour-sonarsource wrote:

Hi @tristanlabelle and thanks for your response,

> 1. Code pervasively access files through canonicalized paths rather than 
> using canonicalized paths only for identity comparisons. The two usages are 
> often conflated and can't be teased apart easily.

I understand that there are possibly many cases where retrieving the canonical 
path is not strictly needed, yet the code does it anyway. However, changing the 
way canonical path for everyone feels a bit drastic to me, as it can also break 
proper code that uses the canonical path for identity comparisons (especially 
with the current implementation limitations I listed earlier in mind). Could 
you elaborate more about the impact of this problem? Does its severity justify 
such a drastic change?

> 2. Hitting the Windows MAX_PATH limitation is a showstopper (broken builds or 
> tests)

I also understand that without this change the `MAX_PATH` limitation can cause 
problems running the tests on certain environments. However, as I mentioned 
earlier, this change breaks the development environments for other environments 
(see [here](https://reviews.llvm.org/D154130#4647550)).

Furthermore, the change doesn't only fix the tests, but also alters the 
behavior in production. [The following 
comment](https://reviews.llvm.org/D154130#4515403) on phabricator caught my 
attention:

> Update clang path canonicalization to avoid resolving substitute drives (ie 
> resolving to a path under a different root). This requires much less change 
> to tests.

Maybe there is actually another way that relies on changing tests only? Do you 
happen to remember the context here? :smile:

I would also appreciate any other opinions on this.

https://github.com/llvm/llvm-project/pull/139739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Fix crash while lazy-loading template specializations (PR #150430)

2025-07-24 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource created 
https://github.com/llvm/llvm-project/pull/150430

## Problem

This is a regression that was observed in Clang 20.

The lazy-loading mechanism for template specializations introduced in #119333 
can currently load additional nodes when called multiple times, which breaks 
assumptions made by code that iterates over specializations. This leads to 
iterator invalidation crashes in some scenarios.

The core issue occurs when:
1. Code calls `spec_begin()` to get an iterator over template specializations. 
This invokes `LoadLazySpecializations()`.
2. Code then calls `spec_end()` to get the end iterator.
3. During the `spec_end()` call, `LoadExternalSpecializations()` is invoked 
again.
4. This can load additional specializations for certain cases, invalidating the 
begin iterator returned in 1.

I was able to trigger the problem when constructing a ParentMapContext. The 
regression test demonstrates two ways to trigger the construction of the 
ParentMapContext on problematic code:
- The ArrayBoundV2 checker
- Unsigned overflow detection in sanitized builds

Unfortunately, simply dumping the ast (e.g. using `-ast-dump-all`) doesn't 
trigger the crash because dumping requires completing the redecl chain before 
iterating over the specializations.

## Solution

The fix ensures that the redeclaration chain is always completed **before** 
loading external specializations by calling `CompleteRedeclChain(D)` at the 
start of `LoadExternalSpecializations()`. The idea is to ensure that all 
`SpecLookups` are fully known and loaded before the call to 
`LoadExternalSpecializationsImpl()`.


>From b850f5d4fa92da0feb0da3ae2f10eee0bd2e43b5 Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Thu, 24 Jul 2025 15:53:08 +0200
Subject: [PATCH] Fix crash while lazy-loading template specializations

---
 clang/lib/Serialization/ASTReader.cpp |  1 +
 ...cializations-lazy-load-parentmap-crash.cpp | 99 +++
 2 files changed, 100 insertions(+)
 create mode 100644 
clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp

diff --git a/clang/lib/Serialization/ASTReader.cpp 
b/clang/lib/Serialization/ASTReader.cpp
index 10aedb68fcd9d..f896f9f11c2b3 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -8488,6 +8488,7 @@ bool 
ASTReader::LoadExternalSpecializationsImpl(SpecLookupTableTy &SpecLookups,
 bool ASTReader::LoadExternalSpecializations(const Decl *D, bool OnlyPartial) {
   assert(D);
 
+  CompleteRedeclChain(D);
   bool NewSpecsFound =
   LoadExternalSpecializationsImpl(PartialSpecializationsLookups, D);
   if (OnlyPartial)
diff --git a/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp 
b/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
new file mode 100644
index 0..2e4489bc5f437
--- /dev/null
+++ b/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
@@ -0,0 +1,99 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file --leading-lines %s %t
+//
+// Prepare the BMIs.
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -o %t/mod_a-part1.pcm 
%t/mod_a-part1.cppm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -o %t/mod_a-part2.pcm 
%t/mod_a-part2.cppm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -o %t/mod_a.pcm 
%t/mod_a.cppm -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a:part1=%t/mod_a-part1.pcm 
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -o %t/mod_b.pcm 
%t/mod_b.cppm -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
+
+// Below are two examples to trigger the construction of the parent map (which 
is necessary to trigger the bug this regression test is for).
+// Using ArrayBoundV2 checker:
+// RUN: %clang_cc1 -std=c++20 -analyze 
-analyzer-checker=security,alpha.security -analyzer-output=text 
%t/test-array-bound-v2.cpp -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm 
-fmodule-file=mod_b=%t/mod_b.pcm
+// Using a sanitized build:
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-fsanitize=unsigned-integer-overflow 
-fsanitize-undefined-ignore-overflow-pattern=all -emit-llvm -o %t/ignored 
%t/test-sanitized-build.cpp -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm 
-fmodule-file=mod_b=%t/mod_b.pcm
+
+//--- mod_a-part1.cppm
+module;
+namespace mod_a {
+template  struct Important;
+}
+
+namespace mod_a {
+Important<0>& instantiate1();
+} // namespace mod_a
+export module mod_a:part1;
+
+export namespace mod_a {
+using ::mod_a::instantiate1;
+}
+
+//--- mod_a-part2.cppm
+module;
+namespace mod_a {
+template  struct Important;
+}
+
+namespace mod_a {
+template  Important& instantiate2();
+namespace part2InternalInstantiations {
+// During the construction o

[clang] [lldb] [Serialization] Support loading template specializations lazily (PR #119333)

2025-07-24 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Hello,

We have encountered crashes after this patch, with stack traces that look like 
this:

```
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and 
include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
  ...
 #0 0x740f12c266f8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
 #1 0x740f12c23de5 llvm::sys::RunSignalHandlers()
 #2 0x740f12c274b1 SignalHandler(int, siginfo_t*, void*) Signals.cpp:0:0
 #3 0x740f12242520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #4 0x740f10a04134 
clang::Redeclarable::DeclLink::getPrevious(clang::TagDecl 
const*) const APValue.cpp:0:0
 #5 0x740f10a99531 clang::CXXRecordDecl::getMostRecentNonInjectedDecl() 
ASTDumper.cpp:0:0
 #6 0x740f10d71be7 
clang::RecursiveASTVisitor::TraverseTemplateInstantiations(clang::ClassTemplateDecl*)
 #7 0x740f10d4cb37 
clang::RecursiveASTVisitor::TraverseClassTemplateDecl(clang::ClassTemplateDecl*)
 #8 0x740f10d46926 
clang::RecursiveASTVisitor::TraverseDecl(clang::Decl*)
 #9 0x740f10d6ac21 
clang::RecursiveASTVisitor::TraverseDeclContextHelper(clang::DeclContext*)
#10 0x740f10d4d0c4 
clang::RecursiveASTVisitor::TraverseNamespaceDecl(clang::NamespaceDecl*)
#11 0x740f10d46b9c 
clang::RecursiveASTVisitor::TraverseDecl(clang::Decl*)
#12 0x740f10d6ac21 
clang::RecursiveASTVisitor::TraverseDeclContextHelper(clang::DeclContext*)
#13 0x740f10d46f87 
clang::RecursiveASTVisitor::TraverseTranslationUnitDecl(clang::TranslationUnitDecl*)
#14 0x740f10d468a0 
clang::RecursiveASTVisitor::TraverseDecl(clang::Decl*)
#15 0x740f10d45d38 
clang::ParentMapContext::ParentMap::ASTVisitor::TraverseDecl(clang::Decl*) 
ParentMapContext.cpp:0:0
#16 0x740f10d42801 
clang::ParentMapContext::ParentMap::ParentMap(clang::ASTContext&)
#17 0x740f10d42876 clang::ParentMapContext::getParents(clang::DynTypedNode 
const&)
#18 0x740f1750739b (anonymous 
namespace)::ScalarExprEmitter::EmitScalarPrePostIncDec(clang::UnaryOperator 
const*, clang::CodeGen::LValue, bool, bool) CGExprScalar.cpp:0:0
#19 0x740f175132be (anonymous 
namespace)::ScalarExprEmitter::VisitUnaryPostDec(clang::UnaryOperator const*) 
CGExprScalar.cpp:0:0
#20 0x740f175051eb (anonymous 
namespace)::ScalarExprEmitter::Visit(clang::Expr*) CGExprScalar.cpp:0:0
#21 0x740f17521bdd (anonymous 
namespace)::ScalarExprEmitter::VisitCastExpr(clang::CastExpr*) 
CGExprScalar.cpp:0:0
#22 0x740f175050dd 
clang::CodeGen::CodeGenFunction::EmitScalarExpr(clang::Expr const*, bool)
#23 0x740f1748fe28 
clang::CodeGen::CodeGenFunction::EvaluateExprAsBool(clang::Expr const*)
#24 0x740f176722d3 
clang::CodeGen::CodeGenFunction::EmitWhileStmt(clang::WhileStmt const&, 
llvm::ArrayRef)
#25 0x740f17670edf clang::CodeGen::CodeGenFunction::EmitStmt(clang::Stmt 
const*, llvm::ArrayRef)
#26 0x740f1767ed60 
clang::CodeGen::CodeGenFunction::EmitCompoundStmtWithoutScope(clang::CompoundStmt
 const&, bool, clang::CodeGen::AggValueSlot)
#27 0x740f176f765e 
clang::CodeGen::CodeGenFunction::GenerateCode(clang::GlobalDecl, 
llvm::Function*, clang::CodeGen::CGFunctionInfo const&) 
#28 0x740f17721d6e 
clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition(clang::GlobalDecl, 
llvm::GlobalValue*)
#29 0x740f1771950e 
clang::CodeGen::CodeGenModule::EmitGlobalDefinition(clang::GlobalDecl, 
llvm::GlobalValue*) 
#30 0x740f1771e832 
clang::CodeGen::CodeGenModule::EmitGlobal(clang::GlobalDecl) 
#31 0x740f17718011 
clang::CodeGen::CodeGenModule::EmitTopLevelDecl(clang::Decl*) 
#32 0x740f177eb7bc (anonymous 
namespace)::CodeGeneratorImpl::HandleTopLevelDecl(clang::DeclGroupRef) 
ModuleBuilder.cpp:0:0
#33 0x740f176e2726 
clang::BackendConsumer::HandleTopLevelDecl(clang::DeclGroupRef) 
#34 0x740f11af6f8a clang::ParseAST(clang::Sema&, bool, bool) 
#35 0x740f155ee356 clang::FrontendAction::Execute() 
#36 0x740f1555060d 
clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) 
#37 0x740f18a75ae7 
clang::ExecuteCompilerInvocation(clang::CompilerInstance*) 
#38 0x63e90df5329f cc1_main(llvm::ArrayRef, char const*, 
void*) 
#39 0x63e90df4f62d ExecuteCC1Tool(llvm::SmallVectorImpl&, 
llvm::ToolContext const&) driver.cpp:0:0
#40 0x63e90df4e669 clang_main(int, char**, llvm::ToolContext const&) 
#41 0x63e90df601a7 main 
#42 0x740f12229d90 __libc_start_call_main 
./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#43 0x740f12229e40 call_init ./csu/../csu/libc-start.c:128:20
#44 0x740f12229e40 __libc_start_main ./csu/../csu/libc-start.c:379:5
#45 0x63e90df4d0c5 _start 
```
I have created a fix and a test case in #150430. Any feedback is appreciated.

https://github.com/llvm/llvm-project/pull/119333
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Fix crash while lazy-loading template specializations (PR #150430)

2025-07-24 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource updated 
https://github.com/llvm/llvm-project/pull/150430

>From 3e49b5c4f7c572f09e39c7c26dfbf6f1c58351ef Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Thu, 24 Jul 2025 15:53:08 +0200
Subject: [PATCH] Fix crash while lazy-loading template specializations

---
 clang/lib/Serialization/ASTReader.cpp |  1 +
 ...cializations-lazy-load-parentmap-crash.cpp | 99 +++
 2 files changed, 100 insertions(+)
 create mode 100644 
clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp

diff --git a/clang/lib/Serialization/ASTReader.cpp 
b/clang/lib/Serialization/ASTReader.cpp
index 10aedb68fcd9d..f896f9f11c2b3 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -8488,6 +8488,7 @@ bool 
ASTReader::LoadExternalSpecializationsImpl(SpecLookupTableTy &SpecLookups,
 bool ASTReader::LoadExternalSpecializations(const Decl *D, bool OnlyPartial) {
   assert(D);
 
+  CompleteRedeclChain(D);
   bool NewSpecsFound =
   LoadExternalSpecializationsImpl(PartialSpecializationsLookups, D);
   if (OnlyPartial)
diff --git a/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp 
b/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
new file mode 100644
index 0..aaa4e98224411
--- /dev/null
+++ b/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
@@ -0,0 +1,99 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file --leading-lines %s %t
+//
+// Prepare the BMIs.
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_a-part1.pcm %t/mod_a-part1.cppm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_a-part2.pcm %t/mod_a-part2.cppm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_a.pcm %t/mod_a.cppm 
-fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_b.pcm %t/mod_b.cppm 
-fmodule-file=mod_a:part2=%t/mod_a-part2.pcm -fmodule-file=mod_a=%t/mod_a.pcm 
-fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
+
+// Below are two examples to trigger the construction of the parent map (which 
is necessary to trigger the bug this regression test is for).
+// Using ArrayBoundV2 checker:
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -analyze 
-analyzer-checker=security,alpha.security -analyzer-output=text 
%t/test-array-bound-v2.cpp -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm 
-fmodule-file=mod_b=%t/mod_b.pcm
+// Using a sanitized build:
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-fsanitize=unsigned-integer-overflow 
-fsanitize-undefined-ignore-overflow-pattern=all -emit-llvm -o %t/ignored 
%t/test-sanitized-build.cpp -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm 
-fmodule-file=mod_b=%t/mod_b.pcm
+
+//--- mod_a-part1.cppm
+module;
+namespace mod_a {
+template  struct Important;
+}
+
+namespace mod_a {
+Important<0>& instantiate1();
+} // namespace mod_a
+export module mod_a:part1;
+
+export namespace mod_a {
+using ::mod_a::instantiate1;
+}
+
+//--- mod_a-part2.cppm
+module;
+namespace mod_a {
+template  struct Important;
+}
+
+namespace mod_a {
+template  Important& instantiate2();
+namespace part2InternalInstantiations {
+// During the construction of the parent map, we iterate over 
ClassTemplateDecl::specializations() for 'Important'.
+// After GH119333, the following instantiations get loaded between the call to 
spec_begin() and spec_end().
+// This used to invalidate the begin iterator returned by spec_begin() by the 
time the end iterator is returned.
+// This is a regression test for that.
+Important<1> fn1();
+Important<2> fn2();
+Important<3> fn3();
+Important<4> fn4();
+Important<5> fn5();
+Important<6> fn6();
+Important<7> fn7();
+Important<8> fn8();
+Important<9> fn9();
+Important<10> fn10();
+Important<11> fn11();
+}
+} // namespace mod_a
+export module mod_a:part2;
+
+export namespace mod_a {
+using ::mod_a::instantiate2;
+}
+
+//--- mod_a.cppm
+export module mod_a;
+export import :part1;
+export import :part2;
+
+//--- mod_b.cppm
+export module mod_b;
+import mod_a;
+
+void a() {
+  mod_a::instantiate1();
+  mod_a::instantiate2<42>();
+}
+
+//--- test-array-bound-v2.cpp
+import mod_b;
+
+extern void someFunc(char* first, char* last);
+void triggerParentMapContextCreationThroughArrayBoundV2() {
+  // This code currently causes the ArrayBoundV2 checker to create the 
ParentMapContext.
+  // Once it detects an access to buf[100], the checker looks through the 
parents find '&' operator.
+  // (since taking the address of past-the-end pointer is allowed by the 
checker)
+  char buf[100];
+  someFu

[clang] [Serialization] Fix crash while lazy-loading template specializations (PR #150430)

2025-07-24 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource updated 
https://github.com/llvm/llvm-project/pull/150430

>From c3cf089fb721a52993c42d0fe18547685d8badb6 Mon Sep 17 00:00:00 2001
From: Michael Jabbour 
Date: Thu, 24 Jul 2025 15:53:08 +0200
Subject: [PATCH] Fix crash while lazy-loading template specializations

---
 clang/lib/Serialization/ASTReader.cpp |  1 +
 ...cializations-lazy-load-parentmap-crash.cpp | 99 +++
 2 files changed, 100 insertions(+)
 create mode 100644 
clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp

diff --git a/clang/lib/Serialization/ASTReader.cpp 
b/clang/lib/Serialization/ASTReader.cpp
index 10aedb68fcd9d..f896f9f11c2b3 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -8488,6 +8488,7 @@ bool 
ASTReader::LoadExternalSpecializationsImpl(SpecLookupTableTy &SpecLookups,
 bool ASTReader::LoadExternalSpecializations(const Decl *D, bool OnlyPartial) {
   assert(D);
 
+  CompleteRedeclChain(D);
   bool NewSpecsFound =
   LoadExternalSpecializationsImpl(PartialSpecializationsLookups, D);
   if (OnlyPartial)
diff --git a/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp 
b/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
new file mode 100644
index 0..bd07ada631355
--- /dev/null
+++ b/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
@@ -0,0 +1,99 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file --leading-lines %s %t
+//
+// Prepare the BMIs.
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_a-part1.pcm %t/mod_a-part1.cppm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_a-part2.pcm %t/mod_a-part2.cppm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_a.pcm %t/mod_a.cppm 
-fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_b.pcm %t/mod_b.cppm 
-fmodule-file=mod_a:part2=%t/mod_a-part2.pcm -fmodule-file=mod_a=%t/mod_a.pcm 
-fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
+
+// Below are two examples to trigger the construction of the parent map (which 
is necessary to trigger the bug this regression test is for).
+// Using ArrayBoundV2 checker:
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -analyze 
-analyzer-checker=security,alpha.security -analyzer-output=text 
%t/test-array-bound-v2.cpp -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm 
-fmodule-file=mod_b=%t/mod_b.pcm
+// Using a sanitized build:
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-fsanitize=unsigned-integer-overflow 
-fsanitize-undefined-ignore-overflow-pattern=all -emit-llvm -o %t/ignored 
%t/test-sanitized-build.cpp -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm 
-fmodule-file=mod_b=%t/mod_b.pcm
+
+//--- mod_a-part1.cppm
+module;
+namespace mod_a {
+template  struct Important;
+}
+
+namespace mod_a {
+Important<0>& instantiate1();
+} // namespace mod_a
+export module mod_a:part1;
+
+export namespace mod_a {
+using ::mod_a::instantiate1;
+}
+
+//--- mod_a-part2.cppm
+module;
+namespace mod_a {
+template  struct Important;
+}
+
+namespace mod_a {
+template  Important& instantiate2();
+namespace part2InternalInstantiations {
+// During the construction of the parent map, we iterate over 
ClassTemplateDecl::specializations() for 'Important'.
+// After GH119333, the following instantiations get loaded between the call to 
spec_begin() and spec_end().
+// This used to invalidate the begin iterator returned by spec_begin() by the 
time the end iterator is returned.
+// This is a regression test for that.
+Important<1> fn1();
+Important<2> fn2();
+Important<3> fn3();
+Important<4> fn4();
+Important<5> fn5();
+Important<6> fn6();
+Important<7> fn7();
+Important<8> fn8();
+Important<9> fn9();
+Important<10> fn10();
+Important<11> fn11();
+}
+} // namespace mod_a
+export module mod_a:part2;
+
+export namespace mod_a {
+using ::mod_a::instantiate2;
+}
+
+//--- mod_a.cppm
+export module mod_a;
+export import :part1;
+export import :part2;
+
+//--- mod_b.cppm
+export module mod_b;
+import mod_a;
+
+void a() {
+  mod_a::instantiate1();
+  mod_a::instantiate2<42>();
+}
+
+//--- test-array-bound-v2.cpp
+import mod_b;
+
+extern void someFunc(char* first, char* last);
+void triggerParentMapContextCreationThroughArrayBoundV2() {
+  // This code currently causes the ArrayBoundV2 checker to create the 
ParentMapContext.
+  // Once it detects an access to buf[100], the checker looks through the 
parents to find '&' operator.
+  // (this is needed since taking the address of past-the-end pointer is 
allowed by the checker)
+  char b

[clang] [Serialization] Fix crash while lazy-loading template specializations (PR #150430)

2025-07-26 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Thank you all for the reviews. I don’t have commit access, so I’ll need someone 
to merge this for me eventually. I’ll be away for the next week and won’t be 
able to respond to any regressions or follow-ups during that time.

If you’re comfortable merging it as is, please feel free to do so. Otherwise, 
I’ll ask for a merge when I’m back.

https://github.com/llvm/llvm-project/pull/150430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Fix crash while lazy-loading template specializations (PR #150430)

2025-07-28 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Hi @ChuanqiXu9, could you please clarify the request to "update the mail"? I've 
double-checked that the commit author is currently set to my email, and I can't 
see any github autogenerated email addresses on the commit metdata.

I want to make sure I understand what needs to be changed. Thank you!

https://github.com/llvm/llvm-project/pull/150430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Fix crash while lazy-loading template specializations (PR #150430)

2025-07-28 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

I am a bit confused where this comes from. The correct email address is also 
shown on the commit: 
https://api.github.com/repos/llvm/llvm-project/commits/c3cf089fb721a52993c42d0fe18547685d8badb6

https://github.com/llvm/llvm-project/pull/150430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Fix crash while lazy-loading template specializations (PR #150430)

2025-07-28 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

Thank you @hahnjo very much for the hint. I had wrongly assumed this is coming 
from the commit metadata.

I just changed the setting. Let me know if it still doesn't work.

Thanks again to you all...

https://github.com/llvm/llvm-project/pull/150430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Fix crash while lazy-loading template specializations (PR #150430)

2025-07-28 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

I am a bit confused where this comes from. The correct email address is also 
shown on the commit: 
https://api.github.com/repos/llvm/llvm-project/commits/c3cf089fb721a52993c42d0fe18547685d8badb6

https://github.com/llvm/llvm-project/pull/150430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][test] Split out staticanalyzer portion of Modules/specializations-lazy-load-parentmap-crash.cpp (PR #151259)

2025-08-01 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource commented:

LGTM. Thank you.

https://github.com/llvm/llvm-project/pull/151259
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][test] Split out staticanalyzer portion of Modules/specializations-lazy-load-parentmap-crash.cpp (PR #151259)

2025-08-01 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/151259
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][test] Split out staticanalyzer portion of Modules/specializations-lazy-load-parentmap-crash.cpp (PR #151259)

2025-08-01 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource commented:

LGTM. Thanks for fixing this.

I left two minor comments. Feel free to ignore if they are not relevant.

https://github.com/llvm/llvm-project/pull/151259
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][test] Split out staticanalyzer portion of Modules/specializations-lazy-load-parentmap-crash.cpp (PR #151259)

2025-08-01 Thread Michael Jabbour via cfe-commits



@@ -0,0 +1,98 @@
+// REQUIRES: staticanalyzer
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file --leading-lines %s %t
+//
+// Prepare the BMIs.
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_a-part1.pcm %t/mod_a-part1.cppm

michael-jabbour-sonarsource wrote:

Just a small nitpick: I think that the triple argument can be removed from the 
commands in the ArrayBoundV2 test now that they are separate (they were only  
needed for the sanitized build test).

https://github.com/llvm/llvm-project/pull/151259
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][test] Split out staticanalyzer portion of Modules/specializations-lazy-load-parentmap-crash.cpp (PR #151259)

2025-08-01 Thread Michael Jabbour via cfe-commits



@@ -8,10 +10,7 @@
 // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_a.pcm %t/mod_a.cppm 
-fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
 // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_b.pcm %t/mod_b.cppm 
-fmodule-file=mod_a:part2=%t/mod_a-part2.pcm -fmodule-file=mod_a=%t/mod_a.pcm 
-fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
 
-// Below are two examples to trigger the construction of the parent map (which 
is necessary to trigger the bug this regression test is for).
-// Using ArrayBoundV2 checker:
-// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -analyze 
-analyzer-checker=security,alpha.security -analyzer-output=text 
%t/test-array-bound-v2.cpp -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm 
-fmodule-file=mod_b=%t/mod_b.pcm
-// Using a sanitized build:
+// Trigger the construction of the parent map (which is necessary to trigger 
the bug this regression test is for) using ArrayBoundV2 checker using a 
sanitized build:

michael-jabbour-sonarsource wrote:

```suggestion
// Trigger the construction of the parent map (which is necessary to trigger 
the bug this regression test is for) using a sanitized build:
```


https://github.com/llvm/llvm-project/pull/151259
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][test] Split out staticanalyzer portion of Modules/specializations-lazy-load-parentmap-crash.cpp (PR #151259)

2025-08-01 Thread Michael Jabbour via cfe-commits



@@ -0,0 +1,98 @@
+// REQUIRES: staticanalyzer
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file --leading-lines %s %t
+//
+// Prepare the BMIs.
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_a-part1.pcm %t/mod_a-part1.cppm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_a-part2.pcm %t/mod_a-part2.cppm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_a.pcm %t/mod_a.cppm 
-fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu 
-emit-module-interface -o %t/mod_b.pcm %t/mod_b.cppm 
-fmodule-file=mod_a:part2=%t/mod_a-part2.pcm -fmodule-file=mod_a=%t/mod_a.pcm 
-fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
+
+// Trigger the construction of the parent map (which is necessary to trigger 
the bug this regression test is for) using ArrayBoundV2 checker:
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -analyze 
-analyzer-checker=security,alpha.security -analyzer-output=text 
%t/test-array-bound-v2.cpp -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm 
-fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm 
-fmodule-file=mod_b=%t/mod_b.pcm
+
+//--- mod_a-part1.cppm
+module;
+namespace mod_a {
+template  struct Important;
+}
+
+namespace mod_a {
+Important<0>& instantiate1();
+} // namespace mod_a
+export module mod_a:part1;
+
+export namespace mod_a {
+using ::mod_a::instantiate1;
+}
+
+//--- mod_a-part2.cppm
+module;
+namespace mod_a {
+template  struct Important;
+}
+
+namespace mod_a {
+template  Important& instantiate2();
+namespace part2InternalInstantiations {
+// During the construction of the parent map, we iterate over 
ClassTemplateDecl::specializations() for 'Important'.
+// After GH119333, the following instantiations get loaded between the call to 
spec_begin() and spec_end().
+// This used to invalidate the begin iterator returned by spec_begin() by the 
time the end iterator is returned.
+// This is a regression test for that.
+Important<1> fn1();
+Important<2> fn2();
+Important<3> fn3();
+Important<4> fn4();
+Important<5> fn5();
+Important<6> fn6();
+Important<7> fn7();
+Important<8> fn8();
+Important<9> fn9();
+Important<10> fn10();
+Important<11> fn11();
+}
+} // namespace mod_a
+export module mod_a:part2;
+
+export namespace mod_a {
+using ::mod_a::instantiate2;
+}
+
+//--- mod_a.cppm
+export module mod_a;
+export import :part1;
+export import :part2;
+
+//--- mod_b.cppm
+export module mod_b;
+import mod_a;
+
+void a() {
+  mod_a::instantiate1();
+  mod_a::instantiate2<42>();
+}
+
+//--- test-array-bound-v2.cpp
+import mod_b;
+
+extern void someFunc(char* first, char* last);
+void triggerParentMapContextCreationThroughArrayBoundV2() {
+  // This code currently causes the ArrayBoundV2 checker to create the 
ParentMapContext.
+  // Once it detects an access to buf[100], the checker looks through the 
parents to find '&' operator.
+  // (this is needed since taking the address of past-the-end pointer is 
allowed by the checker)
+  char buf[100];
+  someFunc(&buf[0], &buf[100]);
+}
+
+//--- test-sanitized-build.cpp

michael-jabbour-sonarsource wrote:

IIUC, this file is not used in this test, and can be removed (along with its 
content). WDYT?

https://github.com/llvm/llvm-project/pull/151259
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Fix crash while lazy-loading template specializations (PR #150430)

2025-08-01 Thread Michael Jabbour via cfe-commits


michael-jabbour-sonarsource wrote:

@sbc100 Yes, this looks like the same failure addressed in #151259 to me

https://github.com/llvm/llvm-project/pull/150430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Fix crash while lazy-loading template specializations (PR #150430)

2025-07-24 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/150430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Fix crash while lazy-loading template specializations (PR #150430)

2025-07-24 Thread Michael Jabbour via cfe-commits


https://github.com/michael-jabbour-sonarsource edited 
https://github.com/llvm/llvm-project/pull/150430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] Revert "[lit][clang] Avoid realpath on Windows due to MAX_PATH limitations" (PR #139739)

2025-05-14 Thread Michael Jabbour via cfe-commits

michael-jabbour-sonarsource wrote:

Thank you very much @compnerd and @tristanlabelle for the information. I am 
writing my thoughts below:

> Everything is being built in S:, there is no reason that the path should be 
> "canonicalised" to %UserProfile%\SourceCache\llvm-project\llvm\... instead of 
> X:\llvm-project which is what you told the tools the path is.

I feel I am missing why being inside `X:` is any different from being inside a 
symlink to another directory. For me, both cases should be treated similarly: 
The canonical path of a file is the one inside `%UserProfile%`. The parts of 
the compiler that don't need the canonical path should always see `X:`, whereas 
the ones that ask for the canonical path should see `%UserProfile%`. Could you 
clarify why should a substituted drive be treated differently from any symlink?

> I agree with that intended definition, but that is not the usage in practice. 
> Code will arbitrarily canonicalize some paths and keep going, rather than 
> using canonicalized paths only for comparisons.

I agree with this, and for me it sounds more like it is a problem in how the 
function is used, not how it behaves. The fix should be to go through the 
usages and make sure they really need to canonicalize IMO.

> I do not know if there is an API that will let you resolve a path into a 
> particular drive, but if so, that would likely be the proper tool to use here.

> In the presence of chains of filesystem decorators, we'll undo the real-path 
> mapping of the entire chain if it changes the drive, whereas we just want the 
> real filesystem at the bottom of the chain to be decorated to avoid changing 
> the drive.

IIUC, both these suggestions aim to address points "1" and "2" in my previous 
response. This might be an improvement over the existing patch (as it would 
start resolving symlinks inside the drive), but concern "3" remains. I would 
still be concerned about calling such functionality a "canonical" path, as it 
doesn't satisfy the canonical property: One may still get different canonical 
paths for the same file, depending on whether they access 
`%UserProfile%/SourceCache/file` and `X:/file`. In other words, I don't agree 
with calling this "root-preserving transformation" a "canonicalization". Could 
you clarify if I am missing something here?

Best regards,
Michael

https://github.com/llvm/llvm-project/pull/139739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] Revert "[lit][clang] Avoid realpath on Windows due to MAX_PATH limitations" (PR #139739)

2025-05-14 Thread Michael Jabbour via cfe-commits

michael-jabbour-sonarsource wrote:

Hello @compnerd, and thank you for sharing the information and for the prompt
response.

> Without this change, a file on a substituted drive will have two separate
> paths.

I am not sure I understand this part. For me, a file on a substituted drive (or
under a symlink) may always have multiple different valid paths to reference
it. What is important, is that it always has a single canonical path. This is
the reason I don’t fully see “root-preserving canonicalization” as a form of a
“canonical path” anymore. To summarize my concerns:

1. The patch seems to work slightly differently from what is described in the
patch description:

1. It doesn't only avoid resolving the mapped drive as claimed, but it also
skips the symlink resolution on mapped drives (it only removes dots in that
case).
2. The impact above is not only limited to substituted/mapped drives, but
to any case where the roots are not identical (for example, also in cross-drive
links).

2. It affects clang’s usability as a library. I feel that such fallback should
be applied on a higher-level API, and ideally should impact only lit tests (as
they are the motivation stated in the patch). For example, by looking at usages
in clang, I could find that many of them still assume that
`FileManager::getCanonicalName` resolves symlinks (at least by looking at the
comments):

1. clangd’s `SourceCode::getCanonicalPath` which claims to resolve symlinks
in its docs. IIUC, it no longer does so if the code is checked out in a
substituted drive (see
[here](https://github.com/llvm/llvm-project/blob/47c892a49136c68425e7ade08553598e63ef4e70/clang-tools-extra/clangd/SourceCode.cpp#L533)).
2. Inside clang itself, there is code that handles directory resolution in
module map and framework header search (for example
[here](https://github.com/llvm/llvm-project/blob/47c892a49136c68425e7ade08553598e63ef4e70/clang/lib/Lex/HeaderSearch.cpp#L573)
and
[here](https://github.com/SonarSource/llvm-project/blob/d84668d2b2ec28a5d6f81e6eb22a17b014a000cf/clang/lib/Lex/ModuleMap.cpp#L449)).
This may not impact Windows users, but I am not entirely sure.

3. Skipping symlink resolution makes it possible for `getCanonicalName` to
return different paths for the same file (breaking the canonical property,
which can impact users of clang as a library, depending on how they use it).

Basically, we have concerns about the correctness of all existing usages of
`getCanonicalName` (inside and outside clang). It also impacted us as users of
clang as a library. If the motivation is only to fix lit tests, I can see a few
alternatives:

1. Try to apply such a fix on a higher-level API where needed, to avoid
unexpected impact? I didn't try to look for such an API myself yet. Do you know
if this been explored while working on the previous patch?

2. Switch to a solution that adapts the test as needed without altering
functionality (e.g. prior to what [was explored after this
comment](https://reviews.llvm.org/D154130#4515403))

3. Revert the patch, and switch to a user-specific workaround which does not
require changing LLVM (as suggested in this PR). Note that the patch, not only
alters the behavior of clang, but also breaks the lit setup for some other
developers on Windows (see [here](https://reviews.llvm.org/D154130#4647550)).

Any thoughts about this are appreciated.

Best regards,
Michael

https://github.com/llvm/llvm-project/pull/139739
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

68 matches

Mail list logo