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]: ABSL_DECLARE_FLAG can't work with MSVC __declspec user MACRO #1817

Open
Mizux opened this issue Jan 31, 2025 · 1 comment
Open

[Bug]: ABSL_DECLARE_FLAG can't work with MSVC __declspec user MACRO #1817

Mizux opened this issue Jan 31, 2025 · 1 comment

Comments

@Mizux
Copy link
Contributor

Mizux commented Jan 31, 2025

Describe the issue

When build a shared lib on windows you usually need a dllspec macro (like ABSL_CONSUME_DLL/ABSL_DLL), unfortunately the current MACRO implem of ABSL_DECLARE_FLAG() do not support this use case...

e.g. if we try to prepend the MACRO...
foo.h

#include "absl/flags/declare.h"
...
#if defined(_MSC_VER) && defined(FOO_BUILD_DLL)
// Annoying stuff for windows -- makes sure clients can import these functions
#if defined(FOO_EXPORT)
#define FOO_DLL __declspec(dllexport)
#else
#define FOO_DLL __declspec(dllimport)
#endif  // defined(FOO_EXPORT)
#endif  // _MSC_VER && FOO_BUILD_DLL

#ifndef FOO_DLL
#define FOO_DLL
#endif

FOO_DLL ABSL_DECLARE_FLAG(bool, foo_bool);
FOO_DLL ABSL_DECLARE_FLAG(int, foo_int);
FOO_DLL ABSL_DECLARE_FLAG(int64_t, foo_long_int);
FOO_DLL ABSL_DECLARE_FLAG(std::string, foo_string);

with CMakeLists.txt:

set(BUILD_SHARED_LIBS ON)
...
add_library(foo)
...
if(MSVC AND BUILD_SHARED_LIBS)
  target_compile_definitions(foo PUBLIC "FOO_BUILD_DLL")
  target_compile_definitions(foo PRIVATE "FOO_EXPORT")
endif()

and later in an app.exe's CMakeLists.txt

add_executable(app)
...
target_link_libraries(app PRIVATE foo)

Since the ABSL_DECLARE_FLAG_INTERNAL implementation declares two time the flags (line 63 and 66)

#define ABSL_DECLARE_FLAG_INTERNAL(type, name) \
extern absl::Flag<type> FLAGS_##name; \
namespace absl /* block flags in namespaces */ {} \
/* second redeclaration is to allow applying attributes */ \
extern absl::Flag<type> FLAGS_##name

we can't prepand with our FOO_DLL nor we can provide it to the macro...

This regression aka double declaration has been introduced in:
1065514#diff-252f54025d208b3afe13aa8d62762bb88282ad00027be08396105994d31f813f

Steps to reproduce the problem

You have a complete working/borken example here. just test on windows with a cmake based build and play with the #if 0/1 to enable my custom macro fix or show the issue...
relevant src: https://github.com/Mizux/absl-cxx/blob/715fdb721515248b8b8c047325f78d46f7dfc0c8/foo/foo.hpp#L25-L41

On any windows git bash with MSVC and CMake installed.

git clone ... && cd absl-cxx
# optional: to check the generator is MSVC 2022
cmake -G 
cmake -S. -Bbuild
cmake --build build --config Release --target app

Expected: compile + link ok

Observed:
if using FOO_DLL ABSL_DECLARE_FLAG(...) path

C:\Users\mizux\dev\absl-cxx\foo\foo.hpp(43,9): warning C4273: 'FLAGS_foo_bool': inconsistent dll linkage [C:\Users\mizux\dev\absl-cxx\build\app\app.vcxproj]
  (compiling source file '../../app/app.cpp')
      C:\Users\mizux\dev\absl-cxx\foo\foo.hpp(43,9):
      see previous definition of 'FLAGS_foo_bool'
...
  foo.vcxproj -> C:\Users\mizux\dev\absl-cxx\build\Release\bin\foo.dll
app.obj : error LNK2019: unresolved external symbol "class absl::lts_20240722::flags_internal::Flag<bool> FLAGS_foo_bool" (?FLAGS_foo_bool@@3V?$Flag@_N@flags_internal@lts_20240722@absl@@A) referenced in function main [C:\Users\mizux\dev\absl-cxx\build\app\app.vcxproj]
app.obj : error LNK2019: unresolved external symbol "class absl::lts_20240722::flags_internal::Flag<int> FLAGS_foo_int" (?FLAGS_foo_int@@3V?$Flag@H@flags_internal@lts_20240722@absl@@A) referenced in function main [C:\Users\mizux\dev\absl-cxx\build\app\app.vcxproj]
app.obj : error LNK2019: unresolved external symbol "class absl::lts_20240722::flags_internal::Flag<__int64> FLAGS_foo_long_int" (?FLAGS_foo_long_int@@3V?$Flag@_J@flags_internal@lts_20240722@absl@@A) referenced in function main [C:\Users\mizux\dev\absl-cxx\build\app\app.vcxproj]
app.obj : error LNK2019: unresolved external symbol "class absl::lts_20240722::flags_internal::Flag<class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > > FLAGS_foo_string" (?FLAGS_foo_string@@3V?$Flag@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@flags_internal@lts_20240722@absl@@A) referenced in function main [C:\Users\mizux\dev\absl-cxx\build\app\app.vcxproj]
C:\Users\mizux\dev\absl-cxx\build\Release\bin\app.exe : fatal error LNK1120: 4 unresolved externals [C:\Users\mizux\dev\absl-cxx\build\app\app.vcxproj]

What version of Abseil are you using?

20240722.0 with a small patch to check c++ dialect detected IIRC

patch: https://github.com/Mizux/absl-cxx/blob/main/patches/abseil-cpp-20240722.0.patch
note: IIRC the patch was for debian where while we want to compile in C++17 absl figure out that the default compiler support c++20 and do not respect the CMAKE_CXX_STANDARD set before the FetchContent/MakeAvailable. Also a CMake version got a regression where check_cxx_source_compiles didn't capture the CMAKE_CXX_STANDARD....
EDIT: seems related to https://gitlab.kitware.com/cmake/cmake/-/merge_requests/306

ref: https://github.com/Mizux/absl-cxx/blob/715fdb721515248b8b8c047325f78d46f7dfc0c8/cmake/dependencies/CMakeLists.txt#L40-L42

What operating system and version are you using?

MSVC Community 2022 (dllspec issue) gWindows cloudtop and windows 11 (X1 from re-stuff)

What compiler and version are you using?

msvc

What build system are you using?

CMake 3.29.5 (from VS Community 2022)

Additional context

the current workaround we use for google/or-tools is to define new MACRO

// The ABSL_DECLARE_DLL_FLAG(dll, type, name) macro expands to:
//   dll extern absl::Flag<type> FLAGS_name;
#define ABSL_DECLARE_DLL_FLAG(dll, type, name) ABSL_DECLARE_DLL_FLAG_INTERNAL(dll, type, name)

// Internal implementation of ABSL_DECLARE_FLAG to allow macro expansion of its
// arguments. Clients must use ABSL_DECLARE_FLAG instead.
#define ABSL_DECLARE_DLL_FLAG_INTERNAL(dll, type, name)      \
  dll extern absl::Flag<type> FLAGS_##name;                  \
  namespace absl /* block flags in namespaces */ {}          \
  /* second redeclaration is to allow applying attributes */ \
  dll extern absl::Flag<type> FLAGS_##name

usage:

ABSL_DECLARE_DLL_FLAG(FOO_DLL, bool, foo_bool);
ABSL_DECLARE_DLL_FLAG(FOO_DLL, int, foo_int);
ABSL_DECLARE_DLL_FLAG(FOO_DLL, int64_t, foo_long_int);
ABSL_DECLARE_DLL_FLAG(FOO_DLL, std::string, foo_string);

=> Would it be possible to add it to abseil-cpp directly ?

@Mizux
Copy link
Contributor Author

Mizux commented Feb 3, 2025

An other suggestion is to change the implementation of ABSL_DECLARE_FLAG_INTERNAL on windows:

// Internal implementation of ABSL_DECLARE_FLAG to allow macro expansion of its
// arguments. Clients must use ABSL_DECLARE_FLAG instead.
#if defined(_MSC_VER)
  #define ABSL_DECLARE_FLAG_INTERNAL(type, name) \
    extern absl::Flag<type> FLAGS_##name
#else
  #define ABSL_DECLARE_DLL_FLAG_INTERNAL(type, name)      \
    extern absl::Flag<type> FLAGS_##name;                  \
    namespace absl /* block flags in namespaces */ {}          \
    /* second redeclaration is to allow applying attributes */ \
    extern absl::Flag<type> FLAGS_##name
#endif  // _MSC_VER

Here a CL to apply this fix in google3 in //thirdparty... cl/722558570
ref: https://opensource.google/documentation/reference/thirdparty

note: This fix is needed to properly fix:

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

No branches or pull requests

1 participant