[C++20] [Module] lambda in initializer of non-inline variable in named modules is internal to the TU incorrectly #110146
Comments
EugeneZelenko
added
clang:modules
|
@llvm/issue-subscribers-clang-modules Author: LEE KYOUNGHEON (stripe2933)
Following code works with Clang 19 + libc++.
import std;
#define FWD(...) static_cast<decltype(__VA_ARGS__)&&>(__VA_ARGS__)
namespace ranges::views {
constexpr auto deref = std::views::transform([](auto &&x) { return *FWD(x); });
}
int main() {
auto opts = { std::optional { 1 }, std::optional { 2 } };
std::println("{}", opts | ranges::views::deref); // [1, 2]
}But this does not:
import std;
import ranges;
int main() {
auto opts = { std::optional { 1 }, std::optional { 2 } };
std::println("{}", opts | ranges::views::deref);
}
export module ranges;
import std;
#define FWD(...) static_cast<decltype(__VA_ARGS__)&&>(__VA_ARGS__)
namespace ranges::views {
export constexpr auto deref = std::views::transform([](auto &&x) { return *FWD(x); });
}Even if I removed |
|
I haven't figured it out completely. But it shows we can workaround it by: |
|
Sadly this tends to be an ABI issue. According to https://itanium-cxx-abi.github.io/cxx-abi/abi.html:
So that according to the Itanium C++ ABI spec, we only set lambda numbering for the cases listed above (including the initializers of inline variables, this is why the above workaround works). However, this is not true for modules clearly. The lambda is not unique to the translation units if they are used in initializers of varaibles in modules. I am not sure if we can change the behavior of the compiler before the above proposed wording get approved in Itanium C++ ABI. Otherwise we can only wait for the action of Itanium C++ group, which may take a longer time. CC @AaronBallman @erichkeane @cor3ntin @mizvekov for the policy issues. |
ChuanqiXu9
changed the title
|
The straight conversion of this example using preprocessor includes, without modules, doesn't work as well, so I tend to think this is not a bug, the user should have used Wouldn't this be an ODR violation anyway, and the real problem here is that we miss diagnosing that? |
What's the example? In the single TU form, https://godbolt.org/z/1zre9eMjb, it works well. Do you mean split the definition of
Then this is why I think this is an ABI issue. If we add a clause to https://itanium-cxx-abi.github.io/cxx-abi/abi.html#closure-types:
it wouldn't be an ODR violation. |
|
I don't see how it relates to ODR. In the given modules case, there is only a single definition, Anyway I think this is something more fundamental. It can be reduced to: And indeed further, to: And now I see that it's the same issue as #59513, and also that you already realized that, @ChuanqiXu9. Anyway I'll leave this reduction here just in case it's useful. |
|
The straight conversion example I mean something like: // --- A.h
constexpr auto lambda = []{};
// --- B.cc
#include "A.h"
// --- C.cc
#include "A.h"Here we have multiple definitions of non-inline I believe the modules example has the same issue. It's exporting the definition of a non-inline variable, which means that if another module imports it, then we are going to end up with multiple TUs defining the same non-inline variable. And non-withstanding the fact that the lambdas still don't correspond. It seems you want to fix the issue by having all variables behave as inline variables? Or maybe just those exported by modules? Should that extend to functions as well? Anyway, that seems like a language evolution issue rather than an ABI issue. |
That issue is an artefact of the textual inclusion model. Modules don't work this way, on the language level at any rate. There is only one definition, in the module, and all that gets exported is the name. Importing TUs simply have access to that name, they don't redefine the entity. |
In the given example, the entity is exported, so how would it have module linkage?
Exporting just the declaration wouldn't work for a constexpr variable, a definition is needed at the point of use. |
Sorry yes my mistake, I was mixing up linkage vs reachability as being orthogonal to
Right, I guess it starts to get tricky with terminology and the precise meaning of "definition", but I'm referring specifically to the language's idea of a definition. While the compiler for sure needs to include such information into the BMI for use on the importing side, as far as I understand that doesn't constitute a definition of the entity (in the ODR sense) in the importing TU. |
No, we don't change the behavior of the variable. The behavior of the variable won't be affected by the proposal. What we changes is the behavior of the lambda: https://itanium-cxx-abi.github.io/cxx-abi/abi.html#closure-types This was defined in the ABI spec and not in the language spec. So I think this is an ABI issue instead of a language evolving issue. And @zygoloid agrees on the direction. I feel good to help to sent this to EWG (or CWG?) if you still have questions. |
…ternal module unit context Close llvm/llvm-project#59513 Close llvm/llvm-project#110146 As we discussed, this is related to ABI: itanium-cxx-abi/cxx-abi#186 I was intending to fix this after it gets merged into the ItaniumC++ABI formally. But it looks like ItaniumC++ABI doesn't update it yet and there are more issue reports for it. Luckily Richard had a clear direction guide here though. So I think it should be good to do this without a formal ItaniumC++ABI wording. The diff of the patch is slightly larger than it was by a simple refacoration to simple the control flow a little bit.
…le unit context Close llvm#59513 Close llvm#110146 As we discussed, this is related to ABI: itanium-cxx-abi/cxx-abi#186 I was intending to fix this after it gets merged into the ItaniumC++ABI formally. But it looks like ItaniumC++ABI doesn't update it yet and there are more issue reports for it. Luckily Richard had a clear direction guide here though. So I think it should be good to do this without a formal ItaniumC++ABI wording. The diff of the patch is slightly larger than it was by a simple refacoration to simple the control flow a little bit.
Following code works with Clang 19 + libc++.
main.cppBut this does not:
main.cppranges.cppmEven if I removed
-fexperimental-modules-reduced-bmiargument, it still throws errors.The text was updated successfully, but these errors were encountered: