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

The gnulink tool probably should not set -Bsymbolic #3248

Closed
acmorrow opened this issue Nov 28, 2018 · 12 comments
Closed

The gnulink tool probably should not set -Bsymbolic #3248

acmorrow opened this issue Nov 28, 2018 · 12 comments

Comments

@acmorrow
Copy link
Contributor

  • Link to SCons Users thread discussing your issue.
  • Version of SCons
    3.x, not sure how much older.

Right now the 'gnulink' tool unconditionally adds -Bsymbolic to the SHLIBVERSIONFLAGS variable. That probably isn't a good idea: the choice of whether to build a dynamic library with internal references as symbolic should probably be left to the developer.

@acmorrow
Copy link
Contributor Author

Some additional background on this: https://software.intel.com/en-us/articles/performance-tools-for-software-developers-bsymbolic-can-cause-dangerous-side-effects

@mwichmann
Copy link
Collaborator

I would tend to agree.

@garyo
Copy link
Contributor

garyo commented May 8, 2019

Yes, I agree too. Not appropriate for default setting. It will be a breaking change, however.

@acmorrow
Copy link
Contributor Author

@garyo - Agree it is a breaking change, but probably one for the better. Those who wanted -Bsymbolic were probably already asking for it explicitly by adding it to SHLINKFLAGS, because why would they expect it to magically arrive? Those who don't want it or don't know what it is are probably unaware of the risks/consequences/subtleties and would be better off without it being auto-added. I propose making the breaking change and adding a release note.

@bdbaddog
Copy link
Contributor

We probably need to figure out how the SCons Deprecation cycle applies to such a change.
Likely we'd announce we're going to do it in next release, and then make the change in the following release?

see: https://github.com/SCons/scons/wiki/DeprecationCycle

@garyo
Copy link
Contributor

garyo commented May 14, 2019

Yes, that's the proper SCons process for making a breaking change. In this case the change is pretty minor, and makes sense, but it still would (in some edge cases, which we can discuss the importance of) violate the principle of not breaking builds in a minor release.

The workaround in either direction is pretty simple: users can reset SHLINKFLAGS to omit -Bsymbolic today, and if we remove it, it's easy for users to add.

We'd certainly need to announce that it's coming before we do it, and follow the parts of the deprecation cycle that make sense (not all of that applies in this case).

mwichmann added a commit to mwichmann/scons that referenced this issue Aug 29, 2019
Pass 1: test case showing the failure.

This is WIP - need to add the fix, but want a CI build to show the fail.

Closes issues SCons#3248

Signed-off-by: Mats Wichmann <mats@linux.com>
mwichmann added a commit to mwichmann/scons that referenced this issue Aug 29, 2019
Pass 1: test case showing the failure.

This is WIP - need to add the fix, but want a CI build to show the fail.

Closes issues SCons#3248

Signed-off-by: Mats Wichmann <mats@linux.com>
mwichmann added a commit to mwichmann/scons that referenced this issue Sep 8, 2019
Pass 1: test case showing the failure.

This is WIP - need to add the fix, but want a CI build to show the fail.

Closes issues SCons#3248

Signed-off-by: Mats Wichmann <mats@linux.com>
@bdbaddog bdbaddog added this to To do in User Pain Points # 1 Nov 19, 2019
@acmorrow
Copy link
Contributor Author

acmorrow commented Apr 9, 2020

Just to add my thoughts. I consider this a bug, not a user breaking change. I don't think a deprecation cycle is necessary. That flag has dangerous side effects and SCons shouldn't be setting link policy like that.

@acmorrow
Copy link
Contributor Author

My thought is that we should, as noted above, immediately remove -Bsymbolic without a deprecation cycle. We can include the following language or something similar in the changelog or release notes:

The -Bsymbolic flag has been removed from SHLIBVERSIONFLAGS in the gnulink Tool. Versioned shared libraries will no longer have this flag applied to the link step by default. Use of this flag is unsafe in C++ projects because it leads to ODR violations and can interfere with proper behavior of new/delete. A build system should not, by default, introduce the potential for undefined behavior. Projects which were relying on automatic application of -Bsymbolic in versioned shared libraries must now explicitly add the -Bsymbolic flag to SHLIBVERSIONFLAGS or SHLIBFLAGS` in their project setup.

@garyo
Copy link
Contributor

garyo commented Apr 21, 2020

I agree it's dangerous and should be removed (I was using it years ago but we stopped once better alternatives became available). People are probably using SCons without noticing that it adds that.

But we can't just spring it on people in a minor release without warning. Maybe it would be enough to do the following: 1. announce on users/devs lists, 2. announce on discord, 3. highlight it in release notes (and why it's not following the usual depr cycle) per @acmorrow's note above. That's my opinion anyway...

@bdbaddog
Copy link
Contributor

@garyo - sounds reasonable. Done.

@mwichmann
Copy link
Collaborator

This isn't even a new issue... https://pairlist4.pair.net/pipermail/scons-users/2016-May/004893.html. Looks like some effort was made to mitigate this already in 3.0.0 (see CHANGES.txt).

bdbaddog added a commit that referenced this issue Apr 21, 2020
Resolve issue #3248 - removed '-Wl,-Bsymbolic' from SHLIBVERSIONFLAGS for gnulink
@bdbaddog
Copy link
Contributor

Fixed in #3621

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

No branches or pull requests

4 participants