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

Fix multiple definitions of g_client error #180

Conversation

dpfrey
Copy link
Contributor

@dpfrey dpfrey commented May 20, 2021

The global variable g_client was defined in both erpc_client_setup.cpp
and erpc_arbitrated_client_setup.cpp. As a result, linker errors were
produced when trying to build eRPC with both files. This change defines
the g_client in a single new place (erpc_setup_globals.cpp).

Resolves: #113

@Hadatko
Copy link
Member

Hadatko commented May 21, 2021

Hi, i think @MichalPrincNXP was fixing something with g_client so i will not answer to this PR if it is correct or not. I let it on Michal. But at least the name of the file should contains _client in name as the only variable is for client and server don't need to include that file.

@Hadatko
Copy link
Member

Hadatko commented May 21, 2021

Also what about usage of pragma weak? We are using that for const numbers in shim code:

#pragma weak erpc_generated_crc
extern const uint32_t erpc_generated_crc = {$crc16};

@dpfrey
Copy link
Contributor Author

dpfrey commented May 25, 2021

Hi @Hadatko,

Thanks for taking a look at this. I think I saw an issue where @MichalPrincNXP said he was going to address it, but it's still not fixed, so I thought that maybe he forgot or had other things to do. I have added two additional commits. The first one (5a96174) renames the file that contains the global variable to include client in the name. The second commit (b33be8c) undoes the approach implemented in the first two commits and instead uses #pragma weak. This approach works as well.

@MichalPrincNXP MichalPrincNXP self-assigned this Jun 24, 2021
@MichalPrincNXP MichalPrincNXP self-requested a review June 24, 2021 08:04
@MichalPrincNXP
Copy link
Member

Hello @dpfrey , thanks, now I understand the issue that happens when both erpc_client_setup.cpp and erpc_arbitrated_client_setup.cpp are being built in one project/make. Internally, we generate individual projects for different IDEs and because it does not make sense to have erpc_client_setup.cpp and erpc_arbitrated_client_setup.cpp in one project this issue has not be observed. Anyway, the solution by you and Dusan seems to be ok and working. May I ask you to rebase to the develop branch head to be able to trigger Travis build (recent commit fixes Travis builds)? Thank you.

The variable g_client is defined in both
erpc_arbitrated_client_setup.cpp and erpc_client_setup.cpp. #pragma weak
is used to avoid a linker failure due to multiple definitions of this
variable.

Resolves: EmbeddedRPC#113
@dpfrey dpfrey force-pushed the pr_g_client_multiple_definitions branch from b33be8c to 59f51e3 Compare June 24, 2021 18:23
@dpfrey
Copy link
Contributor Author

dpfrey commented Jun 24, 2021

I rebased the change onto develop and squashed away the initial approach so that only the #pragma weak change is now present.

@MichalPrincNXP
Copy link
Member

Hello @dpfrey , it seems Clang C++ on Linux and both gcc and clang compilers on Mac are not ok with the way g_client is defined, see Travis build results
May I ask you to try to add extern keyword before the g_client declaration?

Move the pragma weak after the declaration in an attempt to improve
compatibility.
@dpfrey
Copy link
Contributor Author

dpfrey commented Jul 7, 2021

I moved the #pragma weak g_client after the variable declaration and that seemed to resolve the Travis build issues.

@MichalPrincNXP
Copy link
Member

Thank you David, I will merge this PR after the final v1.8.1 release next week, ok?

@MichalPrincNXP MichalPrincNXP merged commit 31cb98c into EmbeddedRPC:develop Jul 19, 2021
@MichalPrincNXP
Copy link
Member

Thank you @dpfrey !

@dpfrey dpfrey deleted the pr_g_client_multiple_definitions branch July 19, 2021 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

multiple definition of g_client
3 participants