Skip to content

Commit

Permalink
[C++20] [Modules] Skip ODR checks if either declaration comes from GMF
Browse files Browse the repository at this point in the history
This patch tries to workaround the case that:
- in a module unit that imports another module unit
- both the module units including overlapped headers
- the compiler emits false positive ODR violation diagnostics for the
  overlapped headers if ODR check is enabled
- the current module units enables PCH

For the third point, we disabled ODR check if the declarations comes
from GMF. However, due to the forth point, the check whether the
declaration comes from GMF failed. Then we still going to check it and
then the users get false positive checks.

What's worse is that, this always happens in clangd, where will generate
the PCH automatically before parsing the input files.

The root cause of the problem we mixed the modules in the semantical
level and the module in the serialization level.

The problem is pretty fundamental and we need time to fix that. But 19.x
is going to be branched and I hope to give clangd better user
experience. So I decided to land this workaround even if it is pretyy
niche and may only work for the case of clangd's pattern.
  • Loading branch information
ChuanqiXu9 committed Jul 19, 2024
1 parent 3b78dfa commit 2f0910d
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 2 deletions.
1 change: 1 addition & 0 deletions clang/lib/Serialization/ASTReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9875,6 +9875,7 @@ void ASTReader::finishPendingActions() {
// We only perform ODR checks for decls not in the explicit
// global module fragment.
!shouldSkipCheckingODR(FD) &&
!shouldSkipCheckingODR(NonConstDefn) &&
FD->getODRHash() != NonConstDefn->getODRHash()) {
if (!isa<CXXMethodDecl>(FD)) {
PendingFunctionOdrMergeFailures[FD].push_back(NonConstDefn);
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/Serialization/ASTReaderDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,7 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
Reader.mergeDefinitionVisibility(OldDef, ED);
// We don't want to check the ODR hash value for declarations from global
// module fragment.
if (!shouldSkipCheckingODR(ED) &&
if (!shouldSkipCheckingODR(ED) && !shouldSkipCheckingODR(OldDef) &&
OldDef->getODRHash() != ED->getODRHash())
Reader.PendingEnumOdrMergeFailures[OldDef].push_back(ED);
} else {
Expand Down Expand Up @@ -2134,7 +2134,7 @@ void ASTDeclReader::MergeDefinitionData(
}

// We don't want to check ODR for decls in the global module fragment.
if (shouldSkipCheckingODR(MergeDD.Definition))
if (shouldSkipCheckingODR(MergeDD.Definition) || shouldSkipCheckingODR(D))
return;

if (D->getODRHash() != MergeDD.ODRHash) {
Expand Down
51 changes: 51 additions & 0 deletions clang/test/Modules/pch-in-module-units.cppm
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Test that we will skip ODR checks for declarations from PCH if they
// were from GMF.
//
// RUN: rm -rf %t
// RUN: mkdir -p %t
// RUN: split-file %s %t
//
// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/A.cppm \
// RUN: -o %t/A.pcm -fskip-odr-check-in-gmf
// RUN: %clang_cc1 -std=c++20 -DDIFF -x c++-header %t/foo.h \
// RUN: -emit-pch -o %t/foo.pch -fskip-odr-check-in-gmf
// RUN: %clang_cc1 -std=c++20 %t/B.cppm -fmodule-file=A=%t/A.pcm -include-pch \
// RUN: %t/foo.pch -verify -fsyntax-only -fskip-odr-check-in-gmf

//--- foo.h
#ifndef FOO_H
#define FOO_H
inline int foo() {
#ifndef DIFF
return 43;
#else
return 45;
#endif
}

class f {
public:
int mem() {
#ifndef DIFF
return 47;
#else
return 45;
#endif
}
};
#endif

//--- A.cppm
module;
#include "foo.h"
export module A;
export using ::foo;
export using ::f;

//--- B.cppm
// expected-no-diagnostics
module;
#include "foo.h"
export module B;
import A;
export int b = foo() + f().mem();

0 comments on commit 2f0910d

Please sign in to comment.