Fixed shared windows builds of test targets by h-vetinari · Pull Request #1445 · abseil/abseil-cpp

h-vetinari · 2023-05-08T02:48:37Z

Regarding #1407

derekmauro · 2023-05-08T20:10:55Z

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 · 2023-05-08T21:39:58Z

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 · 2023-05-09T04:01:57Z

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 · 2023-05-11T17:09:48Z

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 · 2023-05-11T22:04:45Z

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? :)

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 · 2023-05-12T15:53:45Z

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 · 2023-05-13T00:05:17Z

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 · 2023-05-13T01:10:22Z

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 · 2023-05-15T20:03:35Z

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

h-vetinari · 2023-05-15T21:03:51Z

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.

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

This is a heavily modified version of abseil/abseil-cpp#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

h-vetinari added 3 commits May 8, 2023 13:47

add some necessary source files to ABSL_INTERNAL_TEST_DLL_FILES

0702662

update ABSL_CC_TEST_LINKOPTS when using shared build

23bbcc4

make CordzHandle and relevant internal state use ABSL_DLL

9e87faa

h-vetinari mentioned this pull request May 8, 2023

[Bug]: TESTONLY targets don't appear to get installed #1407

Open

derekmauro added the kokoro:run label May 8, 2023

kokoro-team removed the kokoro:run label May 8, 2023

h-vetinari mentioned this pull request May 13, 2023

Backport updated upstream fix for shared windows builds including test deps. conda-forge/abseil-cpp-feedstock#60

Open

h-vetinari mentioned this pull request Jun 6, 2023

[Build] WIP: Shared builds on windows grpc/grpc#33175

Merged

b7r3333 approved these changes Sep 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed shared windows builds of test targets#1445

Fixed shared windows builds of test targets#1445
h-vetinari wants to merge 3 commits intoabseil:masterfrom
h-vetinari:test_dll

h-vetinari commented May 8, 2023

Uh oh!

derekmauro commented May 8, 2023

Uh oh!

h-vetinari commented May 8, 2023 •

edited

Loading

Uh oh!

h-vetinari commented May 9, 2023 •

edited

Loading

Uh oh!

derekmauro commented May 11, 2023

Uh oh!

h-vetinari commented May 11, 2023

Uh oh!

derekmauro commented May 12, 2023

Uh oh!

h-vetinari commented May 13, 2023 •

edited

Loading

Uh oh!

h-vetinari commented May 13, 2023 •

edited

Loading

Uh oh!

derekmauro commented May 15, 2023

Uh oh!

h-vetinari commented May 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

h-vetinari commented May 8, 2023

Uh oh!

derekmauro commented May 8, 2023

Uh oh!

h-vetinari commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

h-vetinari commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekmauro commented May 11, 2023

Uh oh!

h-vetinari commented May 11, 2023

Uh oh!

derekmauro commented May 12, 2023

Uh oh!

h-vetinari commented May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

h-vetinari commented May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekmauro commented May 15, 2023

Uh oh!

h-vetinari commented May 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

h-vetinari commented May 8, 2023 •

edited

Loading

h-vetinari commented May 9, 2023 •

edited

Loading

h-vetinari commented May 13, 2023 •

edited

Loading

h-vetinari commented May 13, 2023 •

edited

Loading