dllexport const variables must have external linkage.

[rnk]

This shouldn't be conditional under -fms-compatibility, it's part of dllexport, which is already under -fms-extensions / -fdeclspec-extensions.

I don't think this is the right place to do this. Can it be done as part of processing the dllexport attribute or as part of the main storage class calculation?

[zahiraam]

In D45978#1077051, @rnk wrote:

This shouldn't be conditional under -fms-compatibility, it's part of dllexport, which is already under -fms-extensions / -fdeclspec-extensions.

I don't think this is the right place to do this. Can it be done as part of processing the dllexport attribute or as part of the main storage class calculation?

Is this better? Thanks.

[zahiraam]

[erichkeane]

@rnk and @aaron.ballman, does this look appropriate here?

[aaron.ballman]

lib/Sema/SemaDeclAttr.cpp

6488 ↗

(On Diff #143971)

Formatting and a typo:

C++ mode a const variables defined in namespace scope -> C++ mode, a const variable defined at namespace scope

6490 ↗

(On Diff #143971)

Why handle this here as opposed to within mergeDLLExportAttr()?

test/Sema/dllexport.c

70

Can you think of any redeclaration scenarios we should also test? e.g., weird cases like this:

extern int const x;
__declspec(dllexport) int const x = 3;

which brings up an interesting question -- does MSVC truly treat it as though it were extern, or just only-kinda-sorta treat it like it was extern? e.g., can you do this:

__declspec(dllexport) const int j;

Regardless, having some codegen tests demonstrating that the variable has the correct semantics that we're emulating would be nice.

[rnk]

I still feel like there has to be a more uniform way to handle this. Basically anything with __declspec(dllexport) on it is effectively upgraded to external linkage.

lib/Sema/SemaDeclAttr.cpp

6491 ↗

(On Diff #143971)

This still shouldn't be under -fms-compatibility, it's really part of the main dllexport feature and doesn't need to be conditional on anything at this point.

[zahiraam]

Aaron,

Please advise.
Thanks.

test/Sema/dllexport.c

70

With MSVC:

ksh-3.2$ cat t2.cpp
__declspec(dllexport) const int j;
ksh-3.2$

ksh-3.2$ cl -c t2.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24210 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

t2.cpp
t2.cpp(1): error C2734: 'j': 'const' object must be initialized if not 'extern'

ksh-3.2$ cat t3.cpp
__declspec(dllexport) int const x = 3;
int main ()
{

int y = x + 10;

return y;

}

ksh-3.2$ cl -c t3.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24210 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

t3.cpp
ksh-3.2$ dumpbin /symbols t3.obj | grep External
008 00000000 SECT3 notype () External | main
ksh-3.2$
ksh-3.2$ cat t4.cpp
extern int const x = 3;
int main ()
{

int y = x + 10;

return y;

}

ksh-3.2$ cl -c t4.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24210 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

t4.cpp
ksh-3.2$ dumpbin /symbols t4.obj | grep External
008 00000000 SECT3 notype External | ?x@@3HB (int const x)
00B 00000000 SECT4 notype () External | main
ksh-3.2$

So we see that with dllexport, the ony symbol marked with "External" is main. With "extern" both main and x are marked as External.

With clang without the patch:
ksh-3.2$ clang -c t2.cpp
t2.cpp:1:33: error: default initialization of an object of const type 'const int'
__declspec(dllexport) const int j;

^
  = 0

t2.cpp:1:33: error: 'j' must have external linkage when declared 'dllexport'
2 errors generated.
ksh-3.2$
ksh-3.2$ clang -c t3.cpp
t3.cpp:1:33: error: 'x' must have external linkage when declared 'dllexport'
__declspec(dllexport) int const x = 3;

^

1 error generated.
ksh-3.2$ clang -c t4.cpp
ksh-3.2$ dumpbin /symbols t4.o | grep External
00F 00000000 SECT1 notype () External | main
010 00000000 SECT5 notype External | ?x@@3HB (int const x)
ksh-3.2$

Clang with patch above at the right place (I am thinking in Sema::AddInitializerToDecl):

ksh-3.2$ clang -c t3.cpp
ksh-3.2$ dumpbin /symbols t3.o | grep External
00E 00000000 SECT1 notype () External | main
00F 00000000 SECT5 notype External | ?x@@3HB (int const x)
ksh-3.2$
ksh-3.2$ clang -c t4.cpp
ksh-3.2$ dumpbin /symbols t4.o | grep External
00C 00000000 SECT1 notype () External | main
00D 00000000 SECT5 notype External | ?x@@3HB (int const x)
ksh-3.2$

Both MSVC and clang have the same behavior with t2.cpp.
With the patch clang will have the same beahvior than MSVC when extern and dllexport are used. That's not quite what MSVC's behavior is!
What are your thoughts?
Thanks.

[aaron.ballman]

test/Sema/dllexport.c

70

Neat, so MSVC treats it as sort-of-extern-but-sort-of-not. I can't tell whether this is intentional and we want to be compatible, or whether this is a bug in MSVC (and if so, is it one uses are relying on and we need to emulate). I'm not certain what the best answer is here. Perhaps @rnk or @majnemer have opinions?

[rnk]

I'm still not sure this is the best place to make this change, but the functionality is important. There are still unaddressed comments (no need to check MSVCCompatibility, formatting), and I think once those are fixed we can land this.

test/Sema/dllexport.c

70

Yes, we should be compatible in this area, it's an extension that we implement, not invalid C++.

[zahiraam]

In D45978#1379901, @rnk wrote:

I'm still not sure this is the best place to make this change, but the functionality is important. There are still unaddressed comments (no need to check MSVCCompatibility, formatting), and I think once those are fixed we can land this.

Since it needs to fail for this:

__declspec(dllexport) const int j;

and pass for this:

__declspec(dllexport) int const x = 3;

I am proposing to add this code in Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit).

[aaron.ballman]

In D45978#1382292, @zahiraam wrote:

In D45978#1379901, @rnk wrote:

I'm still not sure this is the best place to make this change, but the functionality is important. There are still unaddressed comments (no need to check MSVCCompatibility, formatting), and I think once those are fixed we can land this.

Since it needs to fail for this:

__declspec(dllexport) const int j;

and pass for this:

__declspec(dllexport) int const x = 3;

I am proposing to add this code in Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit).

That seems like a reasonable place to try, to me.

[zahiraam]

That seems like a reasonable place to try, to me.

Ok. Let's see if this does the trick. Thanks.

[aaron.ballman]

Can you add tests for C mode as well, as it seems the behavior differs there. Given:

__declspec(dllexport) int const x = 3;
__declspec(dllexport) const int y;
extern int const z = 4;

int main() {
  int a = x + y + z;
  return a;
}

I get:

C:\Users\aballman.GRAMMATECH\source\repos\ConsoleApplication1\ConsoleApplication1>cl -c ConsoleApplication1.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27026.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

ConsoleApplication1.cpp
ConsoleApplication1.cpp(2): error C2734: 'y': 'const' object must be initialized if not 'extern'

C:\Users\aballman.GRAMMATECH\source\repos\ConsoleApplication1\ConsoleApplication1>cl -c /Tc ConsoleApplication1.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27026.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

ConsoleApplication1.cpp

C:\Users\aballman.GRAMMATECH\source\repos\ConsoleApplication1\ConsoleApplication1>dumpbin /symbols ConsoleApplication1.obj | grep External
008 00000000 SECT3  notype       External     | x
009 00000004 UNDEF  notype       External     | y
00A 00000004 SECT3  notype       External     | z
00D 00000000 SECT4  notype ()    External     | main

After commenting out the declaration and use of y and recompiling in C++ mode:

C:\Users\aballman.GRAMMATECH\source\repos\ConsoleApplication1\ConsoleApplication1>dumpbin /symbols ConsoleApplication1.obj | grep External
008 00000000 SECT3  notype       External     | ?z@@3HB (int const z)
00B 00000000 SECT4  notype ()    External     | main

test/Sema/dllexport-1.cpp

2 ↗

(On Diff #185541)

Can remove some extra whitespace before -fsyntax-only (same is true in the other test as well).

[zahiraam]

In D45978#1387385, @aaron.ballman wrote:

Can you add tests for C mode as well, as it seems the behavior differs there.

Aaron,

Let me know if that is enough. Thanks.

[aaron.ballman]

It looks like the patch got mucked up somehow, I only see three testing files in the patch now?

test/Sema/dllexport.c

169

Nothing runs FileCheck in this test, so this isn't checking anything. That should be tested within CodeGen instead.

[zahiraam]

In D45978#1392855, @aaron.ballman wrote:

It looks like the patch got mucked up somehow, I only see three testing files in the patch now?

Oops! Sorry about that.

[aaron.ballman]

test/Sema/dllexport-1.c

1–4 ↗

(On Diff #186275)

This test should live in CodeGen not Sema.

8 ↗

(On Diff #186275)

Are x and z also exported as expected?

test/Sema/dllexport-1.cpp

4 ↗

(On Diff #186275)

This test has no FileCheck line.

The goal here is for the tests in Sema to test the semantic behavior (that warnings are issued when expected and not issued when not expected) and to add tests in CodeGen to test the generated code (are things properly exported, are things we don't expect to be exported not exported, etc). I think you should split your tests accordingly rather than try to verify too much at once.

[zahiraam]

test/Sema/dllexport-1.c

8 ↗

(On Diff #186275)

Only x and y are exported.

@x = dso_local dllexport constant i32 3, align 4
@z = dso_local constant i32 4, align 4
@y = common dso_local dllexport global i32 0, align 4

But then if I take this simple case:
extern int const z = 4;

int main() {

int a = z + 2;
return a;

}
ksh-3.2$ clang -c test3.c
test3.c:1:18: warning: 'extern' variable has an initializer [-Wextern-initializer]
extern int const z = 4;

^

1 warning generated.
ksh-3.2$ dumpbin /symbols test3.o | grep External
00F 00000000 SECT1 notype () External | main
010 00000000 SECT5 notype External | z
ksh-3.2$
When emitting the IR, z is declared as a local constant (not exported):
@z = dso_local constant i32 4, align 4

[aaron.ballman]

test/Sema/dllexport-1.c

1 ↗

(On Diff #186275)

Do we need this many RUN lines? Also, why the different language modes? Also, do you need to use < in the command?

8 ↗

(On Diff #186275)

Okay, please add the expected CHECK lines for x and z to the test. Thank you for clarifying!

test/Sema/dllexport-1.cpp

2 ↗

(On Diff #186275)

I don't think we need this RUN line. I don't know if we need the triple in the previous RUN line either.

4 ↗

(On Diff #186275)

This CHECK line still has no impact; I think it should be removed from the test.

test/Sema/dllexport-2.cpp

2 ↗

(On Diff #186275)

I don't think we need this RUN line. I don't know if we need the triple in the previous RUN line either.

[aaron.ballman]

Oops, sorry, I think I added some comments to a previous revision of the review. To recap:

1. Remove the CHECK line from test/Sema/dllexport-1.cpp
2. Remove the second RUN line from test/Sema/dllexport-1.cpp and test/Sema/dllexport-2.cpp
3. Possibly remove the triples from the RUN line in test/Sema/dllexport-1.cpp and test/Sema/dllexport-2.cpp

[zahiraam]

[aaron.ballman]

LGTM!

[thakis]

lib/Sema/SemaDecl.cpp

10964

Even in unnamed namespaces?

[aaron.ballman]

lib/Sema/SemaDecl.cpp

10964

That would definitely be good to test.

[zahiraam]

lib/Sema/SemaDecl.cpp

10964

It looks like not.

ksh-3.2$ cat test4.cpp
namespace {
__declspec(dllexport) int const x = 3;
}

int main ()
{

int y = x + 10;

return y;

}

ksh-3.2$ cl -c test4.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24210 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

test4.cpp
ksh-3.2$ dumpbin /symbols test4.obj | grep External
008 00000000 SECT3 notype () External | main
ksh-3.2$

ksh-3.2$ clang -c test4.cpp
test4.cpp:2:33: error: '(anonymous namespace)::x' must have external linkage when declared 'dllexport'
__declspec(dllexport) int const x = 3;

^

1 error generated.
ksh-3.2$

So the diag shouldn't go off when the variable is in an anonymous namespace? Do you agree?

[aaron.ballman]

lib/Sema/SemaDecl.cpp

10964

So the diag shouldn't go off when the variable is in an anonymous namespace? Do you agree?

Correct.

test/Sema/dllexport-2.cpp

5 ↗

(On Diff #186641)

Another test I'd like to see is using a typedef to apply the const qualification. e.g.,

typedef const int CInt;
__declspec(dllexport) CInt j2;
__declspec(dllexport) CInt j3 = 3;

We should match the MSVC behavior here as well (I wonder if your use of isLocalConstQualified() should actually be isConstQualified() instead).

[zahiraam]

Let's see if I have included every thing mentioned. Thanks.

[aaron.ballman]

lib/Sema/SemaDecl.cpp

5985

s/NULL/nullptr

Also, I think this should be const NamespaceDecl *.

5987

Does only the immediate declaration context matter? How is this handled?

namespace {
namespace named {
  __declspec(dllexport) int const x = 3; 
}
}

5989–5991

This used to unconditionally warn, but now has predicates -- should this still warn when not in MSVC mode?

[aaron.ballman]

lib/Sema/SemaDecl.cpp

5985

This can be lowered into the below if statement.

5986–5987

is -> Is per our naming convention rules.

5992

No need to test that NS is nonnull here; already done as part of the while loop predicate. Also, you can call NamespaceDecl::isAnonymousNamespace() rather than manually try to test the declared identifier.

I think a more clear way to write this is:

if (VD) {
  const NamespaceDecl *NS = dyn_cast<NamespaceDecl>(VD->getDeclContext());
  while (NS && !IsAnonymousNS) {
    IsAnonymousNS = NS->isAnonymousNamespace();
    NS = dyn_cast<NamespaceDecl>(NS->getParent());
  }
  ...
}

5997–5999

This is a pretty complicated predicate; I'd simplify it a bit and then add some comments to explain it further. The comments from line 5967 could move down here (with modification), in fact. Untested:

bool AnonNSInMicrosoftMode = IsAnonymous && IsMicrosoft;
if ((ND.isExternallyVisible() && AnonNSInMicrosoftMode) ||
    (!AnonNSInMicrosoftMode &&
     (!ND.isExternallyVisible() || VD->isStaticLocal()))) {
  ...
}

test/Sema/dllexport-1.cpp

7 ↗

(On Diff #189659)

I am almost certain that expected-no-diagnostics applies to the entire file, so the @ doesn't make sense. I think you just need one of these for the entire file:

#ifdef MSVC
// expected-no-diagnostics
#endif

(Note, I switched it to use #ifdef as well.)

test/Sema/dllexport-2.cpp

6 ↗

(On Diff #189659)

Please switch to #ifdef rather than #if.

28 ↗

(On Diff #189659)

This isn't correct -- had the typo not been there, then the test would have failed because the file has diagnostics in MSVC mode. This should read:

#ifndef MSVC
// expected-warning@+2 {{__declspec attribute 'dllexport' is not supported}}
#endif

test/SemaCXX/dllexport.cpp

73 ↗

(On Diff #189659)

Typo, but this is the wrong construct. Should be:

#ifndef MS
namespace {
  __declspec(dllexport) int InternalGlobal; // expected-error{{anonymous namespace)::InternalGlobal' must have external linkage when declared 'dllexport'}}
}
#endif

133 ↗

(On Diff #189659)

Similar here as above.

test/SemaCXX/dllimport.cpp

125 ↗

(On Diff #189659)

Here too.

222 ↗

(On Diff #189659)

And here.

[zahiraam]

lib/Sema/SemaDecl.cpp

5989–5991

Please see new version of code.

[aaron.ballman]

I think we're pretty close! Some of the testing code can be cleaned up, but I could also use some help verifying that we're correctly matching the behavior of GCC as well.

test/Sema/dllexport-2.cpp

11 ↗

(On Diff #189843)

I think the pattern to be used here is:

#ifdef MSVC
// expected-error@+4 {{'j' must have external linkage when declared 'dllexport'}}
#else
// expected-warning@+2 {{__declspec attribute 'dllexport' is not supported}}
#endif
__declspec(dllexport) int const j; // expected-error {{default initialization of an object of const type 'const int'}}

25 ↗

(On Diff #189843)

Same here as above.

test/SemaCXX/dllexport.cpp

72–74 ↗

(On Diff #189843)

I don't have a copy of mingw handy -- can you test that this behavior matches the behavior when targeting mingw32 with GCC? Might also be a good idea to test cygwin as a target as well. I would have thought that they would behave the same as MSVC, but I can't easily test it myself.

129 ↗

(On Diff #189843)

Same question here as above.

test/SemaCXX/dllimport.cpp

124 ↗

(On Diff #189843)

Likewise here.

218 ↗

(On Diff #189843)

and here

[zahiraam]

test/SemaCXX/dllexport.cpp

72–74 ↗

(On Diff #189843)

Tested it with a set of combinations. The test is passing...

[aaron.ballman]

LGTM!

[zahiraam]

In D45978#1431057, @aaron.ballman wrote:

LGTM!

Would you mind committing the changes please? Thanks.

[aaron.ballman]

Would you mind committing the changes please? Thanks.

Happy to do so! I've committed in r356458.

lib/Sema/SemaDecl.cpp

if (isa<FunctionDecl>(ND) || !ND.isExternallyVisible()) {
       ND.dropAttr<SelectAnyAttr>();
     }
   }
 
   if (const InheritableAttr *Attr = getDLLAttr(&ND)) {
     // dll attributes require external linkage. Static locals may have external
     // linkage but still cannot be explicitly imported or exported.
     auto *VD = dyn_cast<VarDecl>(&ND);
     if (!ND.isExternallyVisible() || (VD && VD->isStaticLocal())) {
       S.Diag(ND.getLocation(), diag::err_attribute_dll_not_extern)
         << &ND << Attr;
       ND.setInvalidDecl();
     }
   }
 
   // Virtual functions cannot be marked as 'notail'.
   if (auto *Attr = ND.getAttr<NotTailCalledAttr>())
     if (auto *MD = dyn_cast<CXXMethodDecl>(&ND))
       if (MD->isVirtual()) {
         S.Diag(ND.getLocation(),
                diag::err_invalid_attribute_on_virtual_function)
             << Attr;
         ND.dropAttr<NotTailCalledAttr>();
       }
 }
 
 static void checkDLLAttributeRedeclaration(Sema &S, NamedDecl *OldDecl,
                                            NamedDecl *NewDecl,
                                            bool IsSpecialization,
                                            bool IsDefinition) {
   if (OldDecl->isInvalidDecl() || NewDecl->isInvalidDecl())
if (VDecl->isStaticDataMember() && VDecl->getCanonicalDecl()->hasInit()) {
       Diag(Init->getExprLoc(), diag::err_static_data_member_reinitialization)
           << VDecl->getDeclName();
       Diag(VDecl->getCanonicalDecl()->getInit()->getExprLoc(),
            diag::note_previous_initializer)
           << 0;
       return;
     }
 
+    //In Microsoft C++ mode a const variables defined in namespace scope has external
+    // linkage by default if the variable is declared with __declspec(dllexport).
+    if (getLangOpts().CPlusPlus && getLangOpts().MSVCCompat &&
+        VDecl->getType().isLocalConstQualified() &&
+        VDecl->hasAttr<DLLExportAttr>() &&
+        VDecl->getDefinition()) {
+      VDecl->setStorageClass(SC_Extern);
+    }
+
     if (VDecl->hasLocalStorage())
       setFunctionHasBranchProtectedScope();
 
     if (DiagnoseUnexpandedParameterPack(Init, UPPC_Initializer)) {
       VDecl->setInvalidDecl();
       return;
     }
   }
if (VDecl->isLocalVarDecl()) {
     // C++ rules.
     if (VDecl->getStorageClass() == SC_Extern &&
         ((!getLangOpts().CPlusPlus && !VDecl->hasAttr<SelectAnyAttr>()) ||
          !Context.getBaseElementType(VDecl->getType()).isConstQualified()) &&
         !(getLangOpts().CPlusPlus && VDecl->isExternC()) &&
         !isTemplateInstantiation(VDecl->getTemplateSpecializationKind()))
       Diag(VDecl->getLocation(), diag::warn_extern_init);
 
     // C99 6.7.8p4. All file scoped initializers need to be constant.
     if (!getLangOpts().CPlusPlus && !VDecl->isInvalidDecl())
       CheckForConstantInitializer(Init, DclT);
   }
 
   // We will represent direct-initialization similarly to copy-initialization:
   //    int x(1);  -as-> int x = 1;
   //    ClassType x(a,b,c); -as-> ClassType x = ClassType(a,b,c);
   //

test/Sema/dllexport.c

 // Export in local scope.
 void functionScope() {
   __declspec(dllexport)        int LocalVarDecl; // expected-error{{'LocalVarDecl' must have external linkage when declared 'dllexport'}}
   __declspec(dllexport)        int LocalVarDef = 1; // expected-error{{'LocalVarDef' must have external linkage when declared 'dllexport'}}
   __declspec(dllexport) extern int ExternLocalVarDecl;
   __declspec(dllexport) static int StaticLocalVar; // expected-error{{'StaticLocalVar' must have external linkage when declared 'dllexport'}}
 }
 
-
+// const variables
+__declspec(dllexport) int const x = 3;
 
 //===----------------------------------------------------------------------===//
 // Functions
 //===----------------------------------------------------------------------===//
 
 // Export function declaration. Check different placements.
 __attribute__((dllexport)) void decl1A(); // Sanity check with __attribute__
 __declspec(dllexport)      void decl1B();

 
 void __attribute__((dllexport, dllimport))       precedence2A() {} // expected-warning{{'dllimport' attribute ignored}}
 void __declspec(dllexport) __declspec(dllimport) precedence2B() {} // expected-warning{{'dllimport' attribute ignored}}
 
 void __declspec(dllimport) precedenceRedecl1(); // expected-warning{{'dllimport' attribute ignored}}
 void __declspec(dllexport) precedenceRedecl1() {}
 
 void __declspec(dllexport) precedenceRedecl2();
 void __declspec(dllimport) precedenceRedecl2() {} // expected-warning{{'dllimport' attribute ignored}}