[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[via cfe-commits]

yujincheng08 wrote:

I accidentially found this PR also fixes an issue I encountered.

Before: https://godbolt.org/z/E6cehd3oh

Afeter: https://godbolt.org/z/MMfq84rcK

and the false error was
```
In module 'bar' imported from /app/foo.cc:3:
h.hpp:6:34: error: 'S::s' from module 'bar.' is not present in 
definition of 'S' provided earlier
6 | inline static constexpr char s[]{c...};
  |  ^
h.hpp:6:34: note: declaration of 's' does not match
6 | inline static constexpr char s[]{c...};
  |  ^
1 error generated.
```

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

@@ -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.
+
+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.

ChuanqiXu9 wrote:

> 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:
- https://github.com/llvm/llvm-project/issues/61317
- https://github.com/llvm/llvm-project/issues/60486
- https://github.com/llvm/llvm-project/issues/63595
- https://github.com/llvm/llvm-project/issues/63544
- https://github.com/llvm/llvm-project/issues/62943 

(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` 
(https://github.com/llvm/llvm-project/issues/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.

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov approved this pull request.


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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Matheus Izvekov via cfe-commits]

@@ -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.
+
+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.

mizvekov wrote:

> 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.


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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

@@ -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.
+
+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.

ChuanqiXu9 wrote:

>> 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**.

> 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.


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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Matheus Izvekov via cfe-commits]

@@ -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.
+
+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.

mizvekov wrote:

> 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?

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

@@ -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.
+
+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.

ChuanqiXu9 wrote:

> That is surprising to me, because the issue you linked in the original 
> commit, https://github.com/llvm/llvm-project/issues/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 
https://github.com/llvm/llvm-project/issues/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.


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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Matheus Izvekov via cfe-commits]

@@ -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.
+
+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.

mizvekov wrote:

>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.

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> I'd still idly vote against adding this flag/support - but if other modules 
> contributors feel it's the right thing to do, I won't stand in the way.

Yeah, as @mizvekov said, this is intended to be a transparent change to users. 
(unless the users are testing volunteers, which is highly welcomed)

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

@@ -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.
+
+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.
+
+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.

ChuanqiXu9 wrote:

My thought for writing this paragraph is:
1. The most important reason is to get better user experience. This is the 
major and the most important motivation.
2. Saving compilation time is the benefit we got after making this decision. 
And we can mention it to say something is getting better in some degree and 
under some conditions (the existing ODR checker is not good enough).
3. Keep consistent behavior with MSVC. This is a supporting argument. And we 
can find that this is the least important thing from the order. This shouldn't 
be a blocker if someday we think the ODR checker is good enough and want to 
enable the ODR checker by default.

I can understand your concerning totally. This is not a document for teaching 
developers. This document is for users. I feel it will be better to write the 
document in the current way to make the users to understand what's happening.

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

@@ -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.
+
+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.

ChuanqiXu9 wrote:

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.

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/79959

>From beb1a4b89f941f41a6e220447dcda6d6fc231a0b Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Tue, 30 Jan 2024 15:57:35 +0800
Subject: [PATCH 1/2] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf

Close https://github.com/llvm/llvm-project/issues/79240

Cite the comment from @mizvekov in
//github.com/llvm/llvm-project/issues/79240:

> There are two kinds of bugs / issues relevant here:
>
> Clang bugs that this change hides
> Here we can add a Frontend flag that disables the GMF ODR check, just
> so
> we can keep tracking, testing and fixing these issues.
> The Driver would just always pass that flag.
> We could add that flag in this current issue.
> Bugs in user code:
> I don't think it's worth adding a corresponding Driver flag for
> controlling the above Frontend flag, since we intend it's behavior to
> become default as we fix the problems, and users interested in testing
> the more strict behavior can just use the Frontend flag directly.

This patch follows the suggestion:
- Introduce the CC1 flag `-fskip-odr-check-in-gmf` which is by default
  off,
  so that the every existing test will still be tested with checking ODR
  violations.
- Passing `-fskip-odr-check-in-gmf` in the driver to keep the behavior
  we intended.
- Edit the document to tell the users who are still interested in more
  strict checks can use `-Xclang -fno-skip-odr-check-in-gmf` to get the
  existing behavior.
---
 clang/docs/ReleaseNotes.rst   |  3 +-
 clang/docs/StandardCPlusPlusModules.rst   | 22 
 clang/include/clang/Basic/LangOptions.def |  1 +
 clang/include/clang/Driver/Options.td |  8 +++
 clang/include/clang/Serialization/ASTReader.h |  6 +-
 clang/lib/Driver/ToolChains/Clang.cpp |  8 +++
 clang/lib/Serialization/ASTReader.cpp |  2 +-
 clang/lib/Serialization/ASTReaderDecl.cpp | 17 +++---
 clang/lib/Serialization/ASTWriter.cpp |  2 +-
 clang/lib/Serialization/ASTWriterDecl.cpp |  8 +--
 .../Driver/modules-skip-odr-check-in-gmf.cpp  | 10 
 clang/test/Modules/concept.cppm   | 23 +---
 clang/test/Modules/polluted-operator.cppm | 18 +-
 clang/test/Modules/pr76638.cppm   | 16 +-
 clang/test/Modules/skip-odr-check-in-gmf.cppm | 56 +++
 15 files changed, 170 insertions(+), 30 deletions(-)
 create mode 100644 clang/test/Driver/modules-skip-odr-check-in-gmf.cpp
 create mode 100644 clang/test/Modules/skip-odr-check-in-gmf.cppm

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bac6c7162a45b..74f34eeef65f0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -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 `_).
 
 C++23 Feature Support
diff --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index 4e853990a7338..44870f210cde6 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -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.
+
+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.
+
+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`` 

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Matheus Izvekov via cfe-commits]

@@ -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.
+
+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.
+
+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 wrote:

Again I find this paragraph concerning 3). This jusitifies in terms of MSVC 
conformance, but doesn't limit the behavior to that environment. Also I don't 
think we offer compiler switches that decrease standards conformance in order 
to save build time.

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Matheus Izvekov via cfe-commits]

@@ -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.
+
+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.

mizvekov wrote:

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.



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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Matheus Izvekov via cfe-commits]

@@ -3940,6 +3940,14 @@ static bool RenderModulesOptions(Compilation , const 
Driver ,
 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");

mizvekov wrote:

```suggestion
  // FIXME: We provisionally don't check ODR violations for decls in the global 
module fragment.
  CmdArgs.push_back("-fskip-odr-check-in-gmf");
```

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Matheus Izvekov via cfe-commits]

@@ -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.

mizvekov wrote:

```suggestion
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 can't perform a 
strong
ODR violation check. With the introduction of modules, now the compiler have
the chance to perform ODR violations with language semantics across translation 
units.
```

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov commented:

On the Implementation side, this looks good, just some concerns regarding 
documentation.

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Matheus Izvekov via cfe-commits]

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Matheus Izvekov via cfe-commits]

mizvekov wrote:

@dwblaikie The flag is for our own use within LLVM test suite. With the 
previous change, we were unconditionally weakening standards conformance and 
runtime checking, because there are a bunch of bugs which are believed to be 
mostly harmless. However, if we do that without some sort of switch, we can't 
keep testing and fixing things on our end, so that we can some day revert to 
the correct behavior.

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[David Blaikie via cfe-commits]

dwblaikie wrote:

I'd still idly vote against adding this flag/support - but if other modules 
contributors feel it's the right thing to do, I won't stand in the way.

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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated 
https://github.com/llvm/llvm-project/pull/79959

>From beb1a4b89f941f41a6e220447dcda6d6fc231a0b Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Tue, 30 Jan 2024 15:57:35 +0800
Subject: [PATCH] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf

Close https://github.com/llvm/llvm-project/issues/79240

Cite the comment from @mizvekov in
//github.com/llvm/llvm-project/issues/79240:

> There are two kinds of bugs / issues relevant here:
>
> Clang bugs that this change hides
> Here we can add a Frontend flag that disables the GMF ODR check, just
> so
> we can keep tracking, testing and fixing these issues.
> The Driver would just always pass that flag.
> We could add that flag in this current issue.
> Bugs in user code:
> I don't think it's worth adding a corresponding Driver flag for
> controlling the above Frontend flag, since we intend it's behavior to
> become default as we fix the problems, and users interested in testing
> the more strict behavior can just use the Frontend flag directly.

This patch follows the suggestion:
- Introduce the CC1 flag `-fskip-odr-check-in-gmf` which is by default
  off,
  so that the every existing test will still be tested with checking ODR
  violations.
- Passing `-fskip-odr-check-in-gmf` in the driver to keep the behavior
  we intended.
- Edit the document to tell the users who are still interested in more
  strict checks can use `-Xclang -fno-skip-odr-check-in-gmf` to get the
  existing behavior.
---
 clang/docs/ReleaseNotes.rst   |  3 +-
 clang/docs/StandardCPlusPlusModules.rst   | 22 
 clang/include/clang/Basic/LangOptions.def |  1 +
 clang/include/clang/Driver/Options.td |  8 +++
 clang/include/clang/Serialization/ASTReader.h |  6 +-
 clang/lib/Driver/ToolChains/Clang.cpp |  8 +++
 clang/lib/Serialization/ASTReader.cpp |  2 +-
 clang/lib/Serialization/ASTReaderDecl.cpp | 17 +++---
 clang/lib/Serialization/ASTWriter.cpp |  2 +-
 clang/lib/Serialization/ASTWriterDecl.cpp |  8 +--
 .../Driver/modules-skip-odr-check-in-gmf.cpp  | 10 
 clang/test/Modules/concept.cppm   | 23 +---
 clang/test/Modules/polluted-operator.cppm | 18 +-
 clang/test/Modules/pr76638.cppm   | 16 +-
 clang/test/Modules/skip-odr-check-in-gmf.cppm | 56 +++
 15 files changed, 170 insertions(+), 30 deletions(-)
 create mode 100644 clang/test/Driver/modules-skip-odr-check-in-gmf.cpp
 create mode 100644 clang/test/Modules/skip-odr-check-in-gmf.cppm

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bac6c7162a45b..74f34eeef65f0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -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 `_).
 
 C++23 Feature Support
diff --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index 4e853990a7338..44870f210cde6 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -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.
+
+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.
+
+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 

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[via cfe-commits]

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff dc4483659fc51890fdc732acc66a4dcda6e68047 
046ec7d3e8f509d830e2e6081d697415859811c2 -- 
clang/test/Driver/modules-skip-odr-check-in-gmf.cpp 
clang/test/Modules/skip-odr-check-in-gmf.cppm 
clang/include/clang/Serialization/ASTReader.h 
clang/lib/Driver/ToolChains/Clang.cpp clang/lib/Serialization/ASTReader.cpp 
clang/lib/Serialization/ASTReaderDecl.cpp clang/lib/Serialization/ASTWriter.cpp 
clang/lib/Serialization/ASTWriterDecl.cpp clang/test/Modules/concept.cppm 
clang/test/Modules/polluted-operator.cppm clang/test/Modules/pr76638.cppm
``





View the diff from clang-format here.


``diff
diff --git a/clang/include/clang/Serialization/ASTReader.h 
b/clang/include/clang/Serialization/ASTReader.h
index 0263ebd6b6..cd28226c29 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -2453,7 +2453,8 @@ private:
 };
 
 inline bool shouldSkipCheckingODR(const Decl *D) {
-  return D->getOwningModule() && 
D->getASTContext().getLangOpts().SkipODRCheckInGMF &&
+  return D->getOwningModule() &&
+ D->getASTContext().getLangOpts().SkipODRCheckInGMF &&
  D->getOwningModule()->isExplicitGlobalModule();
 }
 
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp 
b/clang/lib/Serialization/ASTReaderDecl.cpp
index 0fad0b8940..ffba04f287 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -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 (!shouldSkipCheckingODR(ED) && OldDef->getODRHash() != 
ED->getODRHash())
+  if (!shouldSkipCheckingODR(ED) &&
+  OldDef->getODRHash() != ED->getODRHash())
 Reader.PendingEnumOdrMergeFailures[OldDef].push_back(ED);
 } else {
   OldDef = ED;
@@ -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() && 
!shouldSkipCheckingODR(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,

``




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

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)


Changes

Close https://github.com/llvm/llvm-project/issues/79240

Cite the comment from @mizvekov in
//github.com/llvm/llvm-project/issues/79240:

 There are two kinds of bugs / issues relevant here:

 Clang bugs that this change hides
 Here we can add a Frontend flag that disables the GMF ODR check, just
 so
 we can keep tracking, testing and fixing these issues.
 The Driver would just always pass that flag.
 We could add that flag in this current issue.
 Bugs in user code:
 I don't think it's worth adding a corresponding Driver flag for
 controlling the above Frontend flag, since we intend it's behavior to
 become default as we fix the problems, and users interested in testing
 the more strict behavior can just use the Frontend flag directly.

This patch follows the suggestion:
- Introduce the CC1 flag `-fskip-odr-check-in-gmf` which is by default off, so 
that the every existing test will still be tested with checking ODR violations.
- Passing `-fskip-odr-check-in-gmf` in the driver to keep the behavior we 
intended.
- Edit the document to tell the users who are still interested in more strict 
checks can use `-Xclang -fno-skip-odr-check-in-gmf` to get the existing 
behavior.

---
Full diff: https://github.com/llvm/llvm-project/pull/79959.diff


15 Files Affected:

- (modified) clang/docs/ReleaseNotes.rst (+2-1) 
- (modified) clang/docs/StandardCPlusPlusModules.rst (+22) 
- (modified) clang/include/clang/Basic/LangOptions.def (+1) 
- (modified) clang/include/clang/Driver/Options.td (+8) 
- (modified) clang/include/clang/Serialization/ASTReader.h (+3-2) 
- (modified) clang/lib/Driver/ToolChains/Clang.cpp (+8) 
- (modified) clang/lib/Serialization/ASTReader.cpp (+1-1) 
- (modified) clang/lib/Serialization/ASTReaderDecl.cpp (+7-7) 
- (modified) clang/lib/Serialization/ASTWriter.cpp (+1-1) 
- (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+4-4) 
- (added) clang/test/Driver/modules-skip-odr-check-in-gmf.cpp (+10) 
- (modified) clang/test/Modules/concept.cppm (+16-7) 
- (modified) clang/test/Modules/polluted-operator.cppm (+15-3) 
- (modified) clang/test/Modules/pr76638.cppm (+13-3) 
- (added) clang/test/Modules/skip-odr-check-in-gmf.cppm (+56) 


``diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bac6c7162a45b..44344ba330c5d 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -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 use the flag '-Xclang -fno-skip-odr-check-in-gmf'.
   (`#79240 `_).
 
 C++23 Feature Support
diff --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index 4e853990a7338..c3d0c702a44c9 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -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.
+
+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.
+
+So in order to get better user experience, saving 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.
+
 ABI Impacts
 ---
 
diff --git a/clang/include/clang/Basic/LangOptions.def 

[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 created 
https://github.com/llvm/llvm-project/pull/79959

Close https://github.com/llvm/llvm-project/issues/79240

Cite the comment from @mizvekov in
//github.com/llvm/llvm-project/issues/79240:

> There are two kinds of bugs / issues relevant here:
>
> Clang bugs that this change hides
> Here we can add a Frontend flag that disables the GMF ODR check, just
> so
> we can keep tracking, testing and fixing these issues.
> The Driver would just always pass that flag.
> We could add that flag in this current issue.
> Bugs in user code:
> I don't think it's worth adding a corresponding Driver flag for
> controlling the above Frontend flag, since we intend it's behavior to
> become default as we fix the problems, and users interested in testing
> the more strict behavior can just use the Frontend flag directly.

This patch follows the suggestion:
- Introduce the CC1 flag `-fskip-odr-check-in-gmf` which is by default off, so 
that the every existing test will still be tested with checking ODR violations.
- Passing `-fskip-odr-check-in-gmf` in the driver to keep the behavior we 
intended.
- Edit the document to tell the users who are still interested in more strict 
checks can use `-Xclang -fno-skip-odr-check-in-gmf` to get the existing 
behavior.

>From 046ec7d3e8f509d830e2e6081d697415859811c2 Mon Sep 17 00:00:00 2001
From: Chuanqi Xu 
Date: Tue, 30 Jan 2024 15:57:35 +0800
Subject: [PATCH] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf

Close https://github.com/llvm/llvm-project/issues/79240

Cite the comment from @mizvekov in
//github.com/llvm/llvm-project/issues/79240:

> There are two kinds of bugs / issues relevant here:
>
> Clang bugs that this change hides
> Here we can add a Frontend flag that disables the GMF ODR check, just
> so
> we can keep tracking, testing and fixing these issues.
> The Driver would just always pass that flag.
> We could add that flag in this current issue.
> Bugs in user code:
> I don't think it's worth adding a corresponding Driver flag for
> controlling the above Frontend flag, since we intend it's behavior to
> become default as we fix the problems, and users interested in testing
> the more strict behavior can just use the Frontend flag directly.

This patch follows the suggestion:
- Introduce the CC1 flag `-fskip-odr-check-in-gmf` which is by default
  off,
  so that the every existing test will still be tested with checking ODR
  violations.
- Passing `-fskip-odr-check-in-gmf` in the driver to keep the behavior
  we intended.
- Edit the document to tell the users who are still interested in more
  strict checks can use `-Xclang -fno-skip-odr-check-in-gmf` to get the
  existing behavior.
---
 clang/docs/ReleaseNotes.rst   |  3 +-
 clang/docs/StandardCPlusPlusModules.rst   | 22 
 clang/include/clang/Basic/LangOptions.def |  1 +
 clang/include/clang/Driver/Options.td |  8 +++
 clang/include/clang/Serialization/ASTReader.h |  5 +-
 clang/lib/Driver/ToolChains/Clang.cpp |  8 +++
 clang/lib/Serialization/ASTReader.cpp |  2 +-
 clang/lib/Serialization/ASTReaderDecl.cpp | 14 ++---
 clang/lib/Serialization/ASTWriter.cpp |  2 +-
 clang/lib/Serialization/ASTWriterDecl.cpp |  8 +--
 .../Driver/modules-skip-odr-check-in-gmf.cpp  | 10 
 clang/test/Modules/concept.cppm   | 23 +---
 clang/test/Modules/polluted-operator.cppm | 18 +-
 clang/test/Modules/pr76638.cppm   | 16 +-
 clang/test/Modules/skip-odr-check-in-gmf.cppm | 56 +++
 15 files changed, 167 insertions(+), 29 deletions(-)
 create mode 100644 clang/test/Driver/modules-skip-odr-check-in-gmf.cpp
 create mode 100644 clang/test/Modules/skip-odr-check-in-gmf.cppm

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bac6c7162a45b..44344ba330c5d 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -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 use the flag '-Xclang -fno-skip-odr-check-in-gmf'.
   (`#79240 `_).
 
 C++23 Feature Support
diff --git a/clang/docs/StandardCPlusPlusModules.rst 
b/clang/docs/StandardCPlusPlusModules.rst
index 4e853990a7338..c3d0c702a44c9 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -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++