Skip to content

[clang] fix assert in ADL finding entity in the implicit global module #109882

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

Merged
merged 2 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ code bases.
still supporting SPARC V8 CPUs need to specify ``-mcpu=v8`` with a
`config file
<https://clang.llvm.org/docs/UsersManual.html#configuration-files>`_.

- The ``clang-rename`` tool has been removed.

C/C++ Language Potentially Breaking Changes
Expand Down Expand Up @@ -115,7 +115,7 @@ C++ Language Changes
- Allow single element access of GCC vector/ext_vector_type object to be
constant expression. Supports the `V.xyzw` syntax and other tidbits
as seen in OpenCL. Selecting multiple elements is left as a future work.
- Implement `CWG1815 <https://wg21.link/CWG1815>`_. Support lifetime extension
- Implement `CWG1815 <https://wg21.link/CWG1815>`_. Support lifetime extension
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: generally we don't like such unrelated changes. We can land them seperately in other NFC patches.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just stray whitespace cleanup that is automatically performed by most unix editors.

I don't think doing a whole separate commit to fix it is worth it, specially as there aren't a lot of lines changed here.

We do generally just ignore these unless there is so much noise it makes review difficult.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't do this in the future the policy is indeed to keep unrelated changes separate.

There are many practical reasons for this but mainly it reduces churn in the event of revert now we lose multiple changes and if they where unrelated that is just a waste all around.

of temporary created by aggregate initialization using a default member
initializer.

Expand Down Expand Up @@ -452,6 +452,9 @@ Miscellaneous Clang Crashes Fixed

- Fixed ``-ast-dump`` crashes on codes involving ``concept`` with ``-ast-dump-decl-types``. (#GH94928)

- Fixed internal assertion firing when a declaration in the implicit global
module is found through ADL. (GH#109879)

OpenACC Specific Changes
------------------------

Expand Down
5 changes: 3 additions & 2 deletions clang/lib/Sema/SemaLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3850,8 +3850,9 @@ void Sema::ArgumentDependentLookup(DeclarationName Name, SourceLocation Loc,
// exports are only valid in module purview and outside of any
// PMF (although a PMF should not even be present in a module
// with an import).
assert(FM && FM->isNamedModule() && !FM->isPrivateModule() &&
"bad export context");
assert(FM &&
(FM->isNamedModule() || FM->isImplicitGlobalModule()) &&
!FM->isPrivateModule() && "bad export context");
// .. are attached to a named module M, do not appear in the
// translation unit containing the point of the lookup..
if (D->isInAnotherModuleUnit() &&
Expand Down
25 changes: 25 additions & 0 deletions clang/test/Modules/GH109879-1.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// RUN: rm -rf %t
// RUN: mkdir -p %t
// RUN: split-file %s %t
//
// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm
// RUN: %clang_cc1 -std=c++20 %t/B.cppm -fprebuilt-module-path=%t -emit-module-interface -o %t/B.pcm
// RUN: %clang_cc1 -fsyntax-only -std=c++20 -fprebuilt-module-path=%t -verify %t/C.cpp

//--- A.cppm
export module A;
export extern "C" void foo(struct Bar);

//--- B.cppm
module;
import A;
export module B;

//--- C.cpp
import B;
struct Bar {};
void test() {
foo(Bar());
// expected-error@-1 {{declaration of 'foo' must be imported}}
// [email protected]:2 {{declaration here is not visible}}
}
29 changes: 29 additions & 0 deletions clang/test/Modules/GH109879-2.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// RUN: rm -rf %t
// RUN: mkdir -p %t
// RUN: split-file %s %t
//
// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm
// RUN: %clang_cc1 -std=c++20 %t/B.cppm -fprebuilt-module-path=%t -emit-module-interface -o %t/B.pcm
// RUN: %clang_cc1 -fsyntax-only -std=c++20 -fprebuilt-module-path=%t -verify %t/C.cpp

//--- foo.h
struct Bar {};
extern "C" void foo(struct Bar);

//--- A.cppm
module;
#include "foo.h"
export module A;
export extern "C" using ::foo;
//--- B.cppm
module;
import A;
export module B;

//--- C.cpp
// expected-no-diagnostics
import B;
#include "foo.h"
void test() {
foo(Bar());
}
Loading