MS ABI: handle inline static data member and inline variable as template static data member

[jyu2]

Summary

MS only run time problem with inline static data member or inline variable.
An inline static data member's initialize function or inline variable's initialize function gets called multiple time.

To fix this, using template static data member’s initialization method instead.
So that inline static data member or inline variable initialize's function can be put into COMDAT group with global being initialized. And also put inline data in the linker directive. So that the function can be called before main.

The bug is report in:
https://bugs.llvm.org/show_bug.cgi?id=37903

[rnk]

Thanks, I think there's another case to handle, though.

lib/CodeGen/CGDeclCXX.cpp

471–473

I think the whole second condition can be simplified to just D->isIinlineSpecified(). We have to consider plain inline global variables, not just static data members, as in:

int f();
inline int gvinit_inline = f();
int useit() { return gvinit_inline ; }

[jyu2]

In D60912#1472983, @rnk wrote:

Thanks, I think there's another case to handle, though.

Hi Reid,
Thank you so much for the review.

Yes, I check the following test case, both gnu and cl call foo twice. Do we want to do differently with that? Thanks. Jennifer
I add MS only, since we are generate same with gnu. Sure remove that should work at run time. But I am not sure if we have abi issues when mix-match object. I will look more about that. Thanks.

bug-37903>g++ x.cpp x1.cpp -std=c++17
bug-37903>./a.out
foo
foo

bug-37903>cat x.h
extern "C" int printf(const char*,...);
int foo();
inline static int aoo = foo(); // C++17 inline variable, thus also a definition
bug-37903>cat x.cpp
#include "x.h"
int foo()
{

printf("foo\n");
return 1;

}
int main()
{
}
bug-37903>cat x1.cpp
#include "x.h"

[rnk]

In D60912#1472986, @jyu2 wrote:

inline static int aoo = foo(); // C++17 inline variable, thus also a definition

This is a static inline global variable, so it technically creates two different globals, so calling foo twice is intended behavior. I think we really want to look at the GVA linkage instead of trying to list all the reasons why something might have weak linkage. Take a look at shouldBeInCOMDAT in CodeGenModule.cpp, I think it has the logic we want.

[jyu2]

In D60912#1472987, @rnk wrote:

In D60912#1472986, @jyu2 wrote:

inline static int aoo = foo(); // C++17 inline variable, thus also a definition

This is a static inline global variable, so it technically creates two different globals, so calling foo twice is intended behavior. I think we really want to look at the GVA linkage instead of trying to list all the reasons why something might have weak linkage. Take a look at shouldBeInCOMDAT in CodeGenModule.cpp, I think it has the logic we want.

Yes, that is good. I will do that way. Thanks.
Jennifer

[jyu2]

Hi Reid,
Thanks for your review and catch what I were missing for inline variable part(only inline static data member were fix in previous patch).
As you suggested, instead using GVA_DiscardableODR to put inline variable's initialization function or inline static member's initialization functions into to COMDAT group with global being initialized.

The GVA_DiscardableODR is for weak linkage of inline variables.

Let me know if you see problems.
Thank you so much!
Thanks.

[rnk]

lgtm

lib/CodeGen/CGDeclCXX.cpp

470–471

I think this can be simplified by removing the isTemplateInstantiation check and then checking for GVA_DiscardableODR or GVA_StrongODR. That will handle the cases of explicit template instantiation that you have below.

It's up to you if you want to implement the simplification. The current code is correct, I don't believe it's possible to create a GVA_StrongODR global variable with a dynamic initializer without template instantiation (phew).

[rnk]

+@hans, per https://bugs.llvm.org/show_bug.cgi?id=37903#c16

[jyu2]

lib/CodeGen/CGDeclCXX.cpp

470–471

Thanks Reid! I also think GVA_StrongODR which is set for template instantiation, but I am not too sure about it. Since we already handle template by calling isTemplateInstantiation, and add GVA_DiscardableODR. I'd like to with this. If we see other problem. We can always add it up.

Thank you so much for your review.

[jyu2]

committed rGc19f4f806972: Fix bug 37903:MS ABI: handle inline static data member and inline variable as… (authored by jyu2).