[RFC] [C++20] [Modules] Relax the ODR check for decls in global module fragment #79240

ChuanqiXu9 · 2024-01-24T02:16:55Z

The false positive ODR violation diagnostic for modules have been an annoying problem for a long time. We've received a lot of issue reports about it and I rarely see the reports saying the mechanism find real ODR violations. (a suspicious one may be #61643).

Then in a private chat with MSVC developer, he told me MSVC don't check ODR violations in the global module fragment. This is a surprise to me and I never thought we can proceed by this. I feel this may be a regression at first. But I feel it might make sense due to:

Like I said before, there are a lot of false positive ODR violation diagnostic while the true positive diagnostics are much less.
On the other hand, given that the things in the global module fragment are mostly from headers, it won't be a regression compared with the era before modules.
It is really hard for the compiler to get ODR Hash correct in the GMF due to the complexity of headers and C++. An example may be [C++20] [Modules] False Positive ODR violation diagnostic due to using inconsistent qualified but the same type #78850. While all of us love standard conforming implementation, sometimes it is really hard to get things right.

So I'd like to take the same strategy with MSVC to not check ODR violations in the global module fragment.

Given the fact that this is actually a regression on the standard conformance, I am not sure if people have different opinions.

llvmbot · 2024-01-24T02:17:11Z

@llvm/issue-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

The false positive ODR violation diagnostic for modules have been an annoying problem for a long time. We've received a lot of issue reports about it and I rarely see the reports saying the mechanism find real ODR violations. (a suspicious one may be https://github.com//issues/61643).

Then in a private chat with MSVC developer, he told me MSVC don't check ODR violations in the global module fragment. This is a surprise to me and I never thought we can proceed by this. I feel this may be a regression at first. But I feel it might make sense due to:

Like I said before, there are a lot of false positive ODR violation diagnostic while the true positive diagnostics are much less.
On the other hand, given that the things in the global module fragment are mostly from headers, it won't be a regression compared with the era before modules.
It is really hard for the compiler to get ODR Hash correct in the GMF due to the complexity of headers and C++. An example may be [C++20] [Modules] False Positive ODR violation diagnostic due to using inconsistent qualified but the same type #78850. While all of us love standard conforming implementation, sometimes it is really to get things right.

So I'd like to take the same strategy with MSVC to not check ODR violations in the global module fragment.

Given the fact that this is actually a regression on the standard conformance, I am not sure if people have different opinions.

Previosly we land 085eae6 to workaround the false positive ODR violations in #76638. However, we decided to not perform ODR checks for decls from GMF in #79240 and we land the corresponding change. So we should be able to remove the workaround now. The original tests get remained.

mizvekov · 2024-01-29T21:55:03Z

This is hiding real bugs, I feel like this is going too far for people who are striving for correctness here.

With this change, this behavior is not even controlled by a flag, so it feels like we are putting this under the rug indefinitely.

cor3ntin · 2024-01-29T22:13:32Z

Putting the behavior behind a flag does sound like a worthy compromise to me.

dwblaikie · 2024-01-30T00:19:12Z

Is anyone signing up to use that flag today, on a significant codebase on an ongoing basis, today? Otherwise I'd be inclined to leave it out until there's such a user signing up. I'd rather not add it speculatively.

dwblaikie · 2024-01-30T00:19:43Z

& might be worth discussing adding the flag in a separate issue, or a PR?

mizvekov · 2024-01-30T01:05:13Z

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.

ChuanqiXu9 · 2024-01-30T03:15:10Z

A frontend only flag looks reasonable. I'll make it later.

@mizvekov

Close llvm#79240 Cite the comment from @mizvekov in //github.com/llvm/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.

@mizvekov

Close #79240 Cite the comment from @mizvekov in //github.com//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.

Close llvm#79240. See the linked issue for details. Given the frequency of issue reporting about false positive ODR checks (I received private issue reports too), I'd like to backport this to 18.x too.

Previosly we land llvm@085eae6 to workaround the false positive ODR violations in llvm#76638. However, we decided to not perform ODR checks for decls from GMF in llvm#79240 and we land the corresponding change. So we should be able to remove the workaround now. The original tests get remained.

@mizvekov

Close llvm#79240 Cite the comment from @mizvekov in //github.com/llvm/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.

ChuanqiXu9 · 2024-02-01T06:56:52Z

I'm trying to backport this to clang18 in #80249

@mizvekov

Close llvm#79240 Cite the comment from @mizvekov in //github.com/llvm/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.

@mizvekov

Close llvm#79240 Cite the comment from @mizvekov in //github.com/llvm/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.

Close llvm#79240. See the linked issue for details. Given the frequency of issue reporting about false positive ODR checks (I received private issue reports too), I'd like to backport this to 18.x too.

Previosly we land llvm@085eae6 to workaround the false positive ODR violations in llvm#76638. However, we decided to not perform ODR checks for decls from GMF in llvm#79240 and we land the corresponding change. So we should be able to remove the workaround now. The original tests get remained.

@mizvekov

Close llvm#79240 Cite the comment from @mizvekov in //github.com/llvm/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.

Close llvm#79240. See the linked issue for details. Given the frequency of issue reporting about false positive ODR checks (I received private issue reports too), I'd like to backport this to 18.x too.

Previosly we land llvm@085eae6 to workaround the false positive ODR violations in llvm#76638. However, we decided to not perform ODR checks for decls from GMF in llvm#79240 and we land the corresponding change. So we should be able to remove the workaround now. The original tests get remained.

@mizvekov

Close llvm#79240 Cite the comment from @mizvekov in //github.com/llvm/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.

Close llvm#79240. See the linked issue for details. Given the frequency of issue reporting about false positive ODR checks (I received private issue reports too), I'd like to backport this to 18.x too.

Previosly we land llvm@085eae6 to workaround the false positive ODR violations in llvm#76638. However, we decided to not perform ODR checks for decls from GMF in llvm#79240 and we land the corresponding change. So we should be able to remove the workaround now. The original tests get remained.

@mizvekov

Close llvm#79240 Cite the comment from @mizvekov in //github.com/llvm/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.

Close llvm#79240. See the linked issue for details. Given the frequency of issue reporting about false positive ODR checks (I received private issue reports too), I'd like to backport this to 18.x too.

Previosly we land llvm@085eae6 to workaround the false positive ODR violations in llvm#76638. However, we decided to not perform ODR checks for decls from GMF in llvm#79240 and we land the corresponding change. So we should be able to remove the workaround now. The original tests get remained.

@mizvekov

Close llvm#79240 Cite the comment from @mizvekov in //github.com/llvm/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.

Close llvm#79240. See the linked issue for details. Given the frequency of issue reporting about false positive ODR checks (I received private issue reports too), I'd like to backport this to 18.x too.

Previosly we land llvm@085eae6 to workaround the false positive ODR violations in llvm#76638. However, we decided to not perform ODR checks for decls from GMF in llvm#79240 and we land the corresponding change. So we should be able to remove the workaround now. The original tests get remained.

@mizvekov

Close llvm#79240 Cite the comment from @mizvekov in //github.com/llvm/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.

Close llvm#79240. See the linked issue for details. Given the frequency of issue reporting about false positive ODR checks (I received private issue reports too), I'd like to backport this to 18.x too.

ChuanqiXu9 added the clang:modules C++20 modules and Clang Header Modules label Jan 24, 2024

ChuanqiXu9 self-assigned this Jan 24, 2024

ChuanqiXu9 closed this as completed in a0b6747 Jan 29, 2024

cor3ntin reopened this Jan 29, 2024

ChuanqiXu9 mentioned this issue Jan 30, 2024

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

Merged

ChuanqiXu9 closed this as completed in #79959 Feb 1, 2024

ChuanqiXu9 mentioned this issue Feb 1, 2024

[backport] [C++20] [Modules] Backport the ability to skip ODR checks in GMF #80249

Merged

ChuanqiXu9 mentioned this issue Mar 11, 2024

release/18.x: backport [C++20] [Moduls] Avoid computing odr hash for functions from comparing constraint expression #84723

Merged

hackjackhack mentioned this issue Mar 26, 2024

Help requested: Identifying higher-priority open issues/PRs hsutter/cppfront#1026

Open

yujincheng08 mentioned this issue Apr 12, 2024

[Bug] ndk clang building release raise error declaration does not match while llvm upstream compiles android/ndk#2013

Closed

spnda mentioned this issue Feb 23, 2025

Don't include std after module declaration spnda/fastgltf#86

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] [C++20] [Modules] Relax the ODR check for decls in global module fragment #79240

[RFC] [C++20] [Modules] Relax the ODR check for decls in global module fragment #79240

ChuanqiXu9 commented Jan 24, 2024 •

edited

Loading

llvmbot commented Jan 24, 2024

mizvekov commented Jan 29, 2024

cor3ntin commented Jan 29, 2024

dwblaikie commented Jan 30, 2024

dwblaikie commented Jan 30, 2024

mizvekov commented Jan 30, 2024

ChuanqiXu9 commented Jan 30, 2024 •

edited

Loading

ChuanqiXu9 commented Feb 1, 2024

[RFC] [C++20] [Modules] Relax the ODR check for decls in global module fragment #79240

[RFC] [C++20] [Modules] Relax the ODR check for decls in global module fragment #79240

Comments

ChuanqiXu9 commented Jan 24, 2024 • edited Loading

llvmbot commented Jan 24, 2024

mizvekov commented Jan 29, 2024

cor3ntin commented Jan 29, 2024

dwblaikie commented Jan 30, 2024

dwblaikie commented Jan 30, 2024

mizvekov commented Jan 30, 2024

ChuanqiXu9 commented Jan 30, 2024 • edited Loading

ChuanqiXu9 commented Feb 1, 2024

ChuanqiXu9 commented Jan 24, 2024 •

edited

Loading

ChuanqiXu9 commented Jan 30, 2024 •

edited

Loading