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.

Sign up for GitHub

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

Jump to bottom

[C++20] [Modules] Introduce -fskip-odr-check-in-gmf #79959

Merged
merged 2 commits into from
Feb 1, 2024
Merged

[C++20] [Modules] Introduce -fskip-odr-check-in-gmf #79959

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
beb1a4b
[C++20] [Modules] Introduce -fskip-odr-check-in-gmf
ChuanqiXu9 Jan 30, 2024
48a37e6
Update Document
ChuanqiXu9 Jan 31, 2024
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
3 changes: 2 additions & 1 deletion clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ C++20 Feature Support

- Clang won't perform ODR checks for decls in the global module fragment any
more to ease the implementation and improve the user's using experience.
This follows the MSVC's behavior.
This follows the MSVC's behavior. Users interested in testing the more strict
behavior can use the flag '-Xclang -fno-skip-odr-check-in-gmf'.
(`#79240 <https://github.com/llvm/llvm-project/issues/79240>`_).

C++23 Feature Support
Expand Down
22 changes: 22 additions & 0 deletions clang/docs/StandardCPlusPlusModules.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,28 @@ Note that **currently** the compiler doesn't consider inconsistent macro definit
Currently Clang would accept the above example. But it may produce surprising results if the
debugging code depends on consistent use of ``NDEBUG`` also in other translation units.

Definitions consistency
^^^^^^^^^^^^^^^^^^^^^^^

The C++ language defines that same declarations in different translation units should have
the same definition, as known as ODR (One Definition Rule). Prior to modules, the translation
units don't dependent on each other and the compiler itself don't and can't perform a strong
ODR violation check. Sometimes it is the linker does some jobs related to ODR, where the
higher level semantics are missing. With the introduction of modules, now the compiler have
the chance to perform ODR violations with language semantics across translation units.
ChuanqiXu9 marked this conversation as resolved.
Show resolved Hide resolved

However, in the practice we found the existing ODR checking mechanism may be too aggressive.
In the many issue reports about ODR violation diagnostics, most of them are false positive
ODR violations and the true positive ODR violations are rarely reported. Also MSVC don't
perform ODR check for declarations in the global module fragment.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are many kinds of issues here and the text might not be conveying the right idea.

  1. There are some clang bugs where wrong semantic analysis occurs when building AST from pcm, where otherwise parsing from source produces correct results. This is not a problem in ODR checking per se, but it can manifest itself as that, and this ends up being helpful.
  2. Bugs in the ODR checker itself, where two identical definitions, per the standard, are mistaken as different, or the opposite.
  3. ODR violations in user code, which can some times be mostly harmless, and some times not.

I read this paragraph as talking about 3 in particular, and giving the idea that the ODR, as specified, ends up barfing at too many harmless violations, and we are finding that it's more trouble than it's worth as we enforce it in more situations.

Regarding MSVC, I don't think we normally would relax standards conformance so broadly in order to be consistent with another compiler. For example, see how we handle compatibility with MSVC regarding delayed template parsing.

Copy link
Member Author

Choose a reason for hiding this comment

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

For 1, I don't understand why it is relevant here. I know what you're saying, but I just feel it is not related here. We're talking about ODR checker, right?

For 2, I thought the term false positive implies 2. But maybe it is not impressive. So I tried to rewrite this paragraph to make it more explicit.

For MSVC, it is a simple supporting argument here. I mean, the document is for users, and what I want to say or express is, while all of us know and understand it is not good, but you (the users) don't need to be too panic since MSVC did the same thing.

BTW, for the delayed template parsing, I remember one of the supporting reason to disable that in C++20 is that MSVC disable that in C++20 too. But this is not relevant here.

Copy link
Contributor

Choose a reason for hiding this comment

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

For 2, I thought the term false positive implies 2. But maybe it is not impressive. So I tried to rewrite this paragraph to make it more explicit.

That is surprising to me, because the issue you linked in the original commit, #78850, seems to be a case of 3) to me, mostly harmless but nonetheless an ODR violation.

This is not just about the semantics of the resulting program itself, think about debug info: There will be two definitions of fun, with slightly different debug info, because they refer to different declarations for T, even if they resolve to the same type.

Even if the standard wording talks about the definitions consisting of the same series of tokens, this to me is a case of rule-as-written vs rule-as-intended. Ie the tokens not only have to be same textually, but they need to refer to the same entity.

For 1, I don't understand why it is relevant here. I know what you're saying, but I just feel it is not related here. We're talking about ODR checker, right?

See above, but I am also currently internally tracking two separate bugs which are a case of 1), and the ODR checker was just an innocent witness.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is surprising to me, because the issue you linked in the original commit, #78850, seems to be a case of 3) to me, mostly harmless but nonetheless an ODR violation.

Yeah, the original reproducer looks like a user's bug. But according to the comment, it should be fine since the function fun() have the same spellings and the looked up type refers to the same entity. The explanation comes from @zygoloid, who is the author of the modules TS.

This is not just about the semantics of the resulting program itself, think about debug info: There will be two definitions of fun, with slightly different debug info, because they refer to different declarations for T, even if they resolve to the same type.

Great insight. I feel this is meaningful. Maybe we need to reach out WG21 to confirm this. Can you access the mailing list of WG21?

For 1, I don't understand why it is relevant here. I know what you're saying, but I just feel it is not related here. We're talking about ODR checker, right?

See above, but I am also currently internally tracking two separate bugs which are a case of 1), and the ODR checker was just an innocent witness.

Sorry. I still don't understand why it is related to the patch itself. I feel you're saying, when we compile pcm to obj, we would do some additional works, then we have divergence between compliing source to obj directly with a two phase compilation (source to pcm, pcm to obj). But I feel it is not related to the patch itself or the document here. (I am open to discuss it. Just want to clarify to make us clear about whay we're saying)

And for the direction to disable ODR checks in GMF as I commented in #79240, I thought it as bad initially. But the explanation from MSVC developer moves me. Since the entities in the GMF are basically from headers. Then it wouldn't be a regression in some level. Also I saw many issue reports complaining the false positive ODR violation diagnostics. In fact, after I disabled the ODR checks in GMF, there 2 people reporting github issues saying they feel good and one people sent me a private email to say it solves his problems. So I feel this is the "correct" way to go while I understand it is not strictly correct.

Also I feel your concern here is about deeper other problems. (e.g., what is ODR violation?) And I want to backport the series patches to 18.x. So I am wondering if we can approve this patch and I can start to backport it. Then we can discuss your concern in other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great insight. I feel this is meaningful. Maybe we need to reach out WG21 to confirm this. Can you access the mailing list of WG21?

I am not a member yet, but I know a few people in there.

Sorry. I still don't understand why it is related to the patch itself.

So a little background: The issue #78850 that was referenced in the original commit, which seems to be the main motivator for that change, was reported as two parts:

  1. A real-world-like repro where the user is just including different libstdc++ headers.
  2. The reduced repro, where there is the real ODR violation we were just arguing about.

I have reason to believe, based on similarity to clang bug I am currently working on, that 2) is unrelated to 1).
Based on the commonality between reproducers of #78850 and the bug I am working on.
They both involve UsingShadowDecls, the merging of which is broken, as I will show on an upcoming patch.

If you look at this from another angle, this reduction was performed by the user / reporter, and reducing a false-positive ODR violation repro is hard, because it's so easy you would introduce a real ODR-violation during a reduction step.
So it's reasonable to double-check / do some due-dilligence here.

So I feel we are taking a drastic step here based on potentially incorrect evidence.

If we can confirm what I am saying, then don't you agree we would be losing the whole casus belli against the GMF ODR checker, and we could go back to square one, revert the whole thing, and take a better look at this?

Even if that is not the only issue, my perception is that other supposed ODR violations could be simple issues like this.
And even if there are ODR violations in shipped system headers, couldn't we:

  1. Provide a more targeted workaround rather than disabling the whole thing.
  2. Stand our ground, since the whole C++20 modules thing is new, we can expect users to be using more recent / fixed system headers.

But the explanation from MSVC developer moves me. Since the entities in the GMF are basically from headers. Then it wouldn't be a regression in some level.

I have never seen MSVC source code, so I don't know how relevant it would be for comparison, but I think clang has enough differences in how sema is applied to textual source vs PCM, that this would give me pause and I wouldn't be so sure.

On the other hand, this does not seem to be a regression, since the whole C++20 modules thing is just coming up and we are talking about a new feature and new users.

Also I feel your concern here is about deeper other problems. (e.g., what is ODR violation?) And I want to backport the series patches to 18.x. So I am wondering if we can approve this patch and I can start to backport it. Then we can discuss your concern in other places.

See above. Would you mind waiting a little for my patch, for us to double check we are taking this step with solid footing?

Copy link
Member Author

@ChuanqiXu9 ChuanqiXu9 Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Great insight. I feel this is meaningful. Maybe we need to reach out WG21 to confirm this. Can you access the mailing list of WG21?

I am not a member yet, but I know a few people in there.

Do you need me to send this? I feel this is the deepest problem in the discussion.

clang bug I am currently working on

Is there an issue report?

So I feel we are taking a drastic step here based on potentially incorrect evidence.

To me, the issue is the straw that broke the camel's back. I've seen too many issue reports saying and complaining the false positive ODR violations publicly and privately. It is a long battle.

If we can confirm what I am saying, then don't you agree we would be losing the whole casus belli against the GMF ODR checker, and we could go back to square one, revert the whole thing, and take a better look at this?

If we can have a stable and correct ODR checker, we have no reasons to not enable it.

Even if that is not the only issue, my perception is that other supposed ODR violations could be simple issues like this.

My experience is "yes, a lot of false positive ODR violations can be fixed by simple patch". The only problem is that there is too many. Or this may be the reason I feel the current ODR checker is not stable enough.

See above. Would you mind waiting a little for my patch, for us to double check we are taking this step with solid footing?

Or let's take a step back. How about always enabling the ODR checker for decls in the GMF but offering a driver flag "-fskip-odr-check-in-gmf"? Then the users can get better experience and offer more precise issue report. Also we don't need to be in the panic of losing standard conformance.

On the other hand, this does not seem to be a regression, since the whole C++20 modules thing is just coming up and we are talking about a new feature and new users.

I think it is saying the users converting a repo from headers to modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need me to send this? I feel this is the deepest problem in the discussion.

Sure, we can move that part of the discussion over there.

Is there an issue report?

Not on the llvm issue tracker yet, I am working on getting issue + patch up soon.

To me, the issue is the straw that broke the camel's back. I've seen too many issue reports saying and complaining the false positive ODR violations publicly and privately. It is a long battle.

How are we tracking these issues, is there a special tag for them? We could take a better look, attack this systematically.

My experience is "yes, a lot of false positive ODR violations can be fixed by simple patch". The only problem is that there is too many. Or this may be the reason I feel the current ODR checker is not stable enough.

Right, but reducing these issues takes a lot of effort. On the other hand, it's possible they all reduce to a few simple cases.
This is my perception so far, working on converting a medium size code base.

Or let's take a step back. How about always enabling the ODR checker for decls in the GMF but offering a driver flag "-fskip-odr-check-in-gmf"? Then the users can get better experience and offer more precise issue report. Also we don't need to be in the panic of losing standard conformance.

Yeah, one concern is losing the bug reports.
While we do offer driver flags to facilitate testing experimental features, which I think C++20 modules falls under,
this may be too technical detail to expose to users.

So I will go ahead and approve this, even though I feel this might have been too hasty, as long as we are running the clang test suite as before, then we can make progress in fixing all the problems and circle back to always enabling the checker.

I think it is saying the users converting a repo from headers to modules.

Right, but that's not a regression.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, we can move that part of the discussion over there.

I'll add you as CC if they agree to do so. I remember there is policy to forbid that. But I am not sure.

Not on the llvm issue tracker yet, I am working on getting issue + patch up soon.

Got it. Not bad. But generally an issue can let us know what's going on and maybe we meet such issues too.

How are we tracking these issues, is there a special tag for them? We could take a better look, attack this systematically.

No. All of them are falling into the clang:modules label. I did a just-in-time search and I found:

(Maybe there are more)

Note that not all of them fails due to bugs in ODR checker. Some of them fails due to bugs in other places and the ODR checker just fires it. Although the result is still false positive ODR violation diagnostics.

And there are another camp about, import-before-include (#61465). From the perspective of implementors, they are not the same thing. But from the perspective of users, it has a same feelings.

Right, but reducing these issues takes a lot of effort. On the other hand, it's possible they all reduce to a few simple cases.
This is my perception so far, working on converting a medium size code base.

Same here.

So I will go ahead and approve this, even though I feel this might have been too hasty, as long as we are running the clang test suite as before, then we can make progress in fixing all the problems and circle back to always enabling the checker.

Thanks. It is always easy to enable the check by default any time.


So in order to get better user experience, save the time checking ODR and keep consistent
behavior with MSVC, we disabled the ODR check for the declarations in the global module
fragment by default. Users who want more strict check can still use the
``-Xclang -fno-skip-odr-check-in-gmf`` flag to get the ODR check enabled. It is also
encouraged to report issues if users find false positive ODR violations or false negative ODR
violations with the flag enabled.
mizvekov marked this conversation as resolved.
Show resolved Hide resolved

ABI Impacts
-----------

Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Basic/LangOptions.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ LANGOPT(MathErrno , 1, 1, "errno in math functions")
BENIGN_LANGOPT(HeinousExtensions , 1, 0, "extensions that we really don't like and may be ripped out at any time")
LANGOPT(Modules , 1, 0, "modules semantics")
COMPATIBLE_LANGOPT(CPlusPlusModules, 1, 0, "C++ modules syntax")
LANGOPT(SkipODRCheckInGMF, 1, 0, "Skip ODR checks for decls in the global module fragment")
LANGOPT(BuiltinHeadersInSystemModules, 1, 0, "builtin headers belong to system modules, and _Builtin_ modules are ignored for cstdlib headers")
BENIGN_ENUM_LANGOPT(CompilingModule, CompilingModuleKind, 3, CMK_None,
"compiling a module interface")
Expand Down
8 changes: 8 additions & 0 deletions clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2985,6 +2985,14 @@ def fmodule_output : Flag<["-"], "fmodule-output">, Flags<[NoXarchOption]>,
Visibility<[ClangOption, CC1Option]>,
HelpText<"Save intermediate module file results when compiling a standard C++ module unit.">;

defm skip_odr_check_in_gmf : BoolOption<"f", "skip-odr-check-in-gmf",
LangOpts<"SkipODRCheckInGMF">, DefaultFalse,
PosFlag<SetTrue, [], [CC1Option],
"Skip ODR checks for decls in the global module fragment.">,
NegFlag<SetFalse, [], [CC1Option],
"Perform ODR checks for decls in the global module fragment.">>,
Group<f_Group>;

def fmodules_prune_interval : Joined<["-"], "fmodules-prune-interval=">, Group<i_Group>,
Visibility<[ClangOption, CC1Option]>, MetaVarName<"<seconds>">,
HelpText<"Specify the interval (in seconds) between attempts to prune the module cache">,
Expand Down
6 changes: 4 additions & 2 deletions clang/include/clang/Serialization/ASTReader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2452,8 +2452,10 @@ class BitsUnpacker {
uint32_t CurrentBitsIndex = ~0;
};

inline bool isFromExplicitGMF(const Decl *D) {
return D->getOwningModule() && D->getOwningModule()->isExplicitGlobalModule();
inline bool shouldSkipCheckingODR(const Decl *D) {
return D->getOwningModule() &&
D->getASTContext().getLangOpts().SkipODRCheckInGMF &&
D->getOwningModule()->isExplicitGlobalModule();
}

} // namespace clang
Expand Down
8 changes: 8 additions & 0 deletions clang/lib/Driver/ToolChains/Clang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3940,6 +3940,14 @@ static bool RenderModulesOptions(Compilation &C, const Driver &D,
Args.ClaimAllArgs(options::OPT_fmodules_disable_diagnostic_validation);
}

// Don't check ODR violations for decls in the global module fragment.
// 1. To keep consistent behavior with MSVC, which don't check ODR violations
// in the global module fragment too.
// 2. Give users better using experience since most issue reports complains
// the false positive ODR violations diagnostic and the true positive ODR
// violations are rarely reported.
CmdArgs.push_back("-fskip-odr-check-in-gmf");
ChuanqiXu9 marked this conversation as resolved.
Show resolved Hide resolved

// Claim `-fmodule-output` and `-fmodule-output=` to avoid unused warnings.
Args.ClaimAllArgs(options::OPT_fmodule_output);
Args.ClaimAllArgs(options::OPT_fmodule_output_EQ);
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Serialization/ASTReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9749,7 +9749,7 @@ void ASTReader::finishPendingActions() {
!NonConstDefn->isLateTemplateParsed() &&
// We only perform ODR checks for decls not in the explicit
// global module fragment.
!isFromExplicitGMF(FD) &&
!shouldSkipCheckingODR(FD) &&
FD->getODRHash() != NonConstDefn->getODRHash()) {
if (!isa<CXXMethodDecl>(FD)) {
PendingFunctionOdrMergeFailures[FD].push_back(NonConstDefn);
Expand Down
17 changes: 9 additions & 8 deletions clang/lib/Serialization/ASTReaderDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,7 +804,7 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
ED->setScopedUsingClassTag(EnumDeclBits.getNextBit());
ED->setFixed(EnumDeclBits.getNextBit());

if (!isFromExplicitGMF(ED)) {
if (!shouldSkipCheckingODR(ED)) {
ED->setHasODRHash(true);
ED->ODRHash = Record.readInt();
}
Expand All @@ -831,7 +831,8 @@ 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 (!isFromExplicitGMF(ED) && OldDef->getODRHash() != ED->getODRHash())
if (!shouldSkipCheckingODR(ED) &&
OldDef->getODRHash() != ED->getODRHash())
Reader.PendingEnumOdrMergeFailures[OldDef].push_back(ED);
} else {
OldDef = ED;
Expand Down Expand Up @@ -872,7 +873,7 @@ void ASTDeclReader::VisitRecordDecl(RecordDecl *RD) {
VisitRecordDeclImpl(RD);
// We should only reach here if we're in C/Objective-C. There is no
// global module fragment.
assert(!isFromExplicitGMF(RD));
assert(!shouldSkipCheckingODR(RD));
RD->setODRHash(Record.readInt());

// Maintain the invariant of a redeclaration chain containing only
Expand Down Expand Up @@ -1101,7 +1102,7 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
if (FD->isExplicitlyDefaulted())
FD->setDefaultLoc(readSourceLocation());

if (!isFromExplicitGMF(FD)) {
if (!shouldSkipCheckingODR(FD)) {
FD->ODRHash = Record.readInt();
FD->setHasODRHash(true);
}
Expand Down Expand Up @@ -1981,7 +1982,7 @@ void ASTDeclReader::ReadCXXDefinitionData(
#undef FIELD

// We only perform ODR checks for decls not in GMF.
if (!isFromExplicitGMF(D)) {
if (!shouldSkipCheckingODR(D)) {
// Note: the caller has deserialized the IsLambda bit already.
Data.ODRHash = Record.readInt();
Data.HasODRHash = true;
Expand Down Expand Up @@ -2147,7 +2148,7 @@ void ASTDeclReader::MergeDefinitionData(
}

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

if (D->getODRHash() != MergeDD.ODRHash) {
Expand Down Expand Up @@ -3520,8 +3521,8 @@ ASTDeclReader::FindExistingResult ASTDeclReader::findExisting(NamedDecl *D) {
// FIXME: We should do something similar if we merge two definitions of the
// same template specialization into the same CXXRecordDecl.
auto MergedDCIt = Reader.MergedDeclContexts.find(D->getLexicalDeclContext());
if (MergedDCIt != Reader.MergedDeclContexts.end() && !isFromExplicitGMF(D) &&
MergedDCIt->second == D->getDeclContext())
if (MergedDCIt != Reader.MergedDeclContexts.end() &&
!shouldSkipCheckingODR(D) && MergedDCIt->second == D->getDeclContext())
Reader.PendingOdrMergeChecks.push_back(D);

return FindExistingResult(Reader, D, /*Existing=*/nullptr,
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Serialization/ASTWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6028,7 +6028,7 @@ void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
Record->push_back(DefinitionBits);

// We only perform ODR checks for decls not in GMF.
if (!isFromExplicitGMF(D)) {
if (!shouldSkipCheckingODR(D)) {
// getODRHash will compute the ODRHash if it has not been previously
// computed.
Record->push_back(D->getODRHash());
Expand Down
8 changes: 4 additions & 4 deletions clang/lib/Serialization/ASTWriterDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
Record.push_back(EnumDeclBits);

// We only perform ODR checks for decls not in GMF.
if (!isFromExplicitGMF(D))
if (!shouldSkipCheckingODR(D))
Record.push_back(D->getODRHash());

if (MemberSpecializationInfo *MemberInfo = D->getMemberSpecializationInfo()) {
Expand All @@ -512,7 +512,7 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
!D->isTopLevelDeclInObjCContainer() &&
!CXXRecordDecl::classofKind(D->getKind()) &&
!D->getIntegerTypeSourceInfo() && !D->getMemberSpecializationInfo() &&
!needsAnonymousDeclarationNumber(D) && !isFromExplicitGMF(D) &&
!needsAnonymousDeclarationNumber(D) && !shouldSkipCheckingODR(D) &&
D->getDeclName().getNameKind() == DeclarationName::Identifier)
AbbrevToUse = Writer.getDeclEnumAbbrev();

Expand Down Expand Up @@ -704,7 +704,7 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
Record.AddSourceLocation(D->getDefaultLoc());

// We only perform ODR checks for decls not in GMF.
if (!isFromExplicitGMF(D))
if (!shouldSkipCheckingODR(D))
Record.push_back(D->getODRHash());

if (D->isDefaulted()) {
Expand Down Expand Up @@ -1510,7 +1510,7 @@ void ASTDeclWriter::VisitCXXMethodDecl(CXXMethodDecl *D) {
D->getFirstDecl() == D->getMostRecentDecl() && !D->isInvalidDecl() &&
!D->hasAttrs() && !D->isTopLevelDeclInObjCContainer() &&
D->getDeclName().getNameKind() == DeclarationName::Identifier &&
!isFromExplicitGMF(D) && !D->hasExtInfo() &&
!shouldSkipCheckingODR(D) && !D->hasExtInfo() &&
!D->isExplicitlyDefaulted()) {
if (D->getTemplatedKind() == FunctionDecl::TK_NonTemplate ||
D->getTemplatedKind() == FunctionDecl::TK_FunctionTemplate ||
Expand Down
10 changes: 10 additions & 0 deletions clang/test/Driver/modules-skip-odr-check-in-gmf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// RUN: %clang -std=c++20 -### -c %s 2>&1 | FileCheck %s
// RUN: %clang -std=c++20 -fno-skip-odr-check-in-gmf -### -c %s 2>&1 \
// RUN: | FileCheck %s --check-prefix=UNUSED
// RUN: %clang -std=c++20 -Xclang -fno-skip-odr-check-in-gmf -### -c %s 2>&1 \
// RUN: | FileCheck %s --check-prefix=NO-SKIP

// CHECK: -fskip-odr-check-in-gmf
// UNUSED: warning: argument unused during compilation: '-fno-skip-odr-check-in-gmf'
// UNUSED-NOT: -fno-skip-odr-check-in-gmf
// NO-SKIP: -fskip-odr-check-in-gmf{{.*}}-fno-skip-odr-check-in-gmf
23 changes: 16 additions & 7 deletions clang/test/Modules/concept.cppm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@
// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm
// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -I%t -DDIFFERENT %t/B.cppm -verify
// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -I%t %t/B.cppm -verify
//
// Testing the behavior of `-fskip-odr-check-in-gmf`
// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf %t/A.cppm -emit-module-interface -o %t/A.pcm
// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf -fprebuilt-module-path=%t -I%t \
// RUN: -DDIFFERENT -DSKIP_ODR_CHECK_IN_GMF %t/B.cppm -verify


//--- foo.h
#ifndef FOO_H
Expand Down Expand Up @@ -70,6 +76,16 @@ module;
export module B;
import A;

#ifdef SKIP_ODR_CHECK_IN_GMF
// expected-error@B.cppm:* {{call to object of type '__fn' is ambiguous}}
// expected-note@* 1+{{candidate function}}
#elif defined(DIFFERENT)
// expected-error@foo.h:41 {{'__fn::operator()' from module 'A.<global>' is not present in definition of '__fn' provided earlier}}
// expected-note@* 1+{{declaration of 'operator()' does not match}}
#else
// expected-no-diagnostics
#endif

template <class T>
struct U {
auto operator+(U) { return 0; }
Expand All @@ -87,10 +103,3 @@ void foo() {

__fn{}(U<int>(), U<int>());
}

#ifdef DIFFERENT
// expected-error@B.cppm:* {{call to object of type '__fn' is ambiguous}}
// expected-note@* 1+{{candidate function}}
#else
// expected-no-diagnostics
#endif
18 changes: 15 additions & 3 deletions clang/test/Modules/polluted-operator.cppm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@
//
// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/a.cppm -o %t/a.pcm
// RUN: %clang_cc1 -std=c++20 %t/b.cppm -fprebuilt-module-path=%t -emit-module-interface -o %t/b.pcm -verify
//
// Testing the behavior of `-fskip-odr-check-in-gmf`
// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf -emit-module-interface %t/a.cppm -o \
// RUN: %t/a.pcm
// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf %t/b.cppm -fprebuilt-module-path=%t \
// RUN: -emit-module-interface -DSKIP_ODR_CHECK_IN_GMF -o %t/b.pcm -verify

//--- foo.h

Expand Down Expand Up @@ -46,10 +52,16 @@ module;
export module a;

//--- b.cppm
// This is actually an ODR violation. But given https://github.com/llvm/llvm-project/issues/79240,
// we don't count it as an ODR violation any more.
// expected-no-diagnostics
module;
#include "bar.h"
export module b;
import a;

#ifdef SKIP_ODR_CHECK_IN_GMF
// expected-no-diagnostics
#else
// expected-error@* {{has different definitions in different modules; first difference is defined here found data member '_S_copy_ctor' with an initializer}}
// expected-note@* {{but in 'a.<global>' found data member '_S_copy_ctor' with a different initializer}}
// expected-error@* {{from module 'a.<global>' is not present in definition of 'variant<_Types...>' provided earlier}}
// expected-note@* {{declaration of 'swap' does not match}}
#endif
16 changes: 13 additions & 3 deletions clang/test/Modules/pr76638.cppm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@
// RUN: %clang_cc1 -std=c++20 %t/mod4.cppm -fmodule-file=mod3=%t/mod3.pcm \
// RUN: -fsyntax-only -verify

// Testing the behavior of `-fskip-odr-check-in-gmf`
// RUN: %clang_cc1 -std=c++20 %t/mod3.cppm -fskip-odr-check-in-gmf \
// RUN: -emit-module-interface -o %t/mod3.pcm
// RUN: %clang_cc1 -std=c++20 %t/mod4.cppm -fmodule-file=mod3=%t/mod3.pcm \
// RUN: -fskip-odr-check-in-gmf -DSKIP_ODR_CHECK_IN_GMF -fsyntax-only -verify

//--- size_t.h

extern "C" {
Expand Down Expand Up @@ -57,13 +63,17 @@ export module mod3;
export using std::align_val_t;

//--- mod4.cppm
// This is actually an ODR violation. But given https://github.com/llvm/llvm-project/issues/79240,
// we don't count it as an ODR violation now.
// expected-no-diagnostics
module;
#include "signed_size_t.h"
#include "csize_t"
#include "align.h"
export module mod4;
import mod3;
export using std::align_val_t;

#ifdef SKIP_ODR_CHECK_IN_GMF
// expected-no-diagnostics
#else
// expected-error@align.h:* {{'std::align_val_t' has different definitions in different modules; defined here first difference is enum with specified type 'size_t' (aka 'int')}}
// expected-note@align.h:* {{but in 'mod3.<global>' found enum with specified type 'size_t' (aka 'unsigned int')}}
#endif
56 changes: 56 additions & 0 deletions clang/test/Modules/skip-odr-check-in-gmf.cppm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// RUN: rm -rf %t
// RUN: mkdir -p %t
// RUN: split-file %s %t
//
// Baseline testing to make sure we can detect the ODR violation from the CC1 invocation.
// RUNX: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
// RUNX: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm
// RUNX: %clang_cc1 -std=c++20 %t/test.cc -fprebuilt-module-path=%t -fsyntax-only -verify
//
// Testing that we can ignore the ODR violation from the driver invocation.
// RUN: %clang -std=c++20 %t/a.cppm --precompile -o %t/a.pcm
// RUN: %clang -std=c++20 %t/b.cppm --precompile -o %t/b.pcm
// RUN: %clang -std=c++20 %t/test.cc -fprebuilt-module-path=%t -fsyntax-only -Xclang -verify \
// RUN: -DIGNORE_ODR_VIOLATION
//
// Testing that the driver can require to check the ODR violation.
// RUN: %clang -std=c++20 -Xclang -fno-skip-odr-check-in-gmf %t/a.cppm --precompile -o %t/a.pcm
// RUN: %clang -std=c++20 -Xclang -fno-skip-odr-check-in-gmf %t/b.cppm --precompile -o %t/b.pcm
// RUN: %clang -std=c++20 -Xclang -fno-skip-odr-check-in-gmf %t/test.cc -fprebuilt-module-path=%t \
// RUN: -fsyntax-only -Xclang -verify

//--- func1.h
bool func(int x, int y) {
return true;
}

//--- func2.h
bool func(int x, int y) {
return false;
}

//--- a.cppm
module;
#include "func1.h"
export module a;
export using ::func;

//--- b.cppm
module;
#include "func2.h"
export module b;
export using ::func;

//--- test.cc
import a;
import b;
bool test() {
return func(1, 2);
}

#ifdef IGNORE_ODR_VIOLATION
// expected-no-diagnostics
#else
// expected-error@func2.h:1 {{'func' has different definitions in different modules;}}
// expected-note@func1.h:1 {{but in 'a.<global>' found a different body}}
#endif
Loading