Allow MS extension: support of constexpr with __declspec(dllimport).

[zahiraam]

Summary

Microsoft allows the support of ‘constexpr’ with ‘declspec(dllimport) starting
from VS2017 15.u update 4. See Constexpr doesn't support declspec(dllimport)
in VS2017 15.8 - Visual Studio Feedback.

Let’s consider this example.
lib.cpp:
declspec(dllexport) int val=12;
declspec(dllexport)
int next(int n)
{

return n + 1;

}

app1.cpp:
#include <iostream>
extern int __declspec(dllimport) next(int n);
int main () {

extern int __declspec(dllimport) val;
constexpr int& val_ref = val;
int i = next(val_ref);
std::cout << "i: " << i << std::endl;
return i;

}

Compiling this will give the expected output.
$ cl /LD lib.cpp
$ cl /EHsc app1.cpp /link lib.lib
$ ./app1.exe
i: 13

The Intel compiler has the same behavior than MSVC.
Clang compiles this test case with this error:
error: constexpr variable 'val_ref' must be initialized by a constant

    expression
constexpr int& val_ref = val;
               ^         ~~~

1 error generated.

I think this should be fixed. This patch is doing that.

Now let’s look now at this example (dllimport at TU scope level):
app2.cpp:
#include <iostream>
extern int declspec(dllimport) next(int n);
extern int declspec(dllimport) val;
constexpr int& val_ref = val;

int main () {

int i = next(val_ref);
std::cout << "i: " << i << std::endl;
return i;

}
Compiling this will result into an unresolved symbol:
$ cl /EHsc app2.cpp /link lib.lib
app2.obj : error LNK2001: unresolved external symbol "int val" (?val@@3HA)
app2.exe : fatal error LNK1120: 1 unresolved externals

ICL and clang generate the same error.

These are the symols generated for all 3 compilers:
ICL:
003 00000000 UNDEF notype External | imp_?next@@YAHH@Z (declspec(dllimport) int cdecl next(int))
004 00000000 UNDEF notype External | imp_?val@@3HA (declspec(dllimport) int val)
00C 00000000 SECT4 notype External | ?val_ref@@3AEAHEA (int & val_ref)
00D 00000000 UNDEF notype External | ?val@@3HA (int val)
MSVC:
00A 00000000 UNDEF notype External | imp_?next@@YAHH@Z (declspec(dllimport) int cdecl next(int))
015 00000000 SECT6 notype Static | ?val_ref@@3AEAHEA (int & val_ref)

The symbols generated by ICL seem to be what's expected. MSVC should be generating an "__imp_?val" symbol.

Clang with the change in this patch is generating the expected symbols:
010 00000000 UNDEF notype External | imp_?val@@3HA (declspec(dllimport) int val)
011 00000000 UNDEF notype External | imp_?next@@YAHH@Z (declspec(dllimport) int __cdecl next(int))
012 00000000 SECT5 notype External | ?val_ref@@3AEAHEA (int & val_ref)
013 00000000 UNDEF notype External | ?val@@3HA (int val)

Fixes issue https://github.com/llvm/llvm-project/issues/53182

Differential Revision: https://reviews.llvm.org/D137107

[rnk]

Unless I'm missing something, I think Clang's behavior here is preferable to MSVC's. MSVC produces code which will not link. Clang turns the linker error into a compiler error, which is generally easier for the user to understand. To my knowledge, it is still true that there is no COFF relocation which can statically initialize a global variable with the address of a dllimported symbol.

For the use case you are considering, how do things work out in practice? Is there some new import patching facility that I'm not aware of? Is the variable linked statically instead of dynamically, and then the linker relaxes the __imp references to direct references?

[rnk]

@akhuang, can you help out with this? The address of dllimport symbols not being constexpr is a big pain point for our users too, and I'd love to fix it:
https://github.com/protocolbuffers/protobuf/issues/10159

I am ready to be wrong here: if things have changed and my info is stale, we should definitely adapt.

[zahiraam]

In D137107#3897326, @rnk wrote:

Unless I'm missing something, I think Clang's behavior here is preferable to MSVC's. MSVC produces code which will not link. Clang turns the linker error into a compiler error, which is generally easier for the user to understand. To my knowledge, it is still true that there is no COFF relocation which can statically initialize a global variable with the address of a dllimported symbol.

For the use case you are considering, how do things work out in practice? Is there some new import patching facility that I'm not aware of? Is the variable linked statically instead of dynamically, and then the linker relaxes the __imp references to direct references?

I haven't used any import patch. I just used MSVC 2019 and the test compiles and links. Not sure how the variable is linked.
The symbols generated are:
009 00000000 UNDEF notype External | imp_?next@@YAHH@Z (declspec(dllimport) int cdecl next(int))
012 00000000 UNDEF notype External | imp_?val@@3HA (__declspec(dllimport) int val)
and this is the asm:
main PROC
$LN3:

sub     rsp, 56                                 ; 00000038H

; Line 5

mov     rax, QWORD PTR __imp_?val@@3HA
mov     QWORD PTR val_ref$[rsp], rax

; Line 6

mov     rax, QWORD PTR val_ref$[rsp]
mov     ecx, DWORD PTR [rax]
call    QWORD PTR __imp_?next@@YAHH@Z
mov     DWORD PTR i$[rsp], eax

; Line 8

mov     eax, DWORD PTR i$[rsp]

; Line 9

add     rsp, 56                                 ; 00000038H
ret     0

main ENDP

[mstorsjo]

In D137107#3897326, @rnk wrote:

Unless I'm missing something, I think Clang's behavior here is preferable to MSVC's. MSVC produces code which will not link. Clang turns the linker error into a compiler error, which is generally easier for the user to understand. To my knowledge, it is still true that there is no COFF relocation which can statically initialize a global variable with the address of a dllimported symbol.

For the use case you are considering, how do things work out in practice? Is there some new import patching facility that I'm not aware of? Is the variable linked statically instead of dynamically, and then the linker relaxes the __imp references to direct references?

The testcase that is shown here is peculiar in the sense that constexpr int& val_ref = val; is located within main(), so it isn't really constexpr (as far as I'd undestand it), but MSVC produces a load from __imp_val.

I am ready to be wrong here: if things have changed and my info is stale, we should definitely adapt.

I'm not aware of anything that would have changed here...

For the cases where this needs to be really statically initialized, the mingw pseudo relocations do fill that spot (which fakes it by initializing it very early on, from a linker-generated list of spots in the binary that needs patching). For non-constexpr cases in C++, Clang solves it by implicitly generating a static constructor which fetches it from the dllimported variable.

[zahiraam]

In D137107#3897568, @mstorsjo wrote:

In D137107#3897326, @rnk wrote:

Unless I'm missing something, I think Clang's behavior here is preferable to MSVC's. MSVC produces code which will not link. Clang turns the linker error into a compiler error, which is generally easier for the user to understand. To my knowledge, it is still true that there is no COFF relocation which can statically initialize a global variable with the address of a dllimported symbol.

For the use case you are considering, how do things work out in practice? Is there some new import patching facility that I'm not aware of? Is the variable linked statically instead of dynamically, and then the linker relaxes the __imp references to direct references?

The testcase that is shown here is peculiar in the sense that constexpr int& val_ref = val; is located within main(), so it isn't really constexpr (as far as I'd undestand it), but MSVC produces a load from __imp_val.

I think I can be convinced that generating a compiler error instead of a link error for the first use case is better, but about this use case (I am not sure I understand why you say “it isn’t a real constexpr”):

extern int __declspec(dllimport) next(int n);
int main () {

extern int _declspec(dllimport) val;
constexpr int& val_ref = val;
int i = next(val_ref);
return i;

}

With MSVC with or without constexpr will lead to the same generated symbol

012 00000000 UNDEF notype External | imp_?val@@3HA (declspec(dllimport) int val)

and the same assembly (load __imp_?val) (is that why you made the comment above?). And the test case runs.

But that’s not the case for clang (without this patch).
With constexpr it’s failing with the error mentioned above.

Without constexpr the symbol generated is

0E 00000000 UNDEF notype External | imp_?val@@3HA (declspec(dllimport) int val)

and the assembly generates a ‘load __imp_? Val’. Same than MSVC, but the test case fails to run. @rnk Shouldn't this run?

I am ready to be wrong here: if things have changed and my info is stale, we should definitely adapt.

I'm not aware of anything that would have changed here...

For the cases where this needs to be really statically initialized, the mingw pseudo relocations do fill that spot (which fakes it by initializing it very early on, from a linker-generated list of spots in the binary that needs patching). For non-constexpr cases in C++, Clang solves it by implicitly generating a static constructor which fetches it from the dllimported variable.

[mstorsjo]

In D137107#3905443, @zahiraam wrote:

and the same assembly (load __imp_?val) (is that why you made the comment above?). And the test case runs.

Yes, pretty much - the actual value of the address of &val isn't known at compile time and it cannot be produced in a way to initialize e.g. static memory, which I would believe that one can expect of constexpr. The runtime linker that loads DLLs can't fix it so that the address is available as a statically initialized data, but to use it it does require runtime code to fetch the address.

Without constexpr the symbol generated is

0E 00000000 UNDEF notype External | imp_?val@@3HA (declspec(dllimport) int val)

and the assembly generates a ‘load __imp_? Val’. Same than MSVC, but the test case fails to run. @rnk Shouldn't this run?

Can you give the full exact repro for this case, without constexpr, which you say fails to run - I tried to follow the instructions but for me it seems to work just fine, without constexpr.

[zahiraam]

In D137107#3906413, @mstorsjo wrote:

In D137107#3905443, @zahiraam wrote:

and the same assembly (load __imp_?val) (is that why you made the comment above?). And the test case runs.

Yes, pretty much - the actual value of the address of &val isn't known at compile time and it cannot be produced in a way to initialize e.g. static memory, which I would believe that one can expect of constexpr. The runtime linker that loads DLLs can't fix it so that the address is available as a statically initialized data, but to use it it does require runtime code to fetch the address.

Without constexpr the symbol generated is

0E 00000000 UNDEF notype External | imp_?val@@3HA (declspec(dllimport) int val)

and the assembly generates a ‘load __imp_? Val’. Same than MSVC, but the test case fails to run. @rnk Shouldn't this run?

Can you give the full exact repro for this case, without constexpr, which you say fails to run - I tried to follow the instructions but for me it seems to work just fine, without constexpr.

Here is an even smaller test case.
lib.cpp:
__declspec(dllexport) int val=12;

t1.cpp:
int main () {

extern int _declspec(dllimport) val;
int& val_ref = val;
return val;

}
$ clang.exe -LD lib.cpp (this will create a.lib and a.exp)
$ clang.exe t1.cpp -la
$ ./a.exe
This will result in a popup message: "The application was unable to start correctly ..."

With MSVC
$ cl /LD lib.cpp (this will create lib.lib and lib.dll)
$ cl t1.cpp /link lib.lib
$ t1.exe
$ echo $?
12
$

[mstorsjo]

In D137107#3906439, @zahiraam wrote:

Here is an even smaller test case.
lib.cpp:
__declspec(dllexport) int val=12;

t1.cpp:
int main () {

extern int _declspec(dllimport) val;
int& val_ref = val;
return val;

}
$ clang.exe -LD lib.cpp (this will create a.lib and a.exp)
$ clang.exe t1.cpp -la
$ ./a.exe
This will result in a popup message: "The application was unable to start correctly ..."

What does llvm-readobj --coff-imports a.exe show here?

What environment are you running this in? -LD is a cl/clang-cl style option and can't be passed to the gnu-style interface of clang.exe.

When I try to follow these examples in cmd.exe, I get the following:

>clang.exe -LD lib.cpp
   Creating library a.lib and object a.exp
LINK : fatal error LNK1561: entry point must be defined
clang: error: linker command failed with exit code 1561 (use -v to see invocation)

It looks to me like the way you're invoking the compiler here is creating the issue you're observing. This testcase works just fine for me when I build it like this:

>clang-cl -LD lib.cpp
>clang-cl t1.cpp -link lib.lib
>t1.exe
>echo %errorlevel%
12

[rnk]

In D137107#3905443, @zahiraam wrote:

extern int __declspec(dllimport) next(int n);
int main () {
  extern int _declspec(dllimport) val;
  constexpr int& val_ref = val;
  int i = next(val_ref);
  return i;
}

@rnk Shouldn't this run?

Yes, I agree, this is a bug. Clang should compile this and reference __imp_next here. However, Clang should continue producing errors when a dllimport symbol is used to initialize a constexpr global variable, which was one of the other cases mentioned in the initial report.

[zahiraam]

In D137107#3906766, @rnk wrote:

In D137107#3905443, @zahiraam wrote:

extern int __declspec(dllimport) next(int n);
int main () {
  extern int _declspec(dllimport) val;
  constexpr int& val_ref = val;
  int i = next(val_ref);
  return i;
}

@rnk Shouldn't this run?

Yes, I agree, this is a bug. Clang should compile this and reference __imp_next here. However, Clang should continue producing errors when a dllimport symbol is used to initialize a constexpr global variable, which was one of the other cases mentioned in the initial report.

@rnk would you mind looking at the fix I am proposing for this bug? Thanks.

[rnk]

Sorry for the delay, dev meeting. I think we should try to add some reviewers for IR gen to help, I don't have a lot of time to be responsive:
https://github.com/llvm/llvm-project/blob/main/clang/CodeOwners.rst#clang-llvm-ir-generation
+@efriedma @hans

clang/test/CodeGenCXX/PR19955.cpp

17 ↗

(On Diff #473693)

This seems concerning to me, are we sure this is the right change? It's important that varp points to the imported var, and not a local copy, which is what https://llvm.org/PR19955 deals with.

[efriedma]

There are multiple notions of "constant" here:

• Whether a value is invariant at runtime.
• Whether a value is legal in a "constexpr" expression.
• Whether we can emit a constant without emitting code that runs at startup to initialize it.

The third is generally a superset of the second; most targets have relocations that allow us emit the address of a global variable into another global variable. dllimport variables are an exception: even though the address is constant, we can't emit a relocation.

To make this work, what we need to do is:

• Extend the notion of a "constant" in the AST/Sema to allow dllimport variables. (There should be code specifically checking for dllimport variables; we just need to drop that check.)
• Teach IR generation (clang/lib/CodeGen) how to emit constants that don't have a relocation. This probably means a check that if we're generating the initializer for a global variable, and the initializer refers to the address of a dllimport variable, we instead emit a global constructor. (Maybe with a higher priority that normal global constructor, so the user can't accidentally observe an uninitialized variable? I forget if MSVC actually has constructor priorities...). Or I guess as an initial patch, we could just emit an error if we detect this case.

[zahiraam]

In D137107#3931259, @efriedma wrote:

There are multiple notions of "constant" here:

• Whether a value is invariant at runtime.
• Whether a value is legal in a "constexpr" expression.
• Whether we can emit a constant without emitting code that runs at startup to initialize it.

The third is generally a superset of the second; most targets have relocations that allow us emit the address of a global variable into another global variable. dllimport variables are an exception: even though the address is constant, we can't emit a relocation.

To make this work, what we need to do is:

• Extend the notion of a "constant" in the AST/Sema to allow dllimport variables. (There should be code specifically checking for dllimport variables; we just need to drop that check.)
• Teach IR generation (clang/lib/CodeGen) how to emit constants that don't have a relocation. This probably means a check that if we're generating the initializer for a global variable, and the initializer refers to the address of a dllimport variable, we instead emit a global constructor. (Maybe with a higher priority that normal global constructor, so the user can't accidentally observe an uninitialized variable? I forget if MSVC actually has constructor priorities...). Or I guess as an initial patch, we could just emit an error if we detect this case.

@efriedma Thanks for taking the time to look at it and for the explanation. I made an attempt to make a few changes. Please let me know if this is moving in the right direction. Thanks.

[efriedma]

That's roughly what I was thinking, yes.

The one missing piece is the code to modify the constructor priorities to make sure that "constant" variables get initialized first. (This ensures we honor the C++ rules for "constant initialization".)

clang/lib/Sema/SemaDecl.cpp

13896 ↗

(On Diff #478624)

I don't understand why this diagnostic is necessary.

clang/test/CodeGenCXX/PR19955.cpp

10 ↗

(On Diff #478624)

We probably want to continue using "null" as the placeholder here, to avoid any confusion.

[efriedma]

clang/lib/CodeGen/CodeGenModule.cpp

4822 ↗

(On Diff #478624)

This check probably needs to be inside tryEmitForInitializer, so other callers of tryEmitForInitializer don't get confused.

[zahiraam]

clang/lib/Sema/SemaDecl.cpp

13896 ↗

(On Diff #478624)

Now that the DLLImport variable can be a constant, HasConstInit is returning true (it was before returning false) and when the variable is global a diagnostic should be reported.

This:

extern int _declspec(dllimport) val;
constexpr int& val_ref = val;

should report a diagnostic, but this:

int foo() { 
   extern int _declspec(dllimport) val; 
   constexpr int& val_ref = val;
}

shouldn't report a diagnostic.

[efriedma]

clang/lib/Sema/SemaDecl.cpp

13896 ↗

(On Diff #478624)

MSVC doesn't report a diagnostic for either of your examples?

[zahiraam]

clang/lib/Sema/SemaDecl.cpp

13896 ↗

(On Diff #478624)

So, these shouldn't fail anymore?
https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/PR19955.cpp#L5
https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/PR19955.cpp#L8
https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/dllimport-constexpr.cpp#L44

[efriedma]

Looking at this again, I just thought of something: in C mode, we probably don't want to generate global constructors, so we might want to continue to print an error in that case.

clang/lib/Sema/SemaDecl.cpp

13896 ↗

(On Diff #478624)

Right, they shouldn't fail.

My perspective is that since we're able to give those the expected semantics, we should just do that. The only reason that "dllimport" is relevant at all is that there isn't a relocation for it, but our plan is to cover that up with a runtime constructor. So the user shouldn't need to know there's trickery behind the scenes unless they disassemble the binary.

[zahiraam]

clang/lib/Sema/SemaDecl.cpp

13896 ↗

(On Diff #478624)

Got it! Thanks. Will remove the diagnostic here then.

[rnk]

clang/lib/Sema/SemaDecl.cpp

13896 ↗

(On Diff #478624)

I worry that some of our users really do care quite a lot about constant initialization. I think emitting a special runtime initializer may be our best option (mingw does it through other means), but this is likely to be disruptive. Chrome has code which runs before CRT initialization to do some early sandboxing. ASan also competes to initialize shadow memory as early as possible (search compiler-rt for .CRT).

I worry that these users will think, "well, I've used [[constinit]] and constexpr, so this global should *definitely* be initialized during startup, and I won't have to worry about init order fiasco problems!", and then they will be surprised to find that this is not the case. Should we care? What can we do about this, realistically? Leaving behind this portability pothole of "dlimport symbols just aren't constexpr" seems worse. Our current behavior may be unduly influenced by my particular values: I think constant initialization is really important, I really care a lot about whether things are relocated or initialized at runtime.

I don't see any good solutions, so please carry on, but if you have any good ideas, please share. :)

[efriedma]

clang/lib/Sema/SemaDecl.cpp

13896 ↗

(On Diff #478624)

For the obscure cases where the user actually cares about that sort of thing, we can make -Wglobal-constructors trigger, I guess? I'm not sure what else we could do that doesn't break the feature.

---

It might be worth trying to reach out to Microsoft to figure out a common design for the runtime initialization thing. MSVC appears to emit broken code in certain cases at the moment. For example, in the following, "z" is correctly initialized at runtime, but "z2" emits a broken direct reference.

_declspec(dllimport) int x;
constexpr int *y = &x;
int *z = &x;
int *z2 = y;

[zahiraam]

clang/lib/Sema/SemaDecl.cpp

13896 ↗

(On Diff #478624)

Thank you for the feedback.

I think these are really corner cases, right? Then maybe it's an acceptable solution to use the -Wglobal-constructors? I suggest we proceed with these changes?

@efriedma the one thing that's missing is the priority in which these constructors are initialized. You said in an earlier comment that they should have higher priority than normal global constructors. What priority should be given to them? I see this PrioritizedCXXGlobalInits being filled in EmitCXXGlobalVarDeclInitFunc. Or should it be through AddGlobalCtor? And what priority should be given to them? Should it be hard coded (horrible!)? Or calculated using OrderGlobalInitsOrStermFinalizers?

[efriedma]

I'm not really familiar with the way constructor priorities work on Windows works... see https://reviews.llvm.org/D131910 ? Adding @aeubanks as a reviewer.

[rnk]

To recap, I'm in favor of implementing dllimport constexpr global initializers with a high priority dynamic initializer. I would suggest using init_priority of 201 to run right after the "compiler" initializers.

[efriedma]

(-Wglobal-constructors warning is still not implemented.)

clang/lib/CodeGen/CGDeclCXX.cpp

572

How is ConstInitAttr relevant here?

clang/test/Sema/dllimport.c

41 ↗

(On Diff #479652)

Like I mentioned before, we might want to restrict this feature to C++.

[zahiraam]

clang/lib/CodeGen/CGDeclCXX.cpp

572

This change made (without the !(D->hasAttr<ConstInitAttr>()) made the LIT behavior of aix-static-init.cpp. The IR generated for
namespace test3 {

struct Test3 {
  constexpr Test3() {};
  ~Test3() {};
};

constinit Test3 t;

} // namespace test3

was different. I would have thought that the change we made for constexpr wouldn't affter constinit?

clang/test/Sema/dllimport.c

41 ↗

(On Diff #479652)

Sorry I forgot about that. I will change that so that the diag is generated only for C++.

[efriedma]

clang/lib/CodeGen/CGDeclCXX.cpp

572

I think the significant bit there isn't the use of constinit; it's the non-trivial destructor. I think the priority modification should only affect constructors, not destructors. (Not sure how to make that work, at first glance.)

[zahiraam]

@eli.friedman Thanks.

clang/lib/CodeGen/CGDeclCXX.cpp

572

Let's see if this is an acceptable solution.

[efriedma]

clang/lib/CodeGen/CGDeclCXX.cpp

572

To fake constant initialization, we need to initialize the variable before anything else runs. But the rearranged prioritization isn't supposed to affect the destructor. From [basic.start.term]: "If an object is initialized statically, the object is destroyed in the same order as if the object was dynamically initialized."

What you're doing here isn't exactly implementing that. What you're doing here is delaying both the initialization and the destruction if the variable has a non-trivial destructor. We need to separate the two to get the behavior we want.

[zahiraam]

clang/lib/CodeGen/CGDeclCXX.cpp

572

Could we consider adding a field to EvaluatedStmt called "HasTrivialDestrutor" and only perform the prioritization change when
!D->HasTrivialDesctructor? Instead of using the test for D->hasConstantInitialization(). This seems to be englobing to many cases.

I considered returning null for HasConstantInitialization in case of var has a non-trivial destructor but that doesn't seem to work.

[efriedma]

clang/lib/CodeGen/CGDeclCXX.cpp

572

Once you separate out the initialization from the destruction, the rest should come together naturally, I think? I'm not sure what other cases could be caught by hasConstantInitialization().

[zahiraam]

clang/lib/CodeGen/CGDeclCXX.cpp

572

Does this change accomplish this? Thanks.

[efriedma]

clang/lib/CodeGen/CGDeclCXX.cpp

572

When a global variable is dynamically initialized, there are two parts to the initialization:

• Constructing the variable (calling the constructor, or equivalent)
• Registering the destructor with the runtime (atexit on Windows)

If an object is initialized statically, the two parts are separated: the variable is emitted already initialized by the compiler. But we still register the destructor at the same time, in the same way.

If a dllimport object is initialized statically, we need to make it appear to user code that the same thing happened. So we need to initialize the object early, but we need to register the destructor at the same time we would have otherwise registered it. To make this work, we need two separate initializer functions with different priorities.

[zahiraam]

clang/lib/CodeGen/CGDeclCXX.cpp

572

Thanks for the clarification. I am slowly getting it, I think.

In terms of IR, for example for this:

struct Test3 {
  constexpr Test3() {};
  ~Test3() {};
};

constinit Test3 t;

Are we looking for something like this (partial IR):

@t = global %struct.Test3 undef
@llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 **201**, ptr @_GLOBAL__I_000201, ptr null }]
@llvm.global_dtors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 **65535**, ptr @_GLOBAL__D_a, ptr null }]
define internal void @__cxx_global_var_init() {
entry:
  call void @_ZN5Test3C1Ev(ptr noundef nonnull align 1 dereferenceable(1) @t)
  %0 = call i32 @atexit(ptr @__dtor_t) 
  ret void
}
define internal void @__finalize_t() {
 %0 = call i32 @unatexit(ptr @__dtor_t) 
 %needs_destruct = icmp eq i32 %0, 0
 br i1 %needs_destruct, label %destruct.call, label %destruct.end

destruct.call:  
  call void @__dtor_t()
.....}
}

Or

define internal void @__cxx_global_var_init()  {
entry:
  call void @_ZN5Test3C1Ev(ptr noundef nonnull align 1 dereferenceable(1) @t)
  ret void
}
define internal void @__finalize_t()  {
%0 = call i32 @atexit(ptr @__dtor_t) 
}

[efriedma]

clang/lib/CodeGen/CGDeclCXX.cpp

572

global_dtors is not at all relevant. (It's not something we ever use for C++ globals, and it doesn't have the right semantics for that anyway, and I don't think we even support it on Windows.) The sequence we currently generate looks something like this:

@llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__sub_I__, ptr null }]

define internal void @_GLOBAL__sub_I__() #1 {
entry:
  store ptr @"?x@@3HA", ptr @"?y@@3UX@@A", align 8, !tbaa !4
  %0 = tail call i32 @atexit(ptr nonnull @"??__Fy@@YAXXZ") #2
  ret void
}

What we want is something like this:

@llvm.global_ctors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 201, ptr @ctor, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @dtor, ptr null }]

define internal void @ctor() {
entry:
  store ptr @"?x@@3HA", ptr @"?y@@3UX@@A", align 8, !tbaa !4
  ret void
}

define internal void @dtor() {
entry:
  %0 = tail call i32 @atexit(ptr nonnull @"??__Fy@@YAXXZ") #2
  ret void
}

[zahiraam]

This uploaded patch is a draft (LIT tests fails but are not updated since I am not sure about these changes) that I am part way confident about. There are still a lot of things that I need to look at, but would you mind looking at it to see if this is moving in the right direction?

[efriedma]

Not sure exactly what code that generates in its current form, but that's roughly the right idea, yes.

[zahiraam]

In D137107#3999681, @efriedma wrote:

Not sure exactly what code that generates in its current form, but that's roughly the right idea, yes.

Thanks.

This is the IR I am getting for this simple case. I think there is still something wrong with the call to the atexit. Still trying to find out where this is generated in the code.

// RUN: %clang_cc1 -no-enable-noundef-analysis x86_64-windows-msvc  \
// RUN: -fno-rtti -fno-threadsafe-statics -fms-extensions -emit-llvm -std=c++2a \
// RUN: -O0 -o - %s | FileCheck %s

// CHECK: @"?b@@3UB@@A" = dso_local global %struct.B undef, align 1
// CHECK: @llvm.global_ctors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 201, ptr @ctor, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @dtor, ptr null }]

// CHECK: define internal void @ctor() #0 {
// CHECK: entry:
// CHECK:   %call = call x86_thiscallcc ptr @"??0B@@QAE@XZ"(ptr nonnull align 1 dereferenceable(1) @"?b@@3UB@@A")
// CHECK:   %0 = call i32 @atexit(ptr @"??__Fb@@YAXXZ") #2    **<== This shouldn't be called here, right?**
// CHECK:   ret void
// CHECK: }

// CHECK: define internal void @dtor() #0 {
// CHECK: entry:
// CHECK:   %0 = call i32 @atexit(ptr @"??__Fb@@YAXXZ.1") #2
// CHECK:   ret void
// CHECK: }

struct B {
  constexpr B() {}
  ~B() {};
};
constinit B b;

[efriedma]

Right, the call to atexit from the constructor function shouldn't be there. I think you need to add a "skip emitting the destructor" flag to EmitCXXCtorInit

[efriedma]

clang/lib/CodeGen/CodeGenModule.cpp

4761 ↗

(On Diff #484006)

Not sure what the D->getType()->isRecordType() check is doing here.

4852 ↗

(On Diff #484006)

I have no idea what this code is supposed to do.

5009 ↗

(On Diff #484006)

I think you want to use priority 201 whether or not there's a destructor.

[zahiraam]

clang/lib/CodeGen/CodeGenModule.cpp

4852 ↗

(On Diff #484006)

I have removed from this the thread_local variables for being processed the same way. Maybe they should be included, not sure?

For the rest is to differentiate between these cases. I found the 2nd test case while doing some unit testing.

struct B {
constexpr B() {}
~B() {};
};
constinit B b;

and

struct A {
  int s;
  ~A();
};

struct D : A {};
D d1 = {};

I think the second test case is not supposed to EmitDeclInit in EmitCXXGlobalVarDeclInit right? But since now tryEmitForInitializer is returning an Initializer, then NeedsGlobalCtor = true;

Actually, when I removed this code, I have 2 LIT tests failing with the same crash.
WDYT?

5009 ↗

(On Diff #484006)

Is that what you mean?

[efriedma]

clang/lib/CodeGen/CodeGenModule.cpp

4852 ↗

(On Diff #484006)

I'm not sure how thread-local var init works on Windows off the top of my head; I'd need to check.

For the given testcases, neither one of those cases should be calling EmitDeclInit; we use constant initialization for both cases, so the global init func should just be registering the destructors. We shouldn't need to examine the initializer beyond checking whether tryEmitForInitializer succeeds.

Not sure what's crashing from your description.

[zahiraam]

clang/lib/CodeGen/CodeGenModule.cpp

4852 ↗

(On Diff #484006)

Removing this code will still result, in both cases to the call to EmitDeclInit, because the first call to EmitCXXCtorInit is called with PerformInit true.

Calling EmitCXXCtorInit with false would result into a ctor function with an empty body for the 1st test case (struct B):

define internal void @ctor() #0 {
entry:
  ret void
}

I would have thought that we should have something like this:

define internal void @ctor() #0 {
entry:
  %call = call x86_thiscallcc ptr @"??0B@@QAE@XZ"(ptr nonnull align 1 dereferenceable(1) @"?b@@3UB@@A")
  ret void
}

No?

[efriedma]

clang/lib/CodeGen/CodeGenModule.cpp

4852 ↗

(On Diff #484006)

because the first call to EmitCXXCtorInit is called with PerformInit true.

That sounds wrong; if tryEmitForInitializer succeeds, we shouldn't perform the init at all.

5009 ↗

(On Diff #484006)

I think it should look something more like this:

if (isStaticInit(D, getLangOpts()) {
  if (NeedsGlobalCtor)
    EmitCXXCtorInit(D, GV, true, 201, llvm::StringLiteral("ctor"), false);
  if (NeedsGlobalDtor)
    EmitCXXCtorInit(D, GV, false, 65535, llvm::StringLiteral("dtor"), true);
  DelayedCXXInitPosition[D] = ~0U;
} else {
  EmitCXXGlobalVarDeclInitFunc(D, GV, NeedsGlobalCtor);
}

[zahiraam]

clang/lib/CodeGen/CodeGenModule.cpp

5009 ↗

(On Diff #484006)

If you agree with the generated IR for this case, then I will start editing the LIT tests accordingly.

// CHECK: @"?b@@3UB@@A" = dso_local global %struct.B undef
// CHECK: @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @dtor, ptr null }]

// CHECK: define internal void @dtor()
// CHECK: entry:
// CHECK:     %0 = call i32 @atexit(ptr @"??__Fb@@YAXXZ")
// CHECK:   ret void

// CHECk: define linkonce_odr dso_local x86_thiscallcc void @"??1B@@QAE@XZ"
// CHECK: entry:
// CHECK:  %this.addr = alloca ptr, align 4
// CHECK:  store ptr %this, ptr %this.addr, align 4
// CHECK:  %this1 = load ptr, ptr %this.addr, align 4
// CHECK:  ret void


// CHECK: define internal void @"??__Fb@@YAXXZ"() #0 {
// CHECK: entry:
// CHECK:   call x86_thiscallcc void @"??1B@@QAE@XZ"(ptr @"?b@@3UB@@A")
// CHECK:   ret void


struct B {
  constexpr B() {}
  ~B() {};
};
constinit B b;

The ctor with priority 201 is generated only when tryEmitForInitializer returns a nullptr (that's the only time when NeedsGlobalCtor is set to true). Correct?

[zahiraam]

clang/lib/CodeGen/CodeGenModule.cpp

5006 ↗

(On Diff #484258)

Do you agree this should be done only when one of those flags is on?

[zahiraam]

clang/test/CodeGenCXX/PR19955.cpp

20 ↗

(On Diff #484278)

@rnk was concerned about varp pointing to dllimport var. That does it I think.

[efriedma]

clang/include/clang/Basic/DiagnosticCommonKinds.td

254

Missing code to emit this warning?

clang/lib/CodeGen/CodeGenModule.cpp

5006 ↗

(On Diff #484258)

Yes, that's fine; I wasn't really paying close attention to the exact code. Just wanted to make the point about the structure of the if statements, and code was the easiest way to explain it.

Maybe the outer if statement should actually be if (isStaticInit(D, getLangOpts()) && NeedsGlobalCtor) {.

On a related note, we might want to avoid the name "ctor", in case that accidentally conflicts with some user code; an "__"-prefixed name would be appropriate.

5009 ↗

(On Diff #484006)

Right, we only need the priority 201 ctor if tryEmitForInitializer fails, but we need constant initialization anyway to meet the C++ language requirements.

Updated code seems much cleaner.

clang/test/CodeGenCXX/PR19955.cpp

16 ↗

(On Diff #484278)

It's easier to debug FileCheck failures if you use CHECK-LABEL for function definitions.

You probably want to use a different prefix that's used for both 32-bit and 64-bit for lines like this.

CHECK-64 isn't used by any FileCheck invocation.

[zahiraam]

clang/lib/CodeGen/CodeGenModule.cpp

5006 ↗

(On Diff #484258)

Maybe the outer if statement should actually be if (isStaticInit(D, getLangOpts()) && NeedsGlobalCtor) {

Not sure about that! There are cases where (isStaticInit(D, getLangOpts())) = true and NeedsGlobalCtor=false, but NeedsGlobalDtor=true. In which case a __dtor needs to be emitted, no?

Writing the condition as you are proposing would actually not get me into the body to emit the __dtor. Is that what we want?

[efriedma]

clang/lib/CodeGen/CodeGenModule.cpp

5006 ↗

(On Diff #484258)

EmitCXXGlobalVarDeclInitFunc should be able to handle that case.

Looking again, I'm a little concerned that in the isStaticInit() case, we're skipping a bunch of the logic in EmitCXXGlobalVarDeclInitFunc. EmitCXXCtorInit handles the basic cases correctly, but there are a lot of special cases in EmitCXXGlobalVarDeclInitFunc.

[zahiraam]

clang/lib/CodeGen/CodeGenModule.cpp

5006 ↗

(On Diff #484258)

I have left the condition as it was to make sure no cases are left. What other cases are you thinking of?

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1926

I think this will trigger in cases we don't actually want it to; we only want to warn when we actually generate a global constructor, not when we end up falling back to generated code within a function. (For example, we sometimes constant-evaluate local variables.)

clang/lib/CodeGen/CodeGenModule.cpp

5006 ↗

(On Diff #484258)

EmitCXXGlobalVarDeclInitFunc has special cases for CUDA, OpenMP, thread-local variables, the InitSeg attribute, and inline variables.

[zahiraam]

clang/lib/CodeGen/CodeGenModule.cpp

5006 ↗

(On Diff #484258)

Let me know if this works? I have added one additional LIT test. I referred to https://learn.microsoft.com/en-us/cpp/cpp/storage-classes-cpp?view=msvc-170#thread_local for thread_local.
For the inline variables, I couldn't find much information about initialization of inline variables. I tried to find a case that wouldn't pass by the new code and wouldn't pass by EmitCXXGlobalVarDeclInitFunc, but no success.

[zahiraam]

ping.

[efriedma]

clang/lib/CodeGen/CGDeclCXX.cpp

519

I don't really like the copy-pasted bits here.

clang/lib/CodeGen/CodeGenModule.cpp

5006 ↗

(On Diff #484258)

Inline variable example:

__declspec(dllimport) extern int x;
inline int *y = &x;
int **z = &y;

This should trigger your new codepath, I think?

clang/test/CodeGenCXX/constant-init.cpp

17 ↗

(On Diff #486274)

This doesn't look like your new codepath is triggering? If your code is working correctly, it should, I think?

[zahiraam]

clang/lib/CodeGen/CodeGenModule.cpp

5006 ↗

(On Diff #484258)

Yes. And the IR looks like this:

@"?y@@3PEAHEA" = linkonce_odr dso_local global ptr null, comdat, align 8
@"?z@@3PEAPEAHEA" = dso_local global ptr @"?y@@3PEAHEA", align 8
@"?x@@3HA" = external dllimport global i32, align 4
@llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 201, ptr @__ctor, ptr null }]

; Function Attrs: noinline nounwind
define linkonce_odr dso_local void @__ctor() #0 comdat {
entry:
  store ptr @"?x@@3HA", ptr @"?y@@3PEAHEA", align 8
  ret void
}

I think this is correct.
Adding it to the LIT testing.

clang/test/CodeGenCXX/constant-init.cpp

17 ↗

(On Diff #486274)

That's right. In all cases where

llvm::Constant *Initializer = emitter->tryEmitForInitializer(*InitDecl);

is returning an initializer (with NeedsGlobalCtor=false and NeedsGlobalDtor=true) the control path is following the old path (no call to EmitCXXCtorInit), i.e. call to EmitCXXGlobalVarDeclInitFunc.

Unless we want to change the value of NeedsGlobaCtor in this case?

With:

diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index f7add1350238..106ac355d356 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp

+++ b/clang/lib/CodeGen/CodeGenModule.cpp

@@ -4898,6 +4898,8 @@ void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D,
     // also don't need to register a destructor.
     if (getLangOpts().CPlusPlus && !NeedsGlobalDtor)
       DelayedCXXInitPosition.erase(D);
+      if (isStaticInit(D, getLangOpts()) && !NeedsGlobalCtor)
+        NeedsGlobalCtor = true;

 #ifndef NDEBUG
      CharUnits VarSize = getContext().getTypeSizeInChars(ASTTy) +

In which case for:

struct Test1 {
constexpr Test1(int) {}
~Test1() {}
};
inline constinit Test1 t2 = 2;

We would get this IR?

@llvm.global_ctors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 201, ptr @__ctor, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @__dtor, ptr null }]

; Function Attrs: noinline nounwind
define linkonce_odr dso_local void @__ctor() #0 comdat {
entry:

%call = call noundef ptr @"??0Test1@@QEAA@H@Z"(ptr noundef nonnull align 1 dereferenceable(1) @"?t2@@3UTest1@@A", i32 noundef 2)
%0 = call i32 @atexit(ptr @"??__Ft2@@YAXXZ") #2
ret void

}

define internal void @"??__Ft2@@YAXXZ"() #0 {
entry:

call void @"??1Test1@@QEAA@XZ"(ptr @"?t2@@3UTest1@@A")
ret void

}

define linkonce_odr dso_local void @__dtor() #0 comdat {
entry:

%0 = call i32 @atexit(ptr @"??__Ft2@@YAXXZ.1") #2
ret void

}

[zahiraam]

Thanks. Please see my comments/questions.

[zahiraam]

ping?

[zahiraam]

Would be nice to have feedback on this! So much work was put into it! @rnk, @efriedma

[efriedma]

clang/lib/CodeGen/CGDeclCXX.cpp

519

It would be nice it we could to unify EmitCXXCtorInit with EmitCXXGlobalVarDeclInitFunc a bit more.

clang/lib/CodeGen/CodeGenModule.cpp

5006 ↗

(On Diff #484258)

Without your patch, the IR looks significantly different; the constructor function is named ??__Ey@@YAXXZ, and there's a reference to the variable in llvm.global_ctors. This is necessary to ensure we only construct the variable once.

In this particular case, I guess the harm of constructing the variable multiple times would be relatively minor, but it's still not a great idea. And we definitely can't emit a linkonce_odr function and just name it "__ctor".

clang/test/CodeGenCXX/constant-init.cpp

17 ↗

(On Diff #486274)

Reading this again, not sure what I was asking for here.

We should not be calling atexit() from a priority 201 constructor; we should always register it with the normal registration priority, so stuff is destroyed in the right order.

[efriedma]

(I apologize it took me so long to get back to reviewing this.)

[rnk]

Thanks for working on this, this is also an issue for our users. I've been out on leave. I replaced myself as a reviewer with Hans.

clang/include/clang/Basic/DiagnosticCommonKinds.td

def ext_clang_diagnose_if : Extension<"'diagnose_if' is a clang extension">,
                             InGroup<GccCompat>;
 def err_too_large_for_fixed_point : Error<
   "this value is too large for this fixed point type">;
 def err_fixed_point_not_enabled : Error<"compile with "
   "'-ffixed-point' to enable fixed point types">;
 def err_unimplemented_conversion_with_fixed_point_type : Error<
   "conversion between fixed point and %0 is not yet supported">;
 
+def warn_for_global_ctor_for_dllimport : Warning<
+  "global constructor is generated for dllimport global var %0">,
+  InGroup<GlobalConstructors>;
+
 // SEH
 def err_seh_expected_handler : Error<
   "expected '__except' or '__finally' block">;
 def err_seh___except_block : Error<
   "%0 only allowed in __except block or filter expression">;
 def err_seh___except_filter : Error<
   "%0 only allowed in __except filter expression">;
 def err_seh___finally_block : Error<

clang/lib/AST/ExprConstant.cpp

static bool CheckLValueConstantExpression(EvalInfo &Info, SourceLocation Loc,
 
   if (BaseVD) {
     if (const VarDecl *Var = dyn_cast<const VarDecl>(BaseVD)) {
       // Check if this is a thread-local variable.
       if (Var->getTLSKind())
         // FIXME: Diagnostic!
         return false;
 
-      // A dllimport variable never acts like a constant, unless we're
-      // evaluating a value for use only in name mangling.
+      // In C mode, a dllimport variable never acts like a constant, unless
+      // we're evaluating a value for use only in name mangling.
+      if (!Info.getCtx().getLangOpts().CPlusPlus)
         if (!isForManglingOnly(Kind) && Var->hasAttr<DLLImportAttr>())
           // FIXME: Diagnostic!
           return false;
 
       // In CUDA/HIP device compilation, only device side variables have
       // constant addresses.
       if (Info.getCtx().getLangOpts().CUDA &&
           Info.getCtx().getLangOpts().CUDAIsDevice &&
           Info.getCtx().CUDAConstantEvalCtx.NoWrongSidedVars) {
         if ((!Var->hasAttr<CUDADeviceAttr>() &&
              !Var->hasAttr<CUDAConstantAttr>() &&

clang/lib/CodeGen/CGDeclCXX.cpp

CodeGenModule::EmitCXXGlobalVarDeclInitFunc(const VarDecl *D,
 
   // According to E.2.3.1 in CUDA-7.5 Programming guide: __device__,
   // __constant__ and __shared__ variables defined in namespace scope,
   // that are of class type, cannot have a non-empty constructor. All
   // the checks have been done in Sema by now. Whatever initializers
   // are allowed are empty and we just need to ignore them here.
   if (getLangOpts().CUDAIsDevice && !getLangOpts().GPUAllowDeviceInit &&
       (D->hasAttr<CUDADeviceAttr>() || D->hasAttr<CUDAConstantAttr>() ||
        D->hasAttr<CUDASharedAttr>()))
     return;
 
   if (getLangOpts().OpenMP &&
       getOpenMPRuntime().emitDeclareTargetVarDefinition(D, Addr, PerformInit))
     return;
 
   // Check if we've already initialized this decl.
   auto I = DelayedCXXInitPosition.find(D);
if (D->getTLSKind()) {
     if (Priority != -1)
       AddGlobalCtor(Fn, Priority, ~0U, COMDATKey);
     else
       EmitPointerToInitFunc(D, Addr, Fn, ISA);
   } else if (auto *IPA = D->getAttr<InitPriorityAttr>()) {
     OrderGlobalInitsOrStermFinalizers Key(IPA->getPriority(),
                                           PrioritizedCXXGlobalInits.size());
     PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn));
+  } else if (D->hasConstantInitialization()) {
+    if (isRecordHasTrivialDestructor(D))
+      CXXGlobalInits.push_back(Fn);
+    else {
+      DiagnosticsEngine &Diags = getContext().getDiagnostics();
+      Diags.Report(diag::warn_for_global_ctor_for_dllimport) << D;
+      OrderGlobalInitsOrStermFinalizers Key(201,
+                                            PrioritizedCXXGlobalInits.size());
+      PrioritizedCXXGlobalInits.push_back(std::make_pair(Key, Fn));
+    }
   } else if (isTemplateInstantiation(D->getTemplateSpecializationKind()) ||
              getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR ||
              D->hasAttr<SelectAnyAttr>()) {
     // C++ [basic.start.init]p2:
     //   Definitions of explicitly specialized class template static data
     //   members have ordered initialization. Other class template static data
     //   members (i.e., implicitly or explicitly instantiated specializations)
     //   have unordered initialization.

clang/lib/CodeGen/CGExprConstant.cpp

if (D->hasAttr<WeakRefAttr>())
       return CGM.GetWeakRefReference(D).getPointer();
 
     if (auto FD = dyn_cast<FunctionDecl>(D))
       return CGM.GetAddrOfFunction(FD);
 
     if (auto VD = dyn_cast<VarDecl>(D)) {
       // We can never refer to a variable with local storage.
       if (!VD->hasLocalStorage()) {
-        if (VD->isFileVarDecl() || VD->hasExternalStorage())
+        if (VD->isFileVarDecl() || VD->hasExternalStorage()) {
+          if (VD->hasAttr<DLLImportAttr>())
+            return nullptr;
           return CGM.GetAddrOfGlobalVar(VD);
+        }
 
         if (VD->isLocalVarDecl()) {
           return CGM.getOrCreateStaticVarDecl(
               *VD, CGM.getLLVMLinkageVarDefinition(VD, /*IsConstant=*/false));
         }
       }
     }
 

clang/test/CodeGenCXX/dllimport.cpp

-// RUN: %clang_cc1 -no-enable-noundef-analysis -triple i686-windows-msvc   -fno-rtti -fno-threadsafe-statics -fms-extensions -emit-llvm -std=c++1y -O0 -o - %s -DMSABI -w | FileCheck --check-prefix=MSC --check-prefix=M32 %s
-// RUN: %clang_cc1 -no-enable-noundef-analysis -triple x86_64-windows-msvc -fno-rtti -fno-threadsafe-statics -fms-extensions -emit-llvm -std=c++1y -O0 -o - %s -DMSABI -w | FileCheck --check-prefix=MSC --check-prefix=M64 %s
+// RUN: %clang_cc1 -no-enable-noundef-analysis -triple i686-windows-msvc   -fno-rtti -fno-threadsafe-statics -fms-extensions -emit-llvm -std=c++1y -O0 -o - %s -DMSABI -w | FileCheck --check-prefix=MSC --check-prefix=M32 --check-prefix=GL32 %s
+// RUN: %clang_cc1 -no-enable-noundef-analysis -triple x86_64-windows-msvc -fno-rtti -fno-threadsafe-statics -fms-extensions -emit-llvm -std=c++1y -O0 -o - %s -DMSABI -w | FileCheck --check-prefix=MSC --check-prefix=M64 --check-prefix=GL64 %s
 // RUN: %clang_cc1 -no-enable-noundef-analysis -triple i686-windows-gnu    -fno-rtti -fno-threadsafe-statics -fms-extensions -emit-llvm -std=c++1y -O0 -o - %s         -w | FileCheck --check-prefix=GNU --check-prefix=G32 %s
 // RUN: %clang_cc1 -no-enable-noundef-analysis -triple x86_64-windows-gnu  -fno-rtti -fno-threadsafe-statics -fms-extensions -emit-llvm -std=c++1y -O0 -o - %s         -w | FileCheck --check-prefix=GNU %s
 // RUN: %clang_cc1 -no-enable-noundef-analysis -triple i686-windows-msvc   -fno-rtti -fno-threadsafe-statics -fms-extensions -fms-compatibility-version=18.00 -emit-llvm -std=c++1y -O1 -disable-llvm-passes -o - %s -DMSABI -w | FileCheck --check-prefix=MO1 --check-prefix=M18 %s
 // RUN: %clang_cc1 -no-enable-noundef-analysis -triple i686-windows-msvc   -fno-rtti -fno-threadsafe-statics -fms-extensions -fms-compatibility-version=19.00 -emit-llvm -std=c++1y -O1 -disable-llvm-passes -o - %s -DMSABI -w | FileCheck --check-prefix=MO1 --check-prefix=M19 %s
 // RUN: %clang_cc1 -no-enable-noundef-analysis -triple i686-windows-gnu    -fno-rtti -fno-threadsafe-statics -fms-extensions                                  -emit-llvm -std=c++1y -O1 -disable-llvm-passes -o - %s         -w | FileCheck --check-prefix=GO1 %s
 
 // CHECK-NOT doesn't play nice with CHECK-DAG, so use separate run lines.
 // RUN: %clang_cc1 -no-enable-noundef-analysis -triple i686-windows-msvc   -fno-rtti -fno-threadsafe-statics -fms-extensions -emit-llvm -std=c++1y -O0 -o - %s -DMSABI -w | FileCheck --check-prefix=MSC2 %s
namespace ClassTemplateStaticDef {
   };
   template <typename A> int T<A*>::x;
   // GNU-DAG: @_ZN22ClassTemplateStaticDef1TIPvE1xE = external dllimport global i32
   int g() { return T<void*>::x; }
 }
 
 namespace PR19933 {
 // Don't dynamically initialize dllimport vars.
-// MSC2-NOT: @llvm.global_ctors
-// GNU2-NOT: @llvm.global_ctors
 
   struct NonPOD { NonPOD(); };
   template <typename T> struct A { static NonPOD x; };
   template <typename T> NonPOD A<T>::x;
   template struct __declspec(dllimport) A<int>;
   USEVARTYPE(NonPOD, A<int>::x);
   // MSC-DAG: @"?x@?$A@H@PR19933@@2UNonPOD@2@A" = external dllimport global %"struct.PR19933::NonPOD"
 

 
 template <typename T> struct PR23770BaseTemplate { void f() {} };
 template <typename T> struct PR23770DerivedTemplate : PR23770BaseTemplate<int> {};
 extern template struct PR23770DerivedTemplate<int>;
 template struct __declspec(dllimport) PR23770DerivedTemplate<int>;
 USEMEMFUNC(PR23770BaseTemplate<int>, f);
 // M32-DAG: declare dllimport x86_thiscallcc void @"?f@?$PR23770BaseTemplate@H@@QAEXXZ"
 
+// MSC-DAG: @"?val@@3HA" = external dllimport global i32
+// GL32-DAG: @"?x@@3PAHA" = dso_local global ptr null
+// GL64-DAG: @"?x@@3PEAHEA" = dso_local global ptr null
+// MSC-DAG: @"?impx@@3HA" = external dllimport global i32
+// MSC-DAG: @llvm.global_ctors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 201, ptr @_GLOBAL__I_000201, ptr null }, { i32, ptr, ptr } { i32 65535, ptr @_GLOBAL__sub_I_dllimport.cpp, ptr null }]
+
 namespace PR27810 {
   template <class T>
   struct basic_ostream {
     struct sentry {
       sentry() { }
       void foo() { }
     };
   };
void foo() {
     // Don't inherit dllimport to src before attaching the initializer.
     static constexpr char src[] = {"hello"};
     T arr[sizeof(src)];
   }
 };
 void baz() { U<int> u; u.foo(); } // No diagnostic.
 
 }
+
+void assigndllimporttoconst () {
+  // MSC-DAG: define dso_local void @"?assigndllimporttoconst@@YAXXZ"()
+  // MSC-DAG: %[[VAL_REF:.*]] = alloca ptr
+  // MSC-DAG-NEXT: store ptr @"?val@@3HA", ptr %[[VAL_REF]]
+  extern int _declspec(dllimport) val;
+  constexpr int& val_ref = val;
+}
+
+_declspec(dllimport) int impx;
+constexpr int *y = &impx;
+int *x = &impx;

clang/test/SemaCXX/PR19955.cpp

 // RUN: %clang_cc1 -triple i686-win32 -verify -std=c++11 %s
 // RUN: %clang_cc1 -triple i686-mingw32 -verify -std=c++11 %s
 
 extern int __attribute__((dllimport)) var;
-constexpr int *varp = &var; // expected-error {{must be initialized by a constant expression}}
+constexpr int *varp = &var;
 
 extern __attribute__((dllimport)) void fun();
 constexpr void (*funp)(void) = &fun; // expected-error {{must be initialized by a constant expression}}
 
 template <void (*)()>
 struct S {};
 S<&fun> x;
 
 template <int *>
 struct U {};
 U<&var> y;

clang/test/SemaCXX/constant-expression-cxx11.cpp

namespace RecursiveOpaqueExpr {
   static_assert(LastNonzero(begin(arr3), end(arr3)) == 2, "");
 }
 
 namespace VLASizeof {
 
   void f(int k) { // expected-note {{here}}
     int arr[k]; // expected-warning {{C99}} expected-note {{function parameter 'k'}}
     constexpr int n = 1 +
-        sizeof(arr) // expected-error {{constant expression}}
+        sizeof(arr) // expected-error{{constexpr variable 'n' must be initialized by a constant expression}}
         * 3;
   }
 }
 
 namespace CompoundLiteral {
   // Matching GCC, file-scope array compound literals initialized by constants
   // are lifetime-extended.
   constexpr int *p = (int*)(int[1]){3}; // expected-warning {{C99}}

clang/test/SemaCXX/dllimport-constexpr.cpp

 
 // Member pointer instantiation.
 template <void (Foo::*MP)()> struct TemplateMemPtr { };
 TemplateMemPtr<&Foo::imported_method> instantiate_mp;
 
 // constexpr initialization doesn't work for dllimport things.
 // expected-error@+1{{must be initialized by a constant expression}}
 constexpr void (*constexpr_import_func)() = &imported_func;
-// expected-error@+1{{must be initialized by a constant expression}}
 constexpr int *constexpr_import_int = &imported_int;
 // expected-error@+1{{must be initialized by a constant expression}}
 constexpr void (Foo::*constexpr_memptr)() = &Foo::imported_method;
 
 // We make dynamic initializers for 'const' globals, but not constexpr ones.
 void (*const const_import_func)() = &imported_func;
 int *const const_import_int = &imported_int;
 void (Foo::*const const_memptr)() = &Foo::imported_method;
 
 // Check that using a non-type template parameter for constexpr global
 // initialization is correctly diagnosed during template instantiation.
 template <void (*FP)()> struct StaticConstexpr {
   // expected-error@+1{{must be initialized by a constant expression}}
   static constexpr void (*g_fp)() = FP;
 };
 void instantiate3() {
   // expected-note@+1 {{requested here}}
   StaticConstexpr<imported_func>::g_fp();
 }
+
+extern int __declspec(dllimport) val;
+constexpr int& val_ref = val;
+
+void assigndllimporttoconst () {
+  extern int _declspec(dllimport) val;
+  constexpr int& val_ref = val;
+}