[PATCH] D45978: dllexport const variables must have external linkage.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: majnemer.
aaron.ballman added a subscriber: majnemer.
aaron.ballman added inline comments.


================
Comment at: test/Sema/dllexport.c:70
+// const variables
+__declspec(dllexport) int const x = 3;
 
----------------
zahiraam wrote:
> aaron.ballman wrote:
> > 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.
> 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.
> 
> 
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D45978/new/

https://reviews.llvm.org/D45978