Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] incorrect merging of UsingShadowDecl #80252

Closed
mizvekov opened this issue Feb 1, 2024 · 3 comments · Fixed by #80245
Closed

[clang] incorrect merging of UsingShadowDecl #80252

mizvekov opened this issue Feb 1, 2024 · 3 comments · Fixed by #80245
Assignees
Labels
clang:modules C++20 modules and Clang Header Modules

Comments

@mizvekov
Copy link
Contributor

mizvekov commented Feb 1, 2024

When merging imported modules, equivalent UsingShadowDecls from the TU and from the module would not be merged, resulting in them becoming different entities, instead of redeclarations.

A simple reproducer would be:

foo.h

namespace baz {
  using foo = char;
  using baz::foo;
}

A.cppm

module;
#include "foo.h"
export module A;

B.cpp

#include "foo.h"
import A;
// Since modules are loaded lazily, force loading by performing a lookup.
using xxx = baz::foo;

Building this with:
-cc1 -std=c++20 -emit-module-interface -o A.pcm A.cppm
-cc1 -std=c++20 -fmodule-file=A=A.pcm -fsyntax-only -ast-dump-all -ast-dump-filter baz C.cpp

This would result in the following AST:

NamespaceDecl ... baz
|-...
`-UsingShadowDecl 0x2563bdbe448 ... imported in A.<global> hidden implicit TypeAlias ... 'foo'

...

NamespaceDecl ... baz
|-...
`-UsingShadowDecl 0x2563bdbe1a0 ... implicit TypeAlias ... 'foo'

The first UsingShadowDecl should have been a redeclaration of the second one.

You would expect:

NamespaceDecl ... baz
|- ....
`-UsingShadowDecl 0x1b63f707b58 prev 0x1b63f7078b0 ... imported in A.<global> hidden implicit TypeAlias ... 'foo'

...

NamespaceDecl ... baz
|- ....
`-UsingShadowDecl 0x1b63f7078b0 ... implicit TypeAlias ... 'foo'

This was originally reduced from this example that manifested itself as an ODR violation in the global module fragment:
foo.h

namespace baz {
  using foo = char;
  using baz::foo;
}

bar.h

class bar {
  bar(baz::foo);
};

A.cppm

module;
#include "foo.h"
export module A;

B.cppm

module;
#include "foo.h"
#include "bar.h"
export module B;

C.cpp

#include "foo.h"
import A;

#include "bar.h"
import B;

// Since modules are loaded lazily, force loading them by performing a lookup.
using xxx = bar;

And this compilation would result in an ODR false positive, where the definitions for the bar constructor don't match due to different parameters. This failure stopped happening when we merged the patch to disable the GMF ODR check.

This had been further reduced from a similar example with pure inclusion of libc++ headers, on MSVC platform. s/foo.h/stddef.h s/bar.h/locale

@mizvekov mizvekov added the clang:modules C++20 modules and Clang Header Modules label Feb 1, 2024
@mizvekov mizvekov self-assigned this Feb 1, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 1, 2024

@llvm/issue-subscribers-clang-modules

Author: Matheus Izvekov (mizvekov)

When merging imported modules, equivalent UsingShadowDecls from the TU and from the module would not be merged, resulting in them becoming different entities, instead of redeclarations.

A simple reproducer would be:

foo.h

namespace baz {
  using foo = char;
  using baz::foo;
}

A.cppm

module;
#include "foo.h"
export module A;

B.cpp

#include "foo.h"
import A;
// Since modules are loaded lazily, force loading by performing a lookup.
using xxx = baz::foo;

Building this with:
-cc1 -std=c++20 -emit-module-interface -o A.pcm A.cppm
-cc1 -std=c++20 -fmodule-file=A=A.pcm -fsyntax-only -ast-dump-all -ast-dump-filter baz C.cpp

This would result in the following AST:

NamespaceDecl ... baz
|-...
`-UsingShadowDecl 0x2563bdbe448 ... imported in A.&lt;global&gt; hidden implicit TypeAlias ... 'foo'

...

NamespaceDecl ... baz
|-...
`-UsingShadowDecl 0x2563bdbe1a0 ... implicit TypeAlias ... 'foo'

The first UsingShadowDecl should have been a redeclaration of the second one.

You would expect:

NamespaceDecl ... baz
|- ....
`-UsingShadowDecl 0x1b63f707b58 prev 0x1b63f7078b0 ... imported in A.&lt;global&gt; hidden implicit TypeAlias ... 'foo'

...

NamespaceDecl ... baz
|- ....
`-UsingShadowDecl 0x1b63f7078b0 ... implicit TypeAlias ... 'foo'

This was originally reduced from this example that manifested itself as an ODR violation in the global module fragment:
foo.h

namespace baz {
  using foo = char;
  using baz::foo;
}

bar.h

class bar {
  bar(baz::foo);
};

A.cppm

module;
#include "foo.h"
export module A;

B.cppm

module;
#include "foo.h"
#include "bar.h"
export module B;

C.cpp

#include "foo.h"
import A;

#include "bar.h"
import B;

// Since modules are loaded lazily, force loading them by performing a lookup.
using xxx = bar;

And this compilation would result in an ODR false positive, where the definitions for the bar constructor don't match due to different parameters. This failure stopped happening when we merged the patch to disable the GMF ODR check.

This had been further reduced from a similar example with pure inclusion of libc++ headers, on MSVC platform. s/foo.h/stddef.h s/bar.h/locale

mizvekov added a commit that referenced this issue Feb 1, 2024
Previously, when deciding if two UsingShadowDecls where mergeable,
we would incorrectly only look for both pointing to the exact redecla
ration, whereas the correct thing is to look for declarations to the
same entity.

This problem has existed as far back as 2013, introduced in commit
fd8634a.

This problem could manifest itself as ODR check false positives
when importing modules.

Fixes: #80252
@mizvekov
Copy link
Contributor Author

mizvekov commented Feb 1, 2024

FYI @ChuanqiXu9

mizvekov added a commit that referenced this issue Feb 1, 2024
Previously, when deciding if two UsingShadowDecls where mergeable,
we would incorrectly only look for both pointing to the exact redecla
ration, whereas the correct thing is to look for declarations to the
same entity.

This problem has existed as far back as 2013, introduced in commit
fd8634a.

This problem could manifest itself as ODR check false positives
when importing modules.

Fixes: #80252
mizvekov added a commit that referenced this issue Feb 2, 2024
Previously, when deciding if two UsingShadowDecls where mergeable,
we would incorrectly only look for both pointing to the exact redecla
ration, whereas the correct thing is to look for declarations to the
same entity.

This problem has existed as far back as 2013, introduced in commit
fd8634a.

This problem could manifest itself as ODR check false positives
when importing modules.

Fixes: #80252
mizvekov added a commit that referenced this issue Feb 2, 2024
Previously, when deciding if two UsingShadowDecls where mergeable,
we would incorrectly only look for both pointing to the exact redecla
ration, whereas the correct thing is to look for declarations to the
same entity.

This problem has existed as far back as 2013, introduced in commit
fd8634a.

This problem could manifest itself as ODR check false positives
when importing modules.

Fixes: #80252
@shafik
Copy link
Collaborator

shafik commented Feb 2, 2024

FYI since you are filing this issue to track your progress, you can self-add the "confirmed" label.

mizvekov added a commit that referenced this issue May 30, 2024
Previously, when deciding if two UsingShadowDecls where mergeable,
we would incorrectly only look for both pointing to the exact redecla
ration, whereas the correct thing is to look for declarations to the
same entity.

This problem has existed as far back as 2013, introduced in commit
fd8634a.

This problem could manifest itself as ODR check false positives
when importing modules.

Fixes: #80252
mizvekov added a commit that referenced this issue May 30, 2024
[clang] fix merging of UsingShadowDecl

Previously, when deciding if two UsingShadowDecls where mergeable,
we would incorrectly only look for both pointing to the exact redecla
ration, whereas the correct thing is to look for declarations to the
same entity.

This problem has existed as far back as 2013, introduced in commit
fd8634a.

This problem could manifest itself as ODR check false positives when
importing modules.

Fixes: #80252
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:modules C++20 modules and Clang Header Modules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants