[C++20][Modules] Can't use same legacy header in different modules #60027
Comments
EugeneZelenko
added
clang:modules
|
@llvm/issue-subscribers-clang-modules |
|
hi, the reproducer looks odd to me since that we don't likely meet the problems if we don't import |
|
I have reproduced this issue with libstdc++, so I suspect it's the same problem here. I'll describe my libstdc++ issue:
module;
using uintptr_t = unsigned long;
namespace std {
using ::uintptr_t;
void align() {
uintptr_t x;
}
} // namespace std
export module a;module;
using uintptr_t = unsigned long;
namespace std {
void align() {
uintptr_t x;
}
} // namespace std
export module b;export module c;
import a;
import b;Compile with clang++ -std=c++20 -x c++-module a.cpp --precompile -o a.pcm
clang++ -std=c++20 -x c++-module b.cpp --precompile -o b.pcm
clang++ -std=c++20 -x c++-module c.cpp --precompile -o c.pcm -fmodule-file=a.pcm -fmodule-file=b.pcmOutputs I'm assuming Visual Studio's headers have a similar problem, so to workaround you can include the |
|
Thanks for the minimal reproducer! It helps a lot. I think this is not a real ODR violation. Since http://eel.is/c++draft/basic.def.odr#14.5 said the names inside the functions should refer to the same entity after the lookup. And |
|
This should fall in the category of #61807. |
|
@masx200 @ChuanqiXu9 I have one more reproducer for you... unfortunately it's the modules "Hello World" example direct from LLVM's documentation (https://clang.llvm.org/docs/StandardCPlusPlusModules.html). Now I build the example on my Windows PC with the following commands: The compilation of Impl.cpp fails with this error: My clang is v17.0.1. |
|
The root problems of such issues are the compiler failed to recognize the sameness of declarations in different translation unit. And this is not a single problem but a catagory of problems. And we probably can only fix them one by one unless we'd like to invent a new BMI format. That said, your issue may still exist after the reproducer of the issue itself got fixed. @csimon-bambecksystems So I believe your report worths a standalone issue. Also since I don't have a windows developing environment, it would be much more helpful if you can reduce it so that it doesn't depends on MSSTL. We can achieve this by preprocess the source files and delete things from it. It is pretty touch and time consuming though.. |
I'm pretty confused by this. For Clang Header Modules we already have a fairly robust merging solution. So some of that infrastructure isn't working where it should be working in C++20 modules? |
Consider reopening #61068?
So in order to get that done, I'd propose this plan of action:
|
|
Just one more comment...
No, I don't think it is, in my case. Look again at what the diagnostic says. On one pass, clang sees a function "with no body". This is incorrect, and you can see it in the diagnostic itself: there is an opening brace So there is the root. What happened (that one time, but not the other time) to the function body? Granted there are some mysterious interactions here with standard C++ modules, and with Microsoft. But my gut says the true problems isn't with those. What do you all think? |
I am not super sure but I fixed multiple similar issues for named modules. One problem may be that the named modules don't emit macros. So with header modules, when we see duplicated header includes and we're allowed to skip them (by pragma once and guard-if). But with named modules, we have to handle them explicitly. So we would meet different corner cases... |
Done.
Let's track the issue in that page. I believe these are 2 different issues.
Sadly we don't know such people now.
Yeah, I admit it is pretty confusing and disappointing. One problem with the analysis (to me) is that it is based on a parser's perspective. But the problem may not live in the parser's side but in the serializer's/ODR checker's side. And from my experience, the answer may simply be that we didn't serialize/check ODR properly for some wried grammar. |
Except we can still end up in a case where two modules bring in the same content (because they didn't see each others macros or pragmas) and we have to merge them. Take two modules that both textually include a header, then both those modules (clang header modules) are included/imported into a translation unit. All possible merging is required there, so far as I know. So any time we see a failure to merge/deduplicate in C++20 modules - we should check if it that merging situation already arises (& is handled correctly) in Clang Header Modules and how it's accounted for there. We may be missing some generalization/implementation sharing from there. We don't want to go tracking down/re-fixing all those issues in C++20 modules if we can help it... Do you have a link to one such previous example, perhaps? Then we could compare/contrast the problem and solution to the Clang Header Modules situation and see if there's a generalization we could benefit from. |
Yeah, generally when we fix such issues reported in C++20 modules, we don't add a test in clang header modules. And vice versa. Luckily, the exceptional examples in my memory exists:
Then now I feel it may be true that And from the implementer's perspective to the above issues, I can't find a common point in the above fixes. This is the reason why I think such issue is not a single issue but a catagory of issues. And we can only fix them case by case. |
Is this already fixed? This reproducer works for me with clang 18 (b3d4549). EDIT: This fails with clang 16.0.6. @SasisaDev could you test if this issue persists in HEAD? |
ah, fair enough. Yeah, @zygoloid did do a lot of fixing of ODR stuff when we were rolling out modules at Google - and the things you've noted in the above are features we don't/haven't used at Google yet (mostly concepts) so haven't been robustly tested in clang header modules deployments, most likely. So, yes, for "newer" feature areas, I agree keeping bugs separate/expecting there to be many distinct fixes required seems fair. (the one about a basic function complaining it has a different body, etc - that seems like something I'd have thought would be covered by existing clang header modules support, so is a bit surprising to me) |
Are you using Windows or LInux? If you're using Windows, I think we can close this one now. |
I can confirm that the reproducer from @davidstone compiles without issues on Windows with clang version 17.0.0 (https://github.com/llvm/llvm-project.git 0d3eee3) |
Got it. In this case, I think we can close the issue and we can reopen this if @SasisaDev find this doesn't solve the issue. |
Results in this error
Both headers at one point include
vcruntime_new.hheader.No
using namespace std;used.Obviously, definitions are not different, they're just not removed by clang when building tree.
I used these commands to build
The text was updated successfully, but these errors were encountered: