Page MenuHomePhabricator

dllexport const variables must have external linkage.
ClosedPublic

Authored by zahiraam on Apr 23 2018, 11:48 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rnk added a comment.Apr 24 2018, 10:31 AM

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 updated this revision to Diff 143971.Apr 25 2018, 11:04 AM
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.

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

aaron.ballman added inline comments.Jan 18 2019, 11:28 AM
lib/Sema/SemaDeclAttr.cpp
6488

Formatting and a typo:

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

6490

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 added a comment.Jan 18 2019, 3:16 PM

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

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 marked an inline comment as done.Jan 26 2019, 2:07 AM

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 added a subscriber: majnemer.
aaron.ballman added inline comments.
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?

ormris added a subscriber: ormris.Jan 31 2019, 1:10 PM
rnk added a comment.Jan 31 2019, 6:01 PM

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

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

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 updated this revision to Diff 185541.Feb 6 2019, 6:32 AM

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

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

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

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.

zahiraam updated this revision to Diff 185920.Feb 8 2019, 12:37 AM

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

test/Sema/dllexport.c
170

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

zahiraam updated this revision to Diff 186275.Feb 11 2019, 9:13 AM

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 added inline comments.Feb 11 2019, 10:32 AM
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 updated this revision to Diff 186460.Feb 12 2019, 7:10 AM
zahiraam marked 3 inline comments as done.
zahiraam added a subscriber: z.
zahiraam added inline comments.
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 added inline comments.Feb 12 2019, 8:08 AM
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.

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 updated this revision to Diff 186641.Feb 13 2019, 6:31 AM
zahiraam marked 5 inline comments as done.
This revision is now accepted and ready to land.Feb 13 2019, 6:43 AM
thakis added inline comments.
lib/Sema/SemaDecl.cpp
11370 ↗(On Diff #186641)

Even in unnamed namespaces?

aaron.ballman added inline comments.Feb 13 2019, 7:47 AM
lib/Sema/SemaDecl.cpp
11370 ↗(On Diff #186641)

That would definitely be good to test.

zahiraam marked an inline comment as done.Feb 14 2019, 2:22 AM
zahiraam added inline comments.
lib/Sema/SemaDecl.cpp
11370 ↗(On Diff #186641)

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 added inline comments.Feb 15 2019, 10:39 AM
lib/Sema/SemaDecl.cpp
11370 ↗(On Diff #186641)

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 updated this revision to Diff 188047.Feb 23 2019, 6:37 AM
zahiraam marked 2 inline comments as done.

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

aaron.ballman added inline comments.Feb 27 2019, 6:13 PM
lib/Sema/SemaDecl.cpp
5971 ↗(On Diff #188047)

s/NULL/nullptr

Also, I think this should be const NamespaceDecl *.

5973 ↗(On Diff #188047)

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

namespace {
namespace named {
  __declspec(dllexport) int const x = 3; 
}
}
5975–5977 ↗(On Diff #188047)

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

zahiraam updated this revision to Diff 189659.Mar 7 2019, 12:31 AM
zahiraam marked 3 inline comments as done.
aaron.ballman added inline comments.Mar 7 2019, 6:55 AM
lib/Sema/SemaDecl.cpp
5970 ↗(On Diff #189659)

This can be lowered into the below if statement.

5971–5972 ↗(On Diff #189659)

is -> Is per our naming convention rules.

5977 ↗(On Diff #189659)

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());
  }
  ...
}
5982–5984 ↗(On Diff #189659)

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 updated this revision to Diff 189843.Mar 8 2019, 4:35 AM
zahiraam marked 10 inline comments as done.
zahiraam added inline comments.
lib/Sema/SemaDecl.cpp
5975–5977 ↗(On Diff #188047)

Please see new version of code.

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 marked 6 inline comments as done.Mar 11 2019, 10:52 AM
zahiraam added inline comments.
test/SemaCXX/dllexport.cpp
72–74 ↗(On Diff #189843)

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

zahiraam updated this revision to Diff 190124.Mar 11 2019, 10:54 AM
zahiraam marked an inline comment as done.

LGTM!

Would you mind committing the changes please? Thanks.

aaron.ballman closed this revision.Mar 19 2019, 7:52 AM

Would you mind committing the changes please? Thanks.

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