[C++20] [Modules] False Positive ODR violation diagnostic due to using inconsistent qualified but the same type #78850

ashley-hawkins · 2024-01-20T15:03:27Z

Clang versions I tested with, from https://apt.llvm.org/ (taken from --version):

Debian clang version 18.0.0 (++20240119100743+cd05ade13a66-1~exp1~20240119220916.1830) cd05ade
Debian clang version 17.0.6 (5)

Current clang version on compiler explorer: clang version 18.0.0git (https://github.com/llvm/llvm-project.git 4684507)

I'm still not sure whether this is a problem with libstdc++ or a problem with clang so I apologise if this is the wrong place to report, but I get this error when including just <chrono> in one module partition and <memory> in another, here is an example on Compiler Explorer: https://godbolt.org/z/Wvv1h5aYa

module.cc

module;
#include <chrono>
export module repro;
export import :part;

part.cc

module;
#include <memory>
export module repro:part;

In this example I am getting the error 'std::align' has different definitions in different modules

full compiler output

In module 'repro:part' imported from /app/module.cc:4:
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/14.0.1/../../../../include/c++/14.0.1/bits/align.h:61:1: error: 'std::align' has different definitions in different modules; definition in module 'repro:part.<global>' first difference is function body
   60 | inline void*
      | ~~~~~~~~~~~~
   61 | align(size_t __align, size_t __size, void*& __ptr, size_t& __space) noexcept
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   62 | {
      | ~
   63 |   if (__space < __size)
      |   ~~~~~~~~~~~~~~~~~~~~~
   64 |     return nullptr;
      |     ~~~~~~~~~~~~~~~
   65 |   const auto __intptr = reinterpret_cast<uintptr_t>(__ptr);
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   66 |   const auto __aligned = (__intptr - 1u + __align) & -__align;
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   67 |   const auto __diff = __aligned - __intptr;
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   68 |   if (__diff > (__space - __size))
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   69 |     return nullptr;
      |     ~~~~~~~~~~~~~~~
   70 |   else
      |   ~~~~
   71 |     {
      |     ~
   72 |       __space -= __diff;
      |       ~~~~~~~~~~~~~~~~~~
   73 |       return __ptr = reinterpret_cast<void*>(__aligned);
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   74 |     }
      |     ~
   75 | }
      | ~
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/14.0.1/../../../../include/c++/14.0.1/bits/align.h:61:1: note: but in '' found a different body
   60 | inline void*
      | ~~~~~~~~~~~~
   61 | align(size_t __align, size_t __size, void*& __ptr, size_t& __space) noexcept
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   62 | {
      | ~
   63 |   if (__space < __size)
      |   ~~~~~~~~~~~~~~~~~~~~~
   64 |     return nullptr;
      |     ~~~~~~~~~~~~~~~
   65 |   const auto __intptr = reinterpret_cast<uintptr_t>(__ptr);
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   66 |   const auto __aligned = (__intptr - 1u + __align) & -__align;
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   67 |   const auto __diff = __aligned - __intptr;
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   68 |   if (__diff > (__space - __size))
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   69 |     return nullptr;
      |     ~~~~~~~~~~~~~~~
   70 |   else
      |   ~~~~
   71 |     {
      |     ~
   72 |       __space -= __diff;
      |       ~~~~~~~~~~~~~~~~~~
   73 |       return __ptr = reinterpret_cast<void*>(__aligned);
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   74 |     }
      |     ~
   75 | }
      | ~
1 error generated.

I looked at the preprocessed source and saw that the problem causing the function definitions to be considered different was one of them was getting uintptr_t from the global namespace, the other was getting uintptr_t from a using in the std namespace.

I could reproduce the same situation without those included headers to demonstrate the problem:

module.cc

module;
typedef long T;
namespace ns {
    using ::T;
}
namespace ns {
inline void fun() {
    (void)(T)0;
}
}
export module repro;
export import :part;

part.cc

module;
typedef long T;
namespace ns {
inline void fun() {
    (void)(T)0;
}
}
export module repro:part;

CMakeLists.txt

cmake_minimum_required(VERSION 3.28)
project(repro)

set(CMAKE_CXX_STANDARD 20)

add_library(repro)
target_sources(repro PUBLIC FILE_SET CXX_MODULES FILES module.cc part.cc)

And here is the above on compiler explorer: https://godbolt.org/z/Ke6eoGnan and the compiler output from that compiler explorer example:

full compiler output

In file included from /app/module.cc:12:
part.cc:4:13: error: 'ns::fun' has different definitions in different modules; definition in module 'repro:part.<global>' first difference is function body
    4 | inline void fun() {
      | ~~~~~~~~~~~~^~~~~~~
    5 |     (void)(T)0;
      |     ~~~~~~~~~~~
    6 | }
      | ~
module.cc:7:13: note: but in '' found a different body
    7 | inline void fun() {
      | ~~~~~~~~~~~~^~~~~~~
    8 |     (void)(T)0;
      |     ~~~~~~~~~~~
    9 | }
      | ~
1 error generated.

The text was updated successfully, but these errors were encountered:

llvmbot · 2024-01-20T16:09:42Z

@llvm/issue-subscribers-clang-modules

Author: None (ashley-hawkins)

Clang versions I tested with, from <https://apt.llvm.org/> (taken from `--version`): - `Debian clang version 18.0.0 (++20240119100743+cd05ade13a66-1~exp1~20240119220916.1830)` cd05ade - `Debian clang version 17.0.6 (5)`

Current clang version on compiler explorer: clang version 18.0.0git (https://github.com/llvm/llvm-project.git 4684507)

I'm still not sure whether this is a problem with libstdc++ or a problem with clang so I apologise if this is the wrong place to report, but I get this error when including just <chrono> in one module partition and <memory> in another, here is an example on Compiler Explorer: https://godbolt.org/z/Wvv1h5aYa
<details>
<summary>module.cc</summary>

module;
#include &lt;chrono&gt;
export module repro;
export import :part;

</details>
<details>
<summary>part.cc</summary>

module;
#include &lt;memory&gt;
export module repro:part;

</details>

In this example I am getting the error 'std::align' has different definitions in different modules
<details>
<summary>full compiler output</summary>

In module 'repro:part' imported from /app/module.cc:4:
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/14.0.1/../../../../include/c++/14.0.1/bits/align.h:61:1: error: 'std::align' has different definitions in different modules; definition in module 'repro:part.&lt;global&gt;' first difference is function body
   60 | inline void*
      | ~~~~~~~~~~~~
   61 | align(size_t __align, size_t __size, void*&amp; __ptr, size_t&amp; __space) noexcept
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   62 | {
      | ~
   63 |   if (__space &lt; __size)
      |   ~~~~~~~~~~~~~~~~~~~~~
   64 |     return nullptr;
      |     ~~~~~~~~~~~~~~~
   65 |   const auto __intptr = reinterpret_cast&lt;uintptr_t&gt;(__ptr);
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   66 |   const auto __aligned = (__intptr - 1u + __align) &amp; -__align;
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   67 |   const auto __diff = __aligned - __intptr;
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   68 |   if (__diff &gt; (__space - __size))
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   69 |     return nullptr;
      |     ~~~~~~~~~~~~~~~
   70 |   else
      |   ~~~~
   71 |     {
      |     ~
   72 |       __space -= __diff;
      |       ~~~~~~~~~~~~~~~~~~
   73 |       return __ptr = reinterpret_cast&lt;void*&gt;(__aligned);
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   74 |     }
      |     ~
   75 | }
      | ~
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/14.0.1/../../../../include/c++/14.0.1/bits/align.h:61:1: note: but in '' found a different body
   60 | inline void*
      | ~~~~~~~~~~~~
   61 | align(size_t __align, size_t __size, void*&amp; __ptr, size_t&amp; __space) noexcept
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   62 | {
      | ~
   63 |   if (__space &lt; __size)
      |   ~~~~~~~~~~~~~~~~~~~~~
   64 |     return nullptr;
      |     ~~~~~~~~~~~~~~~
   65 |   const auto __intptr = reinterpret_cast&lt;uintptr_t&gt;(__ptr);
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   66 |   const auto __aligned = (__intptr - 1u + __align) &amp; -__align;
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   67 |   const auto __diff = __aligned - __intptr;
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   68 |   if (__diff &gt; (__space - __size))
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   69 |     return nullptr;
      |     ~~~~~~~~~~~~~~~
   70 |   else
      |   ~~~~
   71 |     {
      |     ~
   72 |       __space -= __diff;
      |       ~~~~~~~~~~~~~~~~~~
   73 |       return __ptr = reinterpret_cast&lt;void*&gt;(__aligned);
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   74 |     }
      |     ~
   75 | }
      | ~
1 error generated.

</details>

I looked at the preprocessed source and saw that the problem causing the function definitions to be considered different was one of them was getting uintptr_t from the global namespace, the other was getting uintptr_t from a using in the std namespace.

I could reproduce the same situation without those included headers to demonstrate the problem:
<details>
<summary>module.cc</summary>

module;
typedef long T;
namespace ns {
    using ::T;
}
namespace ns {
inline void fun() {
    (void)(T)0;
}
}
export module repro;
export import :part;

</details>
<details>
<summary>part.cc</summary>

module;
typedef long T;
namespace ns {
inline void fun() {
    (void)(T)0;
}
}
export module repro:part;

</details>
<details>
<summary>CMakeLists.txt</summary>

cmake_minimum_required(VERSION 3.28)
project(repro)

set(CMAKE_CXX_STANDARD 20)

add_library(repro)
target_sources(repro PUBLIC FILE_SET CXX_MODULES FILES module.cc part.cc)

</details>

And here is the above on compiler explorer: https://godbolt.org/z/Ke6eoGnan and the compiler output from that compiler explorer example:
<details>
<summary>full compiler output</summary>

In file included from /app/module.cc:12:
part.cc:4:13: error: 'ns::fun' has different definitions in different modules; definition in module 'repro:part.&lt;global&gt;' first difference is function body
    4 | inline void fun() {
      | ~~~~~~~~~~~~^~~~~~~
    5 |     (void)(T)0;
      |     ~~~~~~~~~~~
    6 | }
      | ~
module.cc:7:13: note: but in '' found a different body
    7 | inline void fun() {
      | ~~~~~~~~~~~~^~~~~~~
    8 |     (void)(T)0;
      |     ~~~~~~~~~~~
    9 | }
      | ~
1 error generated.

</details>

dwblaikie · 2024-01-20T17:27:00Z

Hmm, seems like the same as #76638, which claims to be fixed - but this repro shows the failure on godbolt at ToT... so, not sure.

ChuanqiXu9 · 2024-01-22T01:55:18Z

Hmm, seems like the same as #76638, which claims to be fixed - but this repro shows the failure on godbolt at ToT... so, not sure.

#76638 is a simple workaround for the enum type only. And this issue has the same fundamental reason with #76638 but its direct issue is not the same with #76638. I commented this in #76638. I don't have an idea about how to proceed now : (

Changed Things: - Mentioning we need to specify BMIs for indirectly dependent BMIs too. - Remove the note for `delayed-template-parsing` since #61068 got closed. - Add a note for #78850 since we've seen it for a lot of times. - Add a note for #78173 since we've seen it for a lot of times.

dwblaikie · 2024-01-22T15:11:51Z

First thing would be to establish whether the example is conforming, I guess?

If the alias had a different name it'd obviously be non-conforming/ODR violation, right? (because definitions must have the same sequence of tokens)

The (however it's phrased in the spec) "lookups must find the same names" - if that applies to aliases (like you must find the same alias) - then the code is non-conforming, I think? If it applies to the underlying entity (making the alias transparent) then perhaps the code is conforming and we need to canonicalize earlier in some way.

ChuanqiXu9 · 2024-01-23T02:18:07Z

First thing would be to establish whether the example is conforming, I guess?

If the alias had a different name it'd obviously be non-conforming/ODR violation, right? (because definitions must have the same sequence of tokens)

Yes.

The (however it's phrased in the spec) "lookups must find the same names" - if that applies to aliases (like you must find the same alias) - then the code is non-conforming, I think? If it applies to the underlying entity (making the alias transparent) then perhaps the code is conforming and we need to canonicalize earlier in some way.

It should be the later case according to the comment of https://reviews.llvm.org/D156210#4540980

ChuanqiXu9 · 2024-01-24T02:18:54Z

The reproducer can be workaround by #79240. However, I suspect the same issue exist with the declarations in the module purview. But given there is almost no legacy code in module purview, it won't be so hurting any more.

ChuanqiXu9 · 2024-01-29T03:50:32Z

@ashley-hawkins I've landed #79240. Could you try to test your original workload?

ashley-hawkins · 2024-01-29T21:26:25Z

@ChuanqiXu9 I have tested with the development build, and the issue no longer occurs. Many thanks.

ChuanqiXu9 · 2024-01-30T02:51:17Z

@ChuanqiXu9 I have tested with the development build, and the issue no longer occurs. Many thanks.

Thanks for your high quality reporting too. It is important for us to keep forward.

BTW, if you're making a toy and want to get a feeling about modules, it is suggested to use std modules from https://libcxx.llvm.org/Modules.html. Or if you still want to use libstdc++, it is suggested to mock a std module for that by yourself.

mizvekov · 2024-02-01T21:41:57Z

@ashley-hawkins There is some chance the underlying failure will be fixed in #80245

Just based on the fact your own reduction zeroed in on a using declaration, and the above patch fixes an issue with modules which export using declarations that shadow other declarations.

I think it's possible the non-reduced repro exposes a false-positive ODR violation, but the reduction went south somewhere and introduced a different problem.

If you agree that's possible, would you mind giving it a try with the above patch?

Now that the ODR checker is disabled by default in the driver, you can either pass -Xclang -fskip-odr-check-in-gmf to the driver, to enable the checker so you can repro the failure again, or if you are testing with the frontend directly, just don't pass that -fskip-odr-check-in-gmf flag to it.

mizvekov · 2024-02-01T21:55:25Z

I am also reopening because it has to be decided if the reduced repro is a real ODR violation or not.

If not, we have to fix the ODR checker.
If yes, problem has to be reported to upstream libstdc++, and either possible targeted workarounds implemented, or we decide we won't support affected libstdc++ version when using modules.

ChuanqiXu9 · 2024-02-02T02:03:32Z

I am also reopening because it has to be decided if the reduced repro is a real ODR violation or not.

If not, we have to fix the ODR checker.

If yes, problem has to be reported to upstream libstdc++, and either possible targeted workarounds implemented, or we decide we won't support affected libstdc++ version when using modules.

I just got reply from Gaby from WG21, who is the one of original designer of modules. He confirmed this is not an ODR violation.

Here is his reply:

The C++ definition of ODR, unfortunately, is not geared towards ensuring debug info implementation is simple or easy. For starters, source locations of ODR-abiding declarations are not required to be the same across translation units. It gets funkier when you look at templates and their specializations.

ashley-hawkins · 2024-02-02T07:15:38Z

@ashley-hawkins There is some chance the underlying failure will be fixed in #80245

Just based on the fact your own reduction zeroed in on a using declaration, and the above patch fixes an issue with modules which export using declarations that shadow other declarations.

I think it's possible the non-reduced repro exposes a false-positive ODR violation, but the reduction went south somewhere and introduced a different problem.

If you agree that's possible, would you mind giving it a try with the above patch?

Now that the ODR checker is disabled by default in the driver, you can either pass -Xclang -fskip-odr-check-in-gmf to the driver, to enable the checker so you can repro the failure again, or if you are testing with the frontend directly, just don't pass that -fskip-odr-check-in-gmf flag to it.

@mizvekov I have tested the patch, and on libstdc++ 13.2.0 on my original workload, and the non-reduced reproducer with #include <chrono> and #include <memory> in the different modules, with -Xclang -fno-skip-odr-check-in-gmf, still reproduces the same error.

mizvekov · 2024-02-02T07:24:21Z

@ashley-hawkins Thanks for confirming! So we will fix the ODR checker then.

AdityaShantanu · 2024-05-14T17:49:29Z

bump.

sxstd001 · 2024-06-08T15:36:33Z

Is...solved?

github-actions bot added the clang Clang issues not falling into any other category label Jan 20, 2024

EugeneZelenko added clang:modules C++20 modules and Clang Header Modules and removed clang Clang issues not falling into any other category labels Jan 20, 2024

ChuanqiXu9 changed the title ~~C++20 Modules: Clang gives error error: 'std::align' has different definitions in different modules when just including standard headers in two module partitions~~ [C++20] [Modules] False Positive ODR violation diagnostic due to inconsistent type used Jan 22, 2024

ChuanqiXu9 changed the title ~~[C++20] [Modules] False Positive ODR violation diagnostic due to inconsistent type used~~ [C++20] [Modules] False Positive ODR violation diagnostic due to using inconsistent qualified bug the same type Jan 22, 2024

ChuanqiXu9 changed the title ~~[C++20] [Modules] False Positive ODR violation diagnostic due to using inconsistent qualified bug the same type~~ [C++20] [Modules] False Positive ODR violation diagnostic due to using inconsistent qualified but the same type Jan 22, 2024

ChuanqiXu9 self-assigned this Jan 22, 2024

ChuanqiXu9 mentioned this issue Jan 24, 2024

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

Closed

ChuanqiXu9 mentioned this issue Jan 25, 2024

[C++20][Modules] error: 'std::align_val_t' has different definitions in different modules #76638

Closed

ashley-hawkins closed this as completed Jan 29, 2024

mizvekov mentioned this issue Jan 31, 2024

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

Merged

ChuanqiXu9 mentioned this issue Feb 1, 2024

[clang] fix merging of UsingShadowDecl #80245

Merged

mizvekov reopened this Feb 1, 2024

alfaix mentioned this issue Jul 28, 2024

[clangd][modules] Clangd reports ODR violation when including iostream from (global module fragment of) two different modules #100924

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[C++20] [Modules] False Positive ODR violation diagnostic due to using inconsistent qualified but the same type #78850

[C++20] [Modules] False Positive ODR violation diagnostic due to using inconsistent qualified but the same type #78850

ashley-hawkins commented Jan 20, 2024 •

edited

Loading

llvmbot commented Jan 20, 2024

dwblaikie commented Jan 20, 2024

ChuanqiXu9 commented Jan 22, 2024

dwblaikie commented Jan 22, 2024

ChuanqiXu9 commented Jan 23, 2024

ChuanqiXu9 commented Jan 24, 2024

ChuanqiXu9 commented Jan 29, 2024

ashley-hawkins commented Jan 29, 2024

ChuanqiXu9 commented Jan 30, 2024

mizvekov commented Feb 1, 2024 •

edited

Loading

mizvekov commented Feb 1, 2024 •

edited

Loading

ChuanqiXu9 commented Feb 2, 2024

ashley-hawkins commented Feb 2, 2024 •

edited

Loading

mizvekov commented Feb 2, 2024

AdityaShantanu commented May 14, 2024

sxstd001 commented Jun 8, 2024

[C++20] [Modules] False Positive ODR violation diagnostic due to using inconsistent qualified but the same type #78850

[C++20] [Modules] False Positive ODR violation diagnostic due to using inconsistent qualified but the same type #78850

Comments

ashley-hawkins commented Jan 20, 2024 • edited Loading

llvmbot commented Jan 20, 2024

dwblaikie commented Jan 20, 2024

ChuanqiXu9 commented Jan 22, 2024

dwblaikie commented Jan 22, 2024

ChuanqiXu9 commented Jan 23, 2024

ChuanqiXu9 commented Jan 24, 2024

ChuanqiXu9 commented Jan 29, 2024

ashley-hawkins commented Jan 29, 2024

ChuanqiXu9 commented Jan 30, 2024

mizvekov commented Feb 1, 2024 • edited Loading

mizvekov commented Feb 1, 2024 • edited Loading

ChuanqiXu9 commented Feb 2, 2024

ashley-hawkins commented Feb 2, 2024 • edited Loading

mizvekov commented Feb 2, 2024

AdityaShantanu commented May 14, 2024

sxstd001 commented Jun 8, 2024

ashley-hawkins commented Jan 20, 2024 •

edited

Loading

mizvekov commented Feb 1, 2024 •

edited

Loading

mizvekov commented Feb 1, 2024 •

edited

Loading

ashley-hawkins commented Feb 2, 2024 •

edited

Loading