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

Fixed shared windows builds of test targets #1445

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

h-vetinari
Copy link
Contributor

Regarding #1407

@derekmauro
Copy link
Member

How was this tested? It doesn't appear to work.

T:\src\github\abseil-cpp\absl/strings/internal/cordz_handle.h(111,32): error C2220: the following warning is treated as an error [T:\src\github\abseil-cpp\build\absl\abseil_dll.vcxproj]
T:\src\github\abseil-cpp\absl/strings/internal/cordz_handle.h(111,32): warning C4251: 'absl::cord_internal::CordzHandle::global_queue_': struct 'absl::cord_internal::CordzHandle::Queue' needs to have dll-interface to be used by clients of class 'absl::cord_internal::CordzHandle'

I'm guessing it is a simple fix to add ABSL_DLL to the variable in the error message.

@h-vetinari
Copy link
Contributor Author

h-vetinari commented May 8, 2023

the following warning is treated as an error

There's your answer. 😅

I didn't build with (the equivalent of) -Werror. As you say, it's most likely a couple ABSL_DLL markers that need to be added.

Without this PR. compilation fails hard. So we can say the patches here are necessary but not sufficient.

@h-vetinari
Copy link
Contributor Author

h-vetinari commented May 9, 2023

I'm guessing it is a simple fix to add ABSL_DLL to the variable in the error message.

Adding ABSL_DLL to cord_internal::CordzHandle::Queue leads to:

absl/strings/internal/cordz_handle.h(89): warning C4251: 'absl::lts_20230125::cord_internal::CordzHandle::Queue::mutex': class 'absl::lts_20230125::base_internal::SpinLock' needs to have dll-interface to be used by clients of struct 'absl::lts_20230125::cord_internal::CordzHandle::Queue'
absl/strings/internal/cordz_handle.h(90): warning C4251: 'absl::lts_20230125::cord_internal::CordzHandle::Queue::dq_tail': struct 'std::atomic<absl::lts_20230125::cord_internal::CordzHandle *>' needs to have dll-interface to be used by clients of struct 'absl::lts_20230125::cord_internal::CordzHandle::Queue'

Further adding ABSL_DLL to dq_tail then leads to:

absl/strings/internal/cordz_handle.h(90): error C2071: 'absl::lts_20230125::cord_internal::CordzHandle::Queue::dq_tail': illegal storage class
absl/strings/internal/cordz_handle.h(90): warning C4251: 'absl::lts_20230125::cord_internal::CordzHandle::Queue::dq_tail': struct 'std::atomic<absl::lts_20230125::cord_internal::CordzHandle *>' needs to have dll-interface to be used by clients of struct 'absl::lts_20230125::cord_internal::CordzHandle::Queue'

The second warning is actually interesting here because it talks about a struct, whereas the definition is:

std::atomic<CordzHandle*> dq_tail ABSL_GUARDED_BY(mutex){nullptr};

I presume the ABSL_GUARDED_BY annotation may actually turn that whole thing into a struct behind the scenes to carry information about the mutex? No idea how to declare that struct with ABSL_DLL in a way that preserves (the windows equivalent of) __attribute__((guarded_by(x)))

@derekmauro
Copy link
Member

I have a different fix for this that I am testing. It involves not exposing some things in headers to avoid the need for ABSL_DLL. Hopefully it will make it to GitHub soon.

@h-vetinari
Copy link
Contributor Author

I have a different fix for this that I am testing. It involves not exposing some things in headers to avoid the need for ABSL_DLL. Hopefully it will make it to GitHub soon.

That's great news, thanks a lot! Could you let us know here once that happens please? :)

copybara-service bot pushed a commit that referenced this pull request May 12, 2023
This is a heavily modified version of
#1445,
which adds some missing test libraries to the test DLL.

Unlike #1445, this change moves several global variables out of
headers that did not need to be in headers.

For instance, cord_btree_exhaustive_validation was a global
defined/declared in cord_internal, but only used in cord_rep_btree
and its test.

cordz_handle defined a queue in its header even though it wasn't needed,
which also led to ODR problems.

The Spinlock used in CordzHandle is replaced with a Mutex. This was
originally a Mutex, but Chromium asked us to change it to a Spinlock
to avoid a static initializer. After this change, the static
initializer is no longer an issue.

#1407

PiperOrigin-RevId: 531516991
Change-Id: I0e431a193698b20ba03fac6e414c26f153f330a7
@derekmauro
Copy link
Member

Please give 3aa3377 a try. I think either fixes your issue or gets us closer to a fix. It's possible some some ABSL_DLL annotations are still required.

@h-vetinari
Copy link
Contributor Author

h-vetinari commented May 13, 2023

Please give 3aa3377 a try. I think either fixes your issue or gets us closer to a fix. It's possible some some ABSL_DLL annotations are still required.

Thanks! Giving this a shot in conda-forge/abseil-cpp-feedstock#60.

@h-vetinari
Copy link
Contributor Author

h-vetinari commented May 13, 2023

Please give 3aa3377 a try. I think either fixes your issue or gets us closer to a fix. It's possible some some ABSL_DLL annotations are still required.

Thanks! Giving this a shot in conda-forge/abseil-cpp-feedstock#60.

Back to failing with the missing vftable, which I had been battling in conda-forge/abseil-cpp-feedstock#58 as well, and which ended up being "solved" by adding ABSL_DLL to the class.

[370/447] Linking CXX executable bin\absl_cordz_handle_test.exe
FAILED: bin/absl_cordz_handle_test.exe 
cmd.exe /C "cd . && D:\bld\abseil-split_1683936617505\_build_env\Library\bin\cmake.exe -E vs_link_exe --intdir=absl\strings\CMakeFiles\absl_cordz_handle_test.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1429~1.301\bin\HostX64\x64\link.exe /nologo absl\strings\CMakeFiles\absl_cordz_handle_test.dir\internal\cordz_handle_test.cc.obj  /out:bin\absl_cordz_handle_test.exe /implib:absl\strings\absl_cordz_handle_test.lib /pdb:bin\absl_cordz_handle_test.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console  absl\abseil_test_dll.lib  D:\bld\abseil-split_1683936617505\_h_env\Library\lib\gmock_main.lib  absl\abseil_dll.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && cd ."
LINK: command "C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1429~1.301\bin\HostX64\x64\link.exe /nologo absl\strings\CMakeFiles\absl_cordz_handle_test.dir\internal\cordz_handle_test.cc.obj /out:bin\absl_cordz_handle_test.exe /implib:absl\strings\absl_cordz_handle_test.lib /pdb:bin\absl_cordz_handle_test.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console absl\abseil_test_dll.lib D:\bld\abseil-split_1683936617505\_h_env\Library\lib\gmock_main.lib absl\abseil_dll.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:bin\absl_cordz_handle_test.exe.manifest" failed (exit code 1120) with the following output:
cordz_handle_test.cc.obj : error LNK2019: unresolved external symbol "const absl::lts_20230125::cord_internal::CordzHandle::`vftable'" (??_7CordzHandle@cord_internal@lts_20230125@absl@@6B@) referenced in function "public: __cdecl absl::lts_20230125::cord_internal::CordzHandle::CordzHandle(void)" (??0CordzHandle@cord_internal@lts_20230125@absl@@QEAA@XZ)

bin\absl_cordz_handle_test.exe : fatal error LNK1120: 1 unresolved externals

@derekmauro
Copy link
Member

I pulled in #1115 with the missing ABSL_DLL. Is there anything left to do?

@h-vetinari
Copy link
Contributor Author

I pulled in #1115 with the missing ABSL_DLL. Is there anything left to do?

Thanks! The build based on your commit did already include (the equivalent of) #1115, so I'm pretty sure it's still not working properly. Though I admit I don't understand what might be the pertinent differences between our respective build setups.

netkex pushed a commit to netkex/abseil-cpp that referenced this pull request Apr 3, 2024
This is a heavily modified version of
abseil#1445,
which adds some missing test libraries to the test DLL.

Unlike abseil#1445, this change moves several global variables out of
headers that did not need to be in headers.

For instance, cord_btree_exhaustive_validation was a global
defined/declared in cord_internal, but only used in cord_rep_btree
and its test.

cordz_handle defined a queue in its header even though it wasn't needed,
which also led to ODR problems.

The Spinlock used in CordzHandle is replaced with a Mutex. This was
originally a Mutex, but Chromium asked us to change it to a Spinlock
to avoid a static initializer. After this change, the static
initializer is no longer an issue.

abseil#1407

PiperOrigin-RevId: 531516991
Change-Id: I0e431a193698b20ba03fac6e414c26f153f330a7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants