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

[BUG] LNK4217 and LNK4286 warnings when linking //:benchmark_main #1372

Closed
junyer opened this issue Mar 15, 2022 · 9 comments · Fixed by #1373
Closed

[BUG] LNK4217 and LNK4286 warnings when linking //:benchmark_main #1372

junyer opened this issue Mar 15, 2022 · 9 comments · Fixed by #1373

Comments

@junyer
Copy link
Contributor

junyer commented Mar 15, 2022

…
INFO: From Linking regexp_benchmark.exe:
   Creating library bazel-out\x64_windows-dbg\bin\regexp_benchmark.lib and object bazel-out\x64_windows-dbg\bin\regexp_benchmark.exp
LINK : warning LNK4217: symbol '?RegisterBenchmarkInternal@internal@benchmark@@YAPEAVBenchmark@12@PEAV312@@Z (class benchmark::internal::Benchmark * __cdecl benchmark::internal::RegisterBenchmarkInternal(class benchmark::internal::Benchmark *))' defined in 'benchmark.lib(benchmark_register.obj)' is imported by 'regexp_benchmark.obj' in function '"void __cdecl re2::`dynamic initializer for 'benchmark_uniq_100_benchmark_''(void)" (??__Ebenchmark_uniq_100_benchmark_@re2@@YAXXZ)'
LINK : warning LNK4217: symbol '?InitializeStreams@internal@benchmark@@YAHXZ (int __cdecl benchmark::internal::InitializeStreams(void))' defined in 'benchmark.lib(benchmark.obj)' is imported by 'regexp_benchmark.obj' in function '"void __cdecl benchmark::internal::`dynamic initializer for 'stream_init_anchor''(void)" (??__Estream_init_anchor@internal@benchmark@@YAXXZ)'
LINK : warning LNK4286: symbol '?InitializeStreams@internal@benchmark@@YAHXZ (int __cdecl benchmark::internal::InitializeStreams(void))' defined in 'benchmark.lib(benchmark.obj)' is imported by 'benchmark_main.lib(benchmark_main.obj)'
…

(The full logs are here.)

@junyer
Copy link
Contributor Author

junyer commented Mar 15, 2022

Both warnings are essentially saying that __declspec(dllimport) is bogus in that linking context, which makes sense to me because the DLL wasn't involved. Does //:benchmark_main need local_defines = ["BENCHMARK_STATIC_DEFINE"] as well?

@junyer
Copy link
Contributor Author

junyer commented Mar 15, 2022

Oh, and I just noticed this too:

…
INFO: From Compiling src/benchmark_main.cc:
external/com_github_google_benchmark/src/benchmark_main.cc(18): warning C4273: 'main': inconsistent dll linkage
external/com_github_google_benchmark/src/benchmark_main.cc(17): note: see previous definition of 'main'
…

What's up with that? O_o

@sergiud
Copy link
Contributor

sergiud commented Mar 16, 2022

I don't know what local_defines does, but when building and consuming a static library version of benchmark, BENCHMARK_EXPORT should never expand to __declspec(dllimport|dllexport) while BENCHMARK_STATIC_DEFINE should be set both when building and consuming benchmark.

To summarize, the following rules apply:

  • Static library:
    • BENCHMARK_STATIC_DEFINE set the during the library build and propagated to dependencies
    • BENCHMARK_EXPORT is empty
    • (benchmark_EXPORTS not relevant)
  • Shared library:
    • benchmark_EXPORTS set only during the build but not propagated to dependencies
    • BENCHMARK_EXPORT expands to __declspec(dllexport) during the build and __declspec(dllimport) when propagated to dependencies

There might be an inconsistency in the way Bazel propagates defines.

@junyer
Copy link
Contributor Author

junyer commented Mar 16, 2022

Do we need Bazel to create the dynamic library? I'm asking because I'm not aware of any way to make Bazel follow such rules, but we can easily stop Bazel from ever creating the dynamic library by setting linkstatic = True on the cc_library target. (The documentation for linkstatic describes this behaviour towards the end.)

@sergiud
Copy link
Contributor

sergiud commented Mar 16, 2022

This is the easy way out. Alternatively, one could build two library variants separately.

@junyer
Copy link
Contributor Author

junyer commented Mar 16, 2022

@dominichamon, do you know whether anyone or anything expects Bazel to create the dynamic library? My experience has been entirely with dynamic libraries (e.g. Python extensions) that are opened at runtime (e.g. by Python), not linked by Bazel during dependent build actions. Unless the Bazel build needs to support a use case such as packaging, erring on the side of simplicity seems preferable for now.

@dmah42
Copy link
Member

dmah42 commented Mar 16, 2022

when building benchmark_main, i think either a dynamic or static library would be appropriate. i don't think there's much of an expectation either way more broadly.

@junyer
Copy link
Contributor Author

junyer commented Mar 16, 2022

Posted #1373 for review. :)

dmah42 pushed a commit that referenced this issue Mar 17, 2022
If someone or something ever needs the dynamic library as a Bazel build
artifact, we can figure that out for them then, but right now, there is
no strong reason to be wrangling various `export.h`-controlling macros.

Fixes #1372.
@junyer
Copy link
Contributor Author

junyer commented Mar 17, 2022

After following up with #1374, I reran the CI job for RE2 and it's now showing zero warnings related to the benchmark library across Linux, macOS and Windows. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants