urnathan 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,
----------------
ChuanqiXu wrote:
> 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.
> 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).
You are reading more into the std than it says. The std specifies what /source
code/ is meaningful. It says nothing about how a computation system might
represent the program in another form. Most of the latter, for ahead-of-time
translation, is at the discretion of compiler implementors. Part of that is
the domain of the ABI, which specifies an interface to which different
compilers may target, and then have compatibility at the object-file boundary.
> Maybe the ABI standard group need to discuss what the linkage should be.
Correct. And right now there is no consensus to do anything different with such
entities.
The ABI (http://itanium-cxx-abi.github.io/cxx-abi/abi.html) 5.2 documents such
vague-linkage entities. That section would need changes to bless what you are
trying to do.
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
