Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--features=windows_export_all_symbols does not export all symbols #11622

Closed
vnghia opened this issue Jun 22, 2020 · 4 comments
Closed

--features=windows_export_all_symbols does not export all symbols #11622

vnghia opened this issue Jun 22, 2020 · 4 comments
Labels
area-Windows Windows-specific issues and feature requests team-Rules-CPP Issues for C++ rules type: support / not a bug (process)

Comments

@vnghia
Copy link
Contributor

vnghia commented Jun 22, 2020

Description of the problem / feature request:

--features=windows_export_all_symbols does not export all symbols ( extern variable )

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

// utils.h
extern const int a;
// utils.cpp
const int a = 1;
// main.cpp
std::cout << a; // main.obj : error LNK2001: unresolved external symbol "int const a" (?a@@3HB)

// utils.h
DLL_EXPORT extern const int a;
// utils.cpp
const int a = 1;
// main.cpp
std::cout << a; // ok

What operating system are you running Bazel on?

Windows 10

What's the output of bazel info release?

3.3

Have you found anything relevant by searching the web?

I I had a look at this file https://github.com/bazelbuild/bazel/blob/master/third_party/def_parser/def_parser.cc
I think those lines cause the bug

if (!pSymbolTable->Type && (SectChar & IMAGE_SCN_MEM_WRITE)) {
// Read only (i.e. constants) must be excluded
this->DataSymbols.insert(symbol);
} else {
if (pSymbolTable->Type || !(SectChar & IMAGE_SCN_MEM_READ) ||
(SectChar & IMAGE_SCN_MEM_EXECUTE)) {

Because IMAGE_SCN_MEM_READ variable is not inserted to the TableSymbol

Beside, I found something https://stackoverflow.com/questions/19373061/what-happens-to-global-and-static-variables-in-a-shared-library-when-it-is-dynam which say that the extern global variables are not part of the exported symbols ( but __declspec(dllexport) can export it ? )

Any other information, logs, or outputs that you want to share?

If this is a bug, I am very happy to contribute to fix this bug

@aiuto aiuto added area-Windows Windows-specific issues and feature requests team-Rules-CPP Issues for C++ rules untriaged labels Jun 22, 2020
@Riatre
Copy link

Riatre commented Jun 22, 2020

This might be a bug, depends on how you see it.

Basically, according to MSDN, on Windows, it is required to mark the declaration of the function/class/variable as dllexport on the export side, and dllimport on the consuming side.

I believe --features=windows_export_all_symbols is meant to be similar to CMake's WINDOWS_EXPORT_ALL_SYMBOLS, for easier porting. Unfortunately, both does not work out of the box for global variables. That's because MS toolchain enforces the following constraint: global variables (extern) must be marked as __declspec(dllimport) if it will be imported from other library. So even if your dll does export the global variable, you still have to change your header file to consume it, pretty much defeating the purpose of windows_export_all_symbols. Because of all this, I'm not sure whether it is more "correct" to still export those symbols (like CMake), or don't do it since it cannot be used without code change.

@vnghia
Copy link
Contributor Author

vnghia commented Jun 22, 2020

I think it maybe easier if we add __declspec(dllimport) to the code than have a macro to switch between __declspec(dllimport) and __declspec(dllexport). What do you think ?

@Riatre
Copy link

Riatre commented Jun 22, 2020

Yeah, that makes sense. I'm not sure if you can export a symbol marked as __declspec(dllimport) though.

@vnghia
Copy link
Contributor Author

vnghia commented Jun 23, 2020

I add (Setchar & IMAGE_SNC_MEM_READ) && (Setchar & IMAGE_SCN_MEM_SHARED) and the symbol is still exposed. But I am not sure this is the correct way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Windows Windows-specific issues and feature requests team-Rules-CPP Issues for C++ rules type: support / not a bug (process)
Projects
None yet
Development

No branches or pull requests

4 participants