Page MenuHomePhabricator

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

Authored by jyu2 on Apr 19 2019, 11:58 AM.

Details

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

Diff Detail

Event Timeline

jyu2 created this revision.Apr 19 2019, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2019, 11:58 AM
rnk added a comment.Apr 19 2019, 1:01 PM

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 added a comment.Apr 19 2019, 1:15 PM
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 added a comment.Apr 19 2019, 1:19 PM

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 added a comment.Apr 19 2019, 1:51 PM
In D60912#1472987, @rnk 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 updated this revision to Diff 196346.Apr 23 2019, 3:52 PM
jyu2 retitled this revision from MS ABI: handle inline static data member as template static data member to MS ABI: handle inline static data member and inline variable as template static data member.
jyu2 edited the summary of this revision. (Show Details)

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 accepted this revision.Apr 23 2019, 4:16 PM

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

This revision is now accepted and ready to land.Apr 23 2019, 4:16 PM
jyu2 marked an inline comment as done.Apr 25 2019, 10:47 AM
jyu2 added inline comments.
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 closed this revision.Apr 25 2019, 10:48 AM

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