getml / reflect-cpp

A C++20 library for fast serialization, deserialization and validation using reflection. Supports JSON, BSON, CBOR, flexbuffers, msgpack, TOML, XML, YAML / msgpack.org[C++20]
https://getml.github.io/reflect-cpp/
MIT License
821 stars 66 forks source link

Reflection for structs in an anoynoumus namespace results in compilation errors #46

Closed Kicer86 closed 6 months ago

Kicer86 commented 6 months ago

When rfl is being used on a struct in an anonymous namespace, gcc complains at linkage stage with errors like:

/usr/bin/ld: src/.../tests.cpp.o: warning: relocation against `_ZN3rfl8internal11fake_objectIN12_GLOBAL__N_13ABCEEE' in read-only section `.text'
/usr/bin/ld: src/.../tests.cpp.o: in function `auto rfl::internal::get_field_names<(anonymous namespace)::DEF>()':
/home/...//include/rfl/internal/../internal/get_field_names.hpp:120:(.text+0x2950): undefined reference to `rfl::internal::fake_object<(anonymous namespace)::DEF>'
/usr/bin/ld: /home/.../include/rfl/internal/../internal/get_field_names.hpp:120:(.text+0x295b): undefined reference to `rfl::internal::fake_object<(anonymous namespace)::DEF>'
/usr/bin/ld: src/.../tests.cpp.o: in function `auto rfl::internal::get_field_names<(anonymous namespace)::ABC>()':
/home/.../include/rfl/internal/../internal/get_field_names.hpp:120:(.text+0x33fb): undefined reference to `rfl::internal::fake_object<(anonymous namespace)::ABC>'
/usr/bin/ld: /home/.../include/rfl/internal/../internal/get_field_names.hpp:120:(.text+0x3406): undefined reference to `rfl::internal::fake_object<(anonymous namespace)::ABC>'
/usr/bin/ld: /home/.../include/rfl/internal/../internal/get_field_names.hpp:120:(.text+0x3411): undefined reference to `rfl::internal::fake_object<(anonymous namespace)::ABC>'
/usr/bin/ld: /home/.../include/rfl/internal/../internal/get_field_names.hpp:120:(.text+0x341c): undefined reference to `rfl::internal::fake_object<(anonymous namespace)::ABC>'
/usr/bin/ld: /home/.../include/rfl/internal/../internal/get_field_names.hpp:120:(.text+0x3427): undefined reference to `rfl::internal::fake_object<(anonymous namespace)::ABC>'
/usr/bin/ld: src/.../tests.cpp.o:/home/.../include/rfl/internal/../internal/get_field_names.hpp:120: more undefined references to `rfl::internal::fake_object<(anonymous namespace)::ABC>' follow
/usr/bin/ld: warning: creating DT_TEXTREL in a PIE
collect2: error: ld returned 1 exit status

and clang throws erros at compilation time:

/home/.../include/rfl/internal/fake_object.hpp:9:10: error: variable 'rfl::internal::fake_object<(anonymous namespace)::DEF>' is used but not defined in this translation unit, and cannot be defined in any other translation unit because its type does not have linkage
schaumb commented 6 months ago

This issue exists because the current fake_object implementation is unusable with a non-external linkage class.

The solution is relatively simple. This implementation was the first version, and I found this bug myself too.

liuzicheng1987 commented 6 months ago

@schaumb , you are right, of course. Thanks for the input on how to fix it. By the way, I love your work on Boost.PFR.

schaumb commented 6 months ago

duplicated #15

liuzicheng1987 commented 6 months ago

@Kicer86 , I have tried this with GCC 11.4 and remarkably I cannot even reproduce the problem. It compiles and runs through just fine.

However, I can reproduce the issue with Clang-16.

liuzicheng1987 commented 6 months ago

I can confirm that the fix proposed by @schaumb resolves the issue.

However, I am a bit concerned that we are exploiting a bug here. Classes and structs inside unnamed namespaces are internally linked - you should not be able to externally link them.

Maybe the solution is to just come up with a better error message at compile time.

liuzicheng1987 commented 6 months ago

If have now added a static assertion, refer to c71e4ca11c1c953b249de44caf2f4eb3952007cb.

If you are trying to do what @Kicer86 did, you will now get a very clear compilation error that tells you precisely what went wrong. You may try to suppress it by passing REFLECT_CPP_IGNORE_LOCAL_LINKAGE to the compiler and then it might work, because I am using @schaumb's approach.

However, the default is for this not to work, because I am unconvinced that the solution @schaumb proposes does not exploit what is actually a bug in the compilers.

However, I am open to arguments to the contrary.

@schaumb , @Kicer86 , what are your thoughts on this?

schaumb commented 6 months ago

I think this is not a bug because later it can be defined in this compilation unit. And because no one uses it, it shouldn't be a linker error. But I could not argue with my statement.

If you are afraid that template extern wrapper<T> is not what you want to use because of extern an internal linkage class, replace it with template struct static member. This technique is much older, and no one found this to be problematic. (the definition is inside the class, and the declaration is nowhere)


Note: If you want to use static assertion on types, you must check unnamed classes too.

liuzicheng1987 commented 6 months ago

@schaumb , yes, my major concern was the fact that this workaround does indeed externally link something that should be internally linked. I think this is something that compilers shouldn’t allow. It defies the point of internal linkage. But I will try the other approach that works without the extern keyword. I think this looks very promising.

liuzicheng1987 commented 6 months ago

@schaumb , the latter solution does not work on newer version of MSVC. The compiler (quite accurately) returns the following error:

error C7631: variable with internal linkage declared, but not defined.

I will see what I can do, but if you have any ideas, that would be very appreciated.

EDIT: Never mind, I know what the problem is. I will solve it tomorrow. Thank you so much for your input so far.

schaumb commented 6 months ago

you can force MSVC to "not use" the symbol if it is used only in template arguments. https://godbolt.org/z/bWfTTvqo3

liuzicheng1987 commented 6 months ago

@schaumb Yes, that is what I figured out as well, hence the edit. Sorry to have bothered you there. But thank you anyway for the Godbolt example.

I will implement a solution based on that approach tomorrow, but it’s the middle of the night here…

Kicer86 commented 6 months ago

@Kicer86 , I have tried this with GCC 11.4

I have gcc 13.2.1

liuzicheng1987 commented 6 months ago

All right, this issue is finally resolved.

@Kicer86 , thank you for raising this issue.

@schaumb , thank you for all of your input.

By the way, do you want to see something really fucked up? Check out line 77:

https://www.godbolt.org/z/4ThqdnMPz

It's an MSVC-only problem. Luckily, there is a simple fix...

schaumb commented 6 months ago

Yes, I know that problem. This can be used for some hacks, like: The std::is_constant_evaluated() function is only available from C++20. But in MSVC because of the member pointer inconsistency (different handling on compile time template, compile time constexpr function, and runtime), I found a C++11 version implementation. When MSVC checks that the member pointer is already template instantiated, it only compares the offset (like the runtime version).

schaumb commented 6 months ago

Another note for the solution: Probably it would have been easier if the template extern variable was put in an anonymous namespace. (last block:) kép

liuzicheng1987 commented 6 months ago

I like your idea of having extern inside of an unnamed namespace. It certainly seems like less of a workaround than the current solution. I will give that a try as well.