[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