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
