[PATCH] D119409: [C++20] [Modules] Remain variable's definition in module interface

[Nathan Sidwell via Phabricator via cfe-commits]

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:
> > 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.


================
Comment at: clang/test/CodeGenCXX/static-variable-in-module.cpp:2-8
+// RUN: mkdir %t
+// RUN: echo "struct S { S(); };" >> %t/foo.h
+// RUN: echo "static S s = S();" >> %t/foo.h
+// RUN: %clang -std=c++20 -I%t %s -S -emit-llvm -o - | FileCheck %s
+module;
+#include "foo.h"
+export module m;
----------------
ChuanqiXu wrote:
> ChuanqiXu wrote:
> > urnathan wrote:
> > > rather than generate a foo.h file, why not (ab)use the preprocessor with internal line directives?
> > > 
> > > ```
> > > module;
> > > # 3 __FILE__ 1 // use the next physical line number here (and below)
> > > struct S { S(); };
> > > static S s = S();
> > > # 6 "" 2
> > > export module m;
> > > ...
> > > ```
> > Yeah, the form is useful when we need to add expected-* diagnostic message to GMF. But I feel it is a little bit hacker. I guess the form mimics looks like user more wouldn't be worse personally.
> Ok, I admit this is really helpful and time saving : ) (Now all my new test cases would be wrote in this form)
The other thing you can do, which I've seen elsewhere, is an Input subdirectory and put the #include file there.  I prefer the direct encoding, 'cos that's less indirection when trying to understand the testcase :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119409/new/

https://reviews.llvm.org/D119409