ChuanqiXu added inline comments.
================ Comment at: clang/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp:5 // CHECK-DAG: @extern_var_exported = external {{(dso_local )?}}global -// CHECK-DAG: @inline_var_exported = linkonce_odr {{(dso_local )?}}global +// CHECK-DAG: @inline_var_exported = available_externally {{(dso_local )?}}global // CHECK-DAG: @const_var_exported = available_externally {{(dso_local )?}}constant i32 3, ---------------- urnathan wrote: > ChuanqiXu wrote: > > urnathan wrote: > > > I don;t think this is correct. That should still be a linkonce odr, > > > otherwise you'll get conflicts with other module implementation units. > > It is still linkonce_odr in the module it get defined. See the new added > > test case: inline-variable-in-module.cpp for example. The attribute > > `available_externally` is equivalent to external from the perspective of > > linker. See https://llvm.org/docs/LangRef.html#linkage-types. According to > > [dcl.inline]p7, inline variable attached to named module should be defined > > in that domain. Note that the variable attached to global module fragment > > and private module fragment shouldn't be accessed outside the module, so it > > implies that all the variable defined in the module could only be defined > > in the module unit itself. > There's a couple of issues with this. module.cppm is emitting a (linkonce) > definition of inlne_var_exported, but only because it itself is ODR-using > that variable. If you take out the ODR-use in noninline_exported, there is > no longer a symbol emitted. > > But, even if you forced inline vars to be emitted in their defining-module's > interface unit, that would be an ABI change. inline vars are emitted > whereever ODR-used. They have no fixed home TU. Now, we could alter the ABI > and allow interface units to define a home location for inline vars and > similar entities (eg, vtables for keyless classes). But we'd need buy-in > from other compilers to do that. > > FWIW such a discussion did come up early in implementing modules-ts, but we > decided there was enough going on just getting the TS implemented. I'm fine > with revisiting that, but it is a more significant change. > > And it wouldn't apply to (eg) templated variables, which of course could be > instantiated anywhere. Oh, now the key point here is what the correct behavior is instead of the patch. Let's discuss it first. According to [[ http://eel.is/c++draft/dcl.inline#7 | [dcl.inline]p7 ]], > If an inline function or variable that is attached to a named module is > declared in a definition domain, it shall be defined in that domain. I think the intention of the sentence is to define inline variable in the module interface. So if it is required by the standard, I think other compiler need to follow up. As I described in the summary, it might be a difference between C++20 module and ModuleTS. Do you think it is necessary to send the question to WG21? (I get the behavior from reading the words. Maybe I misread or the word is not intentional). Maybe the ABI standard group need to discuss what the linkage should be. Now it may be weak_odr or linkonce_odr. It depends on how we compile the file. If we compile the .cppm file directly, it would be linkonce_odr. And if we compile it to *.pcm file first, it would be weak_odr. I have registered an issue for this: https://github.com/llvm/llvm-project/issues/53838. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119409/new/ https://reviews.llvm.org/D119409 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits