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

Fixing linker warnings with Arm Compiler toolchain (C++) #6707

Closed
wants to merge 1 commit into from

Conversation

MatthiasHertel80
Copy link

The Arm Compiler toolchains will issue warnings about multiple definitions of flatbuffer_version_string, despite the weak attribute.
Warning: L6439W: Multiply defined Global Symbol flatbuffers::flatbuffer_version_string defined in .data._ZN11flatbuffers25flatbuffer_version_stringE(op_resolver.o) rejected in favor of Symbol defined in .data._ZN11flatbuffers25flatbuffer_version_stringE(flatbuffer_conversions.o).

This affects some projects, mainly embedded, where this compiler is popular. This can be seen in issues, like:
tensorflow/tensorflow#45686
https://forum.edgeimpulse.com/t/not-compiling-symbol-multiply-defined/496

I recommend to mimic the MSVC builds and not compile in the version string. It will not be used for the documented statistical purpose on those embedded platforms anyways.

@google-cla
Copy link

google-cla bot commented Jun 23, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@github-actions github-actions bot added the c++ label Jun 23, 2021
@MatthiasHertel80
Copy link
Author

@googlebot I signed it!

@vglavnyy
Copy link
Contributor

Keil (ARMCC) has support of __attribute__((weak)) in GNU mode and the compiler-scpecific keyword __weak.

This change can break user code if someone uses this flatbuffer_version_string in ARMCC GNU mode.
@MatthiasHertel80 Can you add checking for GNU and verify it with the ARMCC compiler?

@aardappel

  • The const char *flatbuffer_version_string might be declared as constexpr, probably this would have the same effect as weak in this declaration.
  • There is the duplicated method:
    namespace flatbuffers {
    // Returns version as string "MAJOR.MINOR.REVISION".
    const char* FLATBUFFERS_VERSION();
    }

    This method requires flatbuffers library and can't be used in header-only mode. It returns a version of flatbuffers.a or flatbuffers.so library. This version might differ from the header one (bad case).

@aardappel
Copy link
Collaborator

aardappel commented Jun 28, 2021

I don't think there's going to be a ton of people relying on flatbuffer_version_string

In fact, I'd love to remove it all-together, and instead rely on an (inline) function of the same. If you see the comment above it (and similarly at the end of building.md), the intent of my original team where I wrote FlatBuffers was for this string to end up in binaries, and to collect statistics on how many apps use FlatBuffers, to justify further development of it. We're way past that stage now, so this can all be removed.

@dbaileychess
Copy link
Collaborator

@MatthiasHertel80 Would you like to make a PR removing this?

@dbaileychess
Copy link
Collaborator

This should be closed by #7046. Let me know if it is or not.

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

Successfully merging this pull request may close these issues.

4 participants