C++ modules appear to be exceedignly strict with intrinsic headers #98021
Comments
EugeneZelenko
added
clang:modules
|
@llvm/issue-subscribers-clang-modules Author: Chris Elrod (chriselrod)
That is, code using `immintrin.h` tends to fail to compile when using modules while working fine with headers.
I'll try to produce a minimal example in the next few hours.
For now, I have an example using boost_unordered.
When problems showed up in my own code using intrinsics, I could generally fix it by declaring all arguments as variables, and then passing the lvalues to the intriinsic function.
Hello.cxxm: #ifndef USE_HEADERS
module;
#endif
#include <boost/unordered/unordered_flat_map.hpp>
#include <iostream>
#ifndef USE_HEADERS
export module Hello;
export {
#endif
void hello() { std::cout << "Hello World!\n"; }
template <typename K, typename V> using map = boost::unordered_flat_map<K, V>;
#ifndef USE_HEADERS
}
#endifuser.cpp: #ifndef USE_HEADERS
import Hello;
#else
#include "hello.cxxm"
#endif
int main() {
hello();
int x = 0;
long y = 0;
map<int*,long*> m;
m[&x] = &y;
[[maybe_unused]] auto f = m.find(&x);
return 0;
}Compiling with headers: $ clang++ -std=c++23 use.cpp -DUSE_HEADERS -o Hello.out
$ ./Hello.out
Hello World!With modules: $ clang++ -std=c++23 --precompile hello.cxxm -o M-hello.pcm
$ clang++ -std=c++23 use.cpp -fmodule-file=Hello=M-hello.pcm M-hello.pcm -o Hello_mod.outresults in I could file this as a |
|
Here is the minimal example using
#ifndef USE_HEADERS
module;
#endif
#include <concepts>
#include <immintrin.h>
#include <iostream>
#ifndef USE_HEADERS
export module Hello;
export {
#endif
void hello() { std::cout << "Hello World!\n"; }
template <typename T> auto vload128(const T *p) {
if constexpr (std::same_as<T, float>) {
return _mm_loadu_ps(p);
} else if constexpr (std::same_as<T, double>) {
return _mm_loadu_pd(p);
} else {
return _mm_loadu_si128(reinterpret_cast<const __m128i *>(p));
}
}
#ifndef USE_HEADERS
}
#endif
#ifndef USE_HEADERS
import Hello;
#else
#include "hello.cxxm"
#endif
int main() {
hello();
float x[4];
[[maybe_unused]] auto v = vload128(x);
return 0;
}Compiling with headers: $ clang++ -std=c++23 use.cpp -DUSE_HEADERS -o Hello.
out
$ ./Hello.out
Hello World!Compiling with modules: $ clang++ -std=c++23 --precompile hello.cxxm -o M-he
llo.pcm
$ clang++ -std=c++23 use.cpp -fmodule-file=Hello=M-hell
o.pcm M-hello.pcm -o Hello_mod.out
In file included from use.cpp:2:
/home/chriselrod/Documents/progwork/cxx/experiments/modules/hello.cxxm:16:14: error: no matching function for call to '_mm_loadu_ps'
16 | return _mm_loadu_ps(p);
| ^~~~~~~~~~~~
use.cpp:10:29: note: in instantiation of function template specialization 'vload128<float>' requested here
10 | [[maybe_unused]] auto v = vload128(x);
| ^
1 error generated. |
|
using something like template <typename T> auto vload128(const T *p) {
if constexpr (std::same_as<T, float>) {
const float *fp = p;
return _mm_loadu_ps(fp);
} else if constexpr (std::same_as<T, double>) {
const double *dp = p;
return _mm_loadu_pd(dp);
} else {
const __m128i *ip = reinterpret_cast<const __m128i *>(p);
return _mm_loadu_si128(ip);
}
}instead allows it to compile, even though |
|
Another workaround is to declare explicit instantiations of the template within the module that defines it. |
|
I can't reproduce this in my local environment with trunk (0182f51). My standard library is libstdc++ 10.2 (this may not be relevent) in linux. Can you try again with trunk? |
|
I just stumbled across this bug as well, upgrading from a clang trunk from around end of June to this commit: ChuanqiXu9/clangd-for-modules@e583176. I'm also using I managed to bisect it to 91d40ef . Maybe something in that commit inadvertently touched some code related to name lookup or module visibility? It is a bit weird though that the date of that commit is after the report date of this bug... |
|
I managed to work around it for now by re-applying 91d40ef on top of ChuanqiXu9/clangd-for-modules@e583176 . I guess reapplying it on |
|
@ChuanqiXu9 : Sadly, with 3c9e345 I still get the errors when including and ...and some more similar ones. I looked at the differences between the original PR and the relanded version. It turns out if I make bool Decl::isInAnotherModuleUnit() const {
auto *M = getOwningModule();
#if 1
if (!M || !M->isNamedModule())
return false;
#else
if (!M)
return false;
// FIXME or NOTE: maybe we need to be clear about the semantics
// of clang header modules. e.g., if this lives in a clang header
// module included by the current unit, should we return false
// here?
//
// This is clear for header units as the specification says the
// header units live in a synthesised translation unit. So we
// can return false here.
M = M->getTopLevelModule();
if (!M->isNamedModule())
return false;
#endif
return M != getASTContext().getCurrentNamedModule();
}The code between |
|
This is just a shot in the dark as I'm really not familiar with this code, but maybe there should be a check for the global module again? Like this: bool Decl::isInAnotherModuleUnit() const {
auto *M = getOwningModule();
if (!M)
return false;
+ if (M->isGlobalModule())
+ return false;
+
// FIXME or NOTE: maybe we need to be clear about the semantics
// of clang header modules. e.g., if this lives in a clang header
// module included by the current unit, should we return false
// here?
//
// This is clear for header units as the specification says the
// header units live in a synthesised translation unit. So we
// can return false here.
M = M->getTopLevelModule();
if (!M->isNamedModule())
return false;
return M != getASTContext().getCurrentNamedModule();
}Otherwise, Also I'm wondering if any change in |
|
@jiixyj Thanks for the analysis, it is helpful. But the suggested check may not be the case. Since the GMF may be in the current module unit semantically. I'll try if the minimal reproducer from @chriselrod works. If yes I guess I can fix it quickly. But if not, maybe it will be better to get a reproducer from you.
Maybe not. Since a translation unit may not be a module unit. Otherwise we can implement |
|
Here is a minimal reproducer. I used cvise to create this, plus some simplifications by hand. map.cppm: module;
static void fun(long);
template <typename = void> struct a {
a() { fun(load()); }
long load();
};
export module map;
export using map = a<>;map.cpp: Run it like: |
|
For the record, here is the resulting module;
static void _mm_movemask_epi8(long);
template <template <typename> class> struct group15 {
void match_occupied() { _mm_movemask_epi8(load_metadata()); }
long load_metadata();
};
template <typename Group> struct table_core {
table_core() {
for_all_elements([] {});
}
static void match_really_occupied() {
Group pg;
pg.match_occupied();
}
template <typename F> void for_all_elements(F) { match_really_occupied; }
};
template <typename> struct plain_integral;
template <typename, typename, typename, typename>
using table_core_impl = table_core<group15<plain_integral>>;
struct Trans_NS_unordered_unordered_flat_map {
table_core_impl<int, int, int, int> table_;
};
export module map;
export template <typename, typename>
using map = Trans_NS_unordered_unordered_flat_map;...and here the "start" module;
#include <boost/unordered/unordered_flat_map.hpp>
export module map;
export template <typename K, typename V>
using map = boost::unordered_flat_map<K, V>; |
|
I remember calling the intrinsics a red herring somewhere, as I later learned it seems to actually be about Out of curiosity, have you also gotten a lot of linker errors that only show up when using modules, not when using headers? |
|
So if I understand correctly, This is the definition of static __inline__ int __DEFAULT_FN_ATTRS _mm_movemask_epi8(__m128i __a) {
return __builtin_ia32_pmovmskb128((__v16qi)__a);
}
Because this is static inline, this becomes "TU-local" when used in a module unit. In the traditional C++ compilation model this is no problem, as each translation unit gets its own copy of the static inline method. Interestingly, GCC defines their I'm not sure what the exact semantics of Maybe Clang could define their intrinsics to be more "modules friendly", like GCC? Or, maybe Clang could just "make modules work" as an exception for Or could On the other hand, the template<typename Group,typename SizePolicy>
static inline std::size_t size_index_for(std::size_t n)
{
/* n/N+1 == ceil((n+1)/N) (extra +1 for the sentinel) */
return SizePolicy::size_index(n/Group::N+1);
}It should probably be just
Yes, I also got linker errors here and there when trying to debug my issue with boost's unordered_map when including it in a module unit. This is just a guess, but maybe Clang gets confused with all those In #78173 @ChuanqiXu9 also mentioned that Clang's diagnostics when |
|
I can fix the compiler errors if I replace |
|
There is an interesting discussion thread from when GCC decided to convert all their It appears that using edit: Another interesting discussion about |
|
What do you think about something like the following patch to I'm not sure if this is blessed by the standard. But maybe it's OK as the GMF is intended as a compatibility mechanism for headers. diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index fd88b6a74297..103eec3c371f 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -6872,21 +6872,33 @@ void Sema::AddOverloadCandidate(
// Functions with internal linkage are only viable in the same module unit.
if (getLangOpts().CPlusPlusModules && Function->isInAnotherModuleUnit()) {
/// FIXME: Currently, the semantics of linkage in clang is slightly
/// different from the semantics in C++ spec. In C++ spec, only names
/// have linkage. So that all entities of the same should share one
/// linkage. But in clang, different entities of the same could have
/// different linkage.
- NamedDecl *ND = Function;
- if (auto *SpecInfo = Function->getTemplateSpecializationInfo())
+ NamedDecl *ND;
+ bool IsInlineFunctionInGMF;
+ if (auto *SpecInfo = Function->getTemplateSpecializationInfo()) {
ND = SpecInfo->getTemplate();
+ IsInlineFunctionInGMF = false;
+ } else {
+ ND = Function;
+ /// As a special case, don't remove a "static inline" function declared
+ /// in the GMF from the overload set since this is a common pattern in C
+ /// code.
+ IsInlineFunctionInGMF =
+ Function->getOwningModule() &&
+ Function->getOwningModule()->isGlobalModule() &&
+ Function->isInlineSpecified();
+ }
- if (ND->getFormalLinkage() == Linkage::Internal) {
+ if (ND->getFormalLinkage() == Linkage::Internal && !IsInlineFunctionInGMF) {
Candidate.Viable = false;
Candidate.FailureKind = ovl_fail_module_mismatched;
return;
}
}
if (isNonViableMultiVersionOverload(Function)) {
Candidate.Viable = false; |
|
@jiixyj as you mentioned, this is due to the limitations of |
|
I agree, this is more of a hack. I do wonder though why the error only appears when the call to the "map.cppm": module;
static inline void fun(long) {}
template <typename T = long> void a() { fun(T{}); }
export module map;
export using ::a;"map.cpp": import map;
auto m = (a(), 0);If I either:
...the problem goes away. I guess it is like this because dependent names are looked up at the point of template instantiation -- and at that point, the |
Yeah, I think so.
Yeah, according to the codes you cited, it may be the case.
For this reproducer, my feeling is, if we implement the standard strictly, the compiler may not have a chance to look at But this is the reason why I didn't implement the check for such a long time. We have too many static functions in headers in this ecosystem. I fear it will make a lot of things gets broken. |
|
Now I changed my mind slightly. I think you can submit your change as a hack workaround due to the current C++ ecosystem. And it is not conflicting with the change we need to made for intrinsics and give warnings for such leaked TU-locals. |
|
Thank you for your feedback! I'll try to create a PR and to write some tests. It looks like this was discussed in the committee before: cplusplus/nbballot#427 There was one paper, but it wasn't accepted. Still, the committee recognizes the problem and they encourage further work on this. And in Clang it actually works most of the time! In MSVC as well as far as I can see. But finding wording for this that can go into the standard is a hard problem it looks like. edit: correction: MSVC fails this: module;
static inline int fun() { return 0; }
template <typename G = void> int a() { return fun(); }
export module map;
export using ::a;...with: |
This is another topic. It is about header units not the named modules. As far as I remember, I didn't see such disucssion in commitee. |
|
I opened a PR: #104701 I expanded the fix/hack for implicit instantiations of templates with internal linkage, as people write code like that: https://github.com/boostorg/unordered/blob/a39cf60e93ab7ee1782bae4ced211dc9f6eff751/include/boost/unordered/detail/foa/core.hpp#L855 With this change, my usage of Boost's |
In C, it is a common pattern to have `static inline` functions in headers to avoid ODR issues. Currently, when those headers are included in a GMF, the names are not found when two-phase name lookup and ADL is involved. Those names are removed by `Sema::AddOverloadCandidate`. Similarly, in C++, sometimes people use templates with internal linkage in headers. As the GMF was designed to be a transitional mechanism for headers, special case those functions in `Sema::AddOverloadCandidate`. This fixes <#98021>.
…4701) In C, it is a common pattern to have `static inline` functions in headers to avoid ODR issues. Currently, when those headers are included in a GMF, the names are not found when two-phase name lookup and ADL is involved. Those names are removed by `Sema::AddOverloadCandidate`. Similarly, in C++, sometimes people use templates with internal linkage in headers. As the GMF was designed to be a transitional mechanism for headers, special case those functions in `Sema::AddOverloadCandidate`. This fixes <llvm/llvm-project#98021>.
|
I'm finally back to converting one of my projects to using modules, and am hitting this again: I'm on a relatively recent commit of clang++ (this is a roughly 10-day old build of main): |
|
I think this might be related to #119947 . Can you try to remove the |
|
If I understand correctly, it isn't the same problem as #119947. That seems to be two separate template instantiations with different structs named so we should always have the exact same function |
|
Ah, you are right! |
|
I still have the same problem, btw. It's this template here: https://github.com/boostorg/unordered/blob/a64d81a3789b6f5deb8d8569503e6fe129df93a6/include/boost/unordered/detail/foa/core.hpp#L856 I removed the |
|
In my case it's this error that is hit: llvm-project/clang/lib/CodeGen/CodeGenModule.cpp Line 4763 in 2f69111 Here is a dump of the two involved FunctionDecl 0x55d5d74a2ed8 </home/jan/.conan2/p/b/boost92dac37a4f13d/p/include/boost/unordered/detail/foa/core.hpp:855:1, line:859:1> line:855:27 imported in ldgr.balance_sheet.<global> hidden used size_index_for 'std::size_t (std::size_t)' implicit_instantiation static inline
|-TemplateArgument type 'boost::unordered::detail::foa::group15<boost::unordered::detail::foa::plain_integral>'
| `-RecordType 0x55d5ce61bbd0 'boost::unordered::detail::foa::group15<boost::unordered::detail::foa::plain_integral>' imported
| `-ClassTemplateSpecialization 0x55d5ce61b1e8 'group15'
|-TemplateArgument type 'boost::unordered::detail::foa::pow2_size_policy'
| `-RecordType 0x55d5ce65fb10 'boost::unordered::detail::foa::pow2_size_policy' imported
| `-CXXRecord 0x55d5ce65f9f8 'pow2_size_policy'
|-ParmVarDecl 0x55d5d74a3028 <col:42, col:54> col:54 imported in ldgr.balance_sheet hidden used n 'std::size_t':'unsigned long'
`-CompoundStmt 0x55d5d74a3708 <line:856:1, line:859:1>
`-ReturnStmt 0x55d5d74a36f8 <line:858:3, col:45>
`-CallExpr 0x55d5d74a36d0 <col:10, col:45> 'std::size_t':'unsigned long'
|-ImplicitCastExpr 0x55d5d74a36b8 <col:10, col:22> 'std::size_t (*)(std::size_t)' <FunctionToPointerDecay>
| `-DeclRefExpr 0x55d5d74a3618 <col:10, col:22> 'std::size_t (std::size_t)' lvalue CXXMethod 0x55d5d5dd6690 'size_index' 'std::size_t (std::size_t)'
| `-NestedNameSpecifier TypeSpec 'boost::unordered::detail::foa::pow2_size_policy'
`-BinaryOperator 0x55d5d74a35f8 <col:33, col:44> 'unsigned long' '+'
|-BinaryOperator 0x55d5d74a35d8 <col:33, col:42> 'unsigned long' '/'
| |-ImplicitCastExpr 0x55d5d74a35c0 <col:33> 'std::size_t':'unsigned long' <LValueToRValue>
| | `-DeclRefExpr 0x55d5d74a35a0 <col:33> 'std::size_t':'unsigned long' lvalue ParmVar 0x55d5d74a3028 'n' 'std::size_t':'unsigned long'
| `-ImplicitCastExpr 0x55d5d74a3588 <col:35, col:42> 'std::size_t':'unsigned long' <LValueToRValue>
| `-DeclRefExpr 0x55d5d74a34e8 <col:35, col:42> 'const std::size_t':'const unsigned long' lvalue Var 0x55d5cc4c12f8 'N' 'const std::size_t':'const unsigned long' non_odr_use_constant
| `-NestedNameSpecifier TypeSpec 'boost::unordered::detail::foa::group15<boost::unordered::detail::foa::plain_integral>'
`-ImplicitCastExpr 0x55d5d74a34d0 <col:44> 'unsigned long' <IntegralCast>
`-IntegerLiteral 0x55d5d74a34b0 <col:44> 'int' 1
FunctionDecl 0x55d5d5dd5508 </home/jan/.conan2/p/b/boost92dac37a4f13d/p/include/boost/unordered/detail/foa/core.hpp:855:1, line:859:1> line:855:27 imported in ldgr.balance.<global> hidden used size_index_for 'std::size_t (std::size_t)' implicit_instantiation static inline
|-TemplateArgument type 'boost::unordered::detail::foa::group15<boost::unordered::detail::foa::plain_integral>'
| `-RecordType 0x55d5ce61bbd0 'boost::unordered::detail::foa::group15<boost::unordered::detail::foa::plain_integral>' imported
| `-ClassTemplateSpecialization 0x55d5ce61b1e8 'group15'
|-TemplateArgument type 'boost::unordered::detail::foa::pow2_size_policy'
| `-RecordType 0x55d5ce65fb10 'boost::unordered::detail::foa::pow2_size_policy' imported
| `-CXXRecord 0x55d5ce65f9f8 'pow2_size_policy'
|-ParmVarDecl 0x55d5d5dd5658 <col:42, col:54> col:54 imported in ldgr.balance hidden used n 'std::size_t':'unsigned long'
`-CompoundStmt 0x55d5d5dd6868 <line:856:1, line:859:1>
`-ReturnStmt 0x55d5d5dd6858 <line:858:3, col:45>
`-CallExpr 0x55d5d5dd6830 <col:10, col:45> 'std::size_t':'unsigned long'
|-ImplicitCastExpr 0x55d5d5dd6818 <col:10, col:22> 'std::size_t (*)(std::size_t)' <FunctionToPointerDecay>
| `-DeclRefExpr 0x55d5d5dd65e8 <col:10, col:22> 'std::size_t (std::size_t)' lvalue CXXMethod 0x55d5d5dd6690 'size_index' 'std::size_t (std::size_t)'
| `-NestedNameSpecifier TypeSpec 'boost::unordered::detail::foa::pow2_size_policy'
`-BinaryOperator 0x55d5d5dd65c8 <col:33, col:44> 'std::size_t':'unsigned long' '+'
|-BinaryOperator 0x55d5d5dd65a8 <col:33, col:42> 'std::size_t':'unsigned long' '/'
| |-ImplicitCastExpr 0x55d5d5dd6590 <col:33> 'std::size_t':'unsigned long' <LValueToRValue>
| | `-DeclRefExpr 0x55d5d5dd6570 <col:33> 'std::size_t':'unsigned long' lvalue ParmVar 0x55d5d5dd5658 'n' 'std::size_t':'unsigned long'
| `-ImplicitCastExpr 0x55d5d5dd6558 <col:35, col:42> 'std::size_t':'unsigned long' <LValueToRValue>
| `-DeclRefExpr 0x55d5d5dd64c0 <col:35, col:42> 'const std::size_t':'const unsigned long' lvalue Var 0x55d5cc4c12f8 'N' 'const std::size_t':'const unsigned long' non_odr_use_constant
| `-NestedNameSpecifier TypeSpec 'boost::unordered::detail::foa::group15<boost::unordered::detail::foa::plain_integral>'
`-ImplicitCastExpr 0x55d5d5dd64a8 <col:44> 'std::size_t':'unsigned long' <IntegralCast>
`-IntegerLiteral 0x55d5d5dd6488 <col:44> 'int' 1So the |
Removing
I'm also including Ideally, #if __GNUC__ >= 4
#define DLL_LOCAL __attribute__ ((visibility ("hidden")))
#else
#define DLL_LOCAL
#endif
I imagine removing
Thanks for the deep dive. |
|
One hacky (?) fix is to just disable this error for decls from the GMF: diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 3469676b74bc..cb2e1fa1b0e1 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -4746,24 +4746,36 @@ llvm::Constant *CodeGenModule::GetOrCreateLLVMFunction(
// Handle dropped DLL attributes.
if (D && shouldDropDLLAttribute(D, Entry)) {
Entry->setDLLStorageClass(llvm::GlobalValue::DefaultStorageClass);
setDSOLocal(Entry);
}
// If there are two attempts to define the same mangled name, issue an
// error.
if (IsForDefinition && !Entry->isDeclaration()) {
+
+ bool SkipCheck = false;
+ if (D->isFromGlobalModule()) {
+ SkipCheck = true;
+ } else if (const FunctionDecl *FD = cast_or_null<FunctionDecl>(D)) {
+ if (const FunctionTemplateDecl *FunTmpl = FD->getPrimaryTemplate()) {
+ if (FunTmpl->isFromGlobalModule()) {
+ SkipCheck = true;
+ }
+ }
+ }
+
GlobalDecl OtherGD;
// Check that GD is not yet in DiagnosedConflictingDefinitions is required
// to make sure that we issue an error only once.
- if (lookupRepresentativeDecl(MangledName, OtherGD) &&
+ if (!SkipCheck && lookupRepresentativeDecl(MangledName, OtherGD) &&
(GD.getCanonicalDecl().getDecl() !=
OtherGD.getCanonicalDecl().getDecl()) &&
DiagnosedConflictingDefinitions.insert(GD).second) {
getDiags().Report(D->getLocation(), diag::err_duplicate_mangled_name)
<< MangledName;
getDiags().Report(OtherGD.getDecl()->getLocation(),
diag::note_previous_definition);
}
}
With this my program compiles and all tests run successfully. |
I think this worth another issue. |
I feel the CodeGen part is correct. The problem part is the producer. We shouldn't contain the template instantiation including TULocal template parameters. |
That is, code using
immintrin.htends to fail to compile when using modules while working fine with headers.I'll try to produce a minimal example in the next few hours.
For now, I have an example using boost_unordered.
When problems showed up in my own code using intrinsics, I could generally fix it by declaring all arguments as variables, and then passing the lvalues to the intriinsic function.
Hello.cxxm:
user.cpp:
Compiling with headers:
$ clang++ -std=c++23 use.cpp -DUSE_HEADERS -o Hello.out $ ./Hello.out Hello World!With modules:
results in
I could file this as a
boost_unorderedissue or make a PR there, as I've generally found I can work around the problem.But I'll see about creating a minimal reproducer using
#include <immintrin.h>directlry that works with headers but fails with modules.The text was updated successfully, but these errors were encountered: