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 building plugins with address sanitizer enabled #3905

Merged
merged 1 commit into from Jun 2, 2022

Conversation

criezy
Copy link
Member

@criezy criezy commented May 22, 2022

The issues was introduced with commit 0974f16. Before that commit the $LDFLAGS were used for the link command on the plugins, and this includes the sanitizer flags. After that commit it uses the $SAVED_LDFLAGS instead and that results in link errors (undefined symbols) when address sanitizer is enabled (and probably also for than and ubsan, although I have not checked).

Reverting the commit fixes the issue for me on macOS. However it is probably not a good fix, and is meant to open a discussion on the best way to fix the issue. Which is why I marked it as draft.

The commit was added in PR #2082 by @besser82 . From the discussion there I understand that it fixes link errors on some platforms (see #2077 (comment)). So I am guessing reverting it would reintroduce those errors. However those are no longer visible on buildbot so I am not sure what the issues were.

If I understood that correctly and the change from this PR is not good, my suggestion would be to introduce a PLUGIN_LDFLAGS initially set to SAVED_LDFLAGS but to which we add the asan, tsan, and ubsan flags (and maybe others?) depending on the configure options used. Does anybody have a better suggestion?

@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented May 23, 2022

If I understood that correctly and the change from this PR is not good, my suggestion would be to introduce a PLUGIN_LDFLAGS initially set to SAVED_LDFLAGS but to which we add the asan, tsan, and ubsan flags (and maybe others?) depending on the configure options used. Does anybody have a better suggestion?

I think it would be the more reasonable.

Adding the sanitizer flags to the PLUGIN_LDFLAGS fixes link errors
for the plugins when asan, tsan, or ubsan is enabled.

Adding the original LDFLAGS to PLUGIN_LDFLAGS means we no longer needs
to use both in the link command for plugins.
@criezy
Copy link
Member Author

@criezy criezy commented May 23, 2022

There was actually already a PLUGIN_LDFLAGS defined in configure. But I have now made changes

  • to set it initially to the LDFLAGS defined when configure is called (which allows to remove SAVED_LDLFAGS from the link command for the plugin)
  • to add sanitizer flags to it

@criezy criezy marked this pull request as ready for review May 23, 2022
@criezy criezy changed the title Revert "BUILD: Use unmodified SAVED_LDFLAGS from env for linking plug… Fix building plugins with address sanitizer enabled May 23, 2022
@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented May 24, 2022

Looks good to me although I didn't test it.
I can do tests next week-end but I am sure the change could be merged without them.

Thanks! This is way cleaner like this.

@criezy
Copy link
Member Author

@criezy criezy commented Jun 2, 2022

I will merge this now. I don't expect issues since the changes are relatively simple and I tested them, but will monitor buildbot to make sure nothing breaks.

@criezy criezy merged commit 4d2bd56 into scummvm:master Jun 2, 2022
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants