Created attachment 361242 [details] [review]
Hack patch

This patch adds the proper link_args to the libraries, but does not set them depending on the system as would be required for non-Linux builds.

Review of attachment 361242 [details] [review]:

::: c/meson.build
@@ +1543,3 @@
 
+# HACK
+library_link_args = [ '-Wl,-z,nodelete' ]

This could easily be replaced by something we already use in GTK's build:

```
library_link_args = []

if host_machine.system() == 'linux'
  foreach ldflag: [
    '-Wl,-Bsymbolic',
    '-Wl,-z,nodelete',
    '-Wl,-z,relro',
    '-Wl,-z,now',
    ]
    if cc.has_argument(ldflag)
      library_link_args += ldflag
    endif
  endforeach
endif
```

(In reply to Emmanuele Bassi (:ebassi) from comment #2)
> This could easily be replaced by something we already use in GTK's build:
> 
> *snip*

Porting everything to Meson is going to be a bit of a mess if every project has to cargo-cult this kind of stuff.

(In reply to Philip Withnall from comment #3)
> (In reply to Emmanuele Bassi (:ebassi) from comment #2)
> > This could easily be replaced by something we already use in GTK's build:
> > 
> > *snip*
> 
> Porting everything to Meson is going to be a bit of a mess if every project
> has to cargo-cult this kind of stuff.

Better than to depend on an archive of m4 macros…

While I agree that we should move reasonable defaults to Meson, in this scase:

  * -z,nodelete is kind of specific linker flag for GObject-based libraries with static types
  * -z,relro and -z,now are hardening toggles that have been doing the rounds in distro packages for a while:

https://mudongliang.github.io/2016/07/11/relro-a-not-so-well-known-memory-corruption-mitigation-technique.html

The only linker flag that may make sense as a default provided by Meson for shared libraries is `-Bsymbolic`, but then you get to pick your poison, and choose between `-Bsymbolic` and `-Bsymbolic-functions` depending on whether you have exported global symbols or only functions.

Compiler and linker flags that go beyond blanket warnings or errors should really be decided by project maintainers.

If we bumped up the Meson dependency to the newly released 0.43.0, we could replace the foreach block above with:

library_link_flags = []
if host_machine.system() == 'linux'
  library_link_flags += cc.get_supported_arguments([
    '-Wl,-Bsymbolic',
    '-Wl,-z,nodelete',
    '-Wl,-z,relro',
    '-Wl,-z,now',
  ])
endif

But the list of arguments is still decided by the project.

This can be simplified with:
library_link_flags = cc.get_supported_arguments([
    '-Wl,-Bsymbolic',
    '-Wl,-z,nodelete',
    '-Wl,-z,relro',
    '-Wl,-z,now',
  ])

And depend on meson 0.43.

Oops, didn't notice last comment already suggest it.

(In reply to Emmanuele Bassi (:ebassi) from comment #4)
>   * -z,nodelete is kind of specific linker flag for GObject-based libraries
> with static types

Yes, it makes sense for GLib to specify this one manually.

>   * -z,relro and -z,now are hardening toggles that have been doing the
> rounds in distro packages for a while:
> 
> https://mudongliang.github.io/2016/07/11/relro-a-not-so-well-known-memory-
> corruption-mitigation-technique.html

Meson should provide its own option for adding hardening flags (which should probably be default in non-plain builds). I agree with Philip that projects should not be cargo-culting these flags around.

> The only linker flag that may make sense as a default provided by Meson for
> shared libraries is `-Bsymbolic`, but then you get to pick your poison, and
> choose between `-Bsymbolic` and `-Bsymbolic-functions` depending on whether
> you have exported global symbols or only functions.

Perhaps. Or perhaps -Bsymbolic would be a sane default linker flag for installed shared libraries?

(In reply to Michael Catanzaro from comment #7)
> Perhaps. Or perhaps -Bsymbolic would be a sane
> default linker flag for installed shared libraries?

I agree. Meson should add -Bsymbolic unless the developer specifies that the library wants to support interposition for its symbols.

(In reply to Xavier Claessens from comment #5)
> This can be simplified with:
> library_link_flags = cc.get_supported_arguments([
>     '-Wl,-Bsymbolic',
>     '-Wl,-z,nodelete',
>     '-Wl,-z,relro',
>     '-Wl,-z,now',
>   ])
> 
> And depend on meson 0.43.

Sadly, no: we can't do this because Meson 0.44 started warning when trying to check for linker arguments.

At this point, I'd just use:

```
library_link_flags = []
if host_machine.system() == 'linux' and (cc.get_id() == 'gcc' or cc.get_id() == 'clang')
  library_link_flags += [
    '-Wl,-Bsymbolic',
    '-Wl,-z,nodelete',
    '-Wl,-z,relro',
    '-Wl,-z,now',
  ]
endif
```

As those make sense only on the Linux linker, and are guaranteed to work with GCC and Clang.

(In reply to Jan Alexander Steffens (heftig) from comment #8)
> (In reply to Michael Catanzaro from comment #7)
> > Perhaps. Or perhaps -Bsymbolic would be a sane
> > default linker flag for installed shared libraries?
> 
> I agree. Meson should add -Bsymbolic unless the developer specifies that the
> library wants to support interposition for its symbols.

Feel free to open a bug against Meson; in the meantime, GLib should not block on this.

(In reply to Emmanuele Bassi (:ebassi) from comment #9)
> Sadly, no: we can't do this because Meson 0.44 started warning when trying
> to check for linker arguments.

Is that a regression in meson? or did it never worked? I guess cc.get_supported_arguments() is for cflags and we could add cc.get_supported_link_arguments() in meson for ldflags, right?

(In reply to Xavier Claessens from comment #11)
> (In reply to Emmanuele Bassi (:ebassi) from comment #9)
> > Sadly, no: we can't do this because Meson 0.44 started warning when trying
> > to check for linker arguments.
> 
> Is that a regression in meson? or did it never worked?

It never really worked, so Meson started emitting warnings for it. Sadly, there's no real replacement, as of yet.

> I guess
> cc.get_supported_arguments() is for cflags and we could add
> cc.get_supported_link_arguments() in meson for ldflags, right?

Yep, that would be the solution. Again, I don't want to block on new Meson functionality unless it's absolutely necessary.

(In reply to Emmanuele Bassi (:ebassi) from comment #12)
> Yep, that would be the solution. Again, I don't want to block on new Meson
> functionality unless it's absolutely necessary.

Right, no need to block on it, but I reported a feature request because I think it would be nicer: https://github.com/mesonbuild/meson/issues/3335

Created attachment 371299 [details] [review]
Meson: Add a few link args if supported

This uses new API added in meson 0.46.0.

Review of attachment 371299 [details] [review]:

This needs some justifications and some splitting up, and can’t be pushed until Meson 0.46.0 is available in BuildStream (but that’s in progress on another bug).

::: meson.build
@@ +339,3 @@
   ]
+  test_c_link_args = [
+    '-Wl,-Bsymbolic',

The autotools build currently uses -Bsymbolic-functions. Do we have a justification for switching to -Bsymbolic?

Also, the autotools build has a --disable-Bsymbolic option. Do we have a justification for dropping that?

@@ +340,3 @@
+  test_c_link_args = [
+    '-Wl,-Bsymbolic',
+    '-Wl,-z,nodelete',

Similarly, the autotools build has a --disable-znodelete option. Do we have a justification for dropping that?

@@ +342,3 @@
+    '-Wl,-z,nodelete',
+    '-Wl,-z,relro',
+    '-Wl,-z,now',

I don’t think we currently build with relro or now. Can you split these out into separate patches, so this patch essentially just makes the Meson build match what’s currently done in autotools? Then these other ones can be considered/tested/potentially-reverted-if-needed separately.

(For example, -Wl,-z,now may cause slowdowns on application startup because it causes all the dynamic symbols to be resolved early, rather than when they’re first trampolined — this might turn out to be unacceptable for GLib.)

(In reply to Philip Withnall from comment #15)
> Review of attachment 371299 [details] [review] [review]:
> 
> This needs some justifications and some splitting up, and can’t be pushed
> until Meson 0.46.0 is available in BuildStream (but that’s in progress on
> another bug).
> 
> ::: meson.build
> @@ +339,3 @@
>    ]
> +  test_c_link_args = [
> +    '-Wl,-Bsymbolic',
> 
> The autotools build currently uses -Bsymbolic-functions. Do we have a
> justification for switching to -Bsymbolic?

We have exported global symbols — look for GLIB_VAR — so `-Bsymbolic` would cover those as well.

> Also, the autotools build has a --disable-Bsymbolic option. Do we have a
> justification for dropping that?

I can't find a justification for *adding* it in the first place; I only see Allison's commit in 2010, with no bug reference.

Personally, I think it's wrong to allow disabling it if the linker supports it: we have clear advantages, and no disadvantages.

> @@ +340,3 @@
> +  test_c_link_args = [
> +    '-Wl,-Bsymbolic',
> +    '-Wl,-z,nodelete',
> 
> Similarly, the autotools build has a --disable-znodelete option. Do we have
> a justification for dropping that?

The -z,nodelete flag was added by copy-pasting the fragment that added -Bsymbolic-functions, so it got the AC_ARG_ENABLE bit as well.

If our linker supports -z,nodelete then we should definitely use it; I don't see a particular reason to disable it, outside of platform-specific behaviour, and in that case we have a check.

> @@ +342,3 @@
> +    '-Wl,-z,nodelete',
> +    '-Wl,-z,relro',
> +    '-Wl,-z,now',
> 
> I don’t think we currently build with relro or now. Can you split these out
> into separate patches, so this patch essentially just makes the Meson build
> match what’s currently done in autotools? Then these other ones can be
> considered/tested/potentially-reverted-if-needed separately.

Good point, +1.

(In reply to Emmanuele Bassi (:ebassi) from comment #16)
> (In reply to Philip Withnall from comment #15)
> > Review of attachment 371299 [details] [review] [review] [review]:
> > 
> > This needs some justifications and some splitting up, and can’t be pushed
> > until Meson 0.46.0 is available in BuildStream (but that’s in progress on
> > another bug).
> > 
> > ::: meson.build
> > @@ +339,3 @@
> >    ]
> > +  test_c_link_args = [
> > +    '-Wl,-Bsymbolic',
> > 
> > The autotools build currently uses -Bsymbolic-functions. Do we have a
> > justification for switching to -Bsymbolic?
> 
> We have exported global symbols — look for GLIB_VAR — so `-Bsymbolic` would
> cover those as well.

I know what the difference is, and I suspect that -Bsymbolic is more appropriate; but with my conservative maintainer hat on, I’d like to not potentially break things without thinking about it first.

> > Also, the autotools build has a --disable-Bsymbolic option. Do we have a
> > justification for dropping that?
> 
> I can't find a justification for *adding* it in the first place; I only see
> Allison's commit in 2010, with no bug reference.
> 
> Personally, I think it's wrong to allow disabling it if the linker supports
> it: we have clear advantages, and no disadvantages.

There’s one potential disadvantage in that if you pass --disable-Bsymbolic, you can interpose symbols for debugging inside GLib. That’s crack though, and I would be happy to see that go. It is worth thinking about a bit though.

> > @@ +340,3 @@
> > +  test_c_link_args = [
> > +    '-Wl,-Bsymbolic',
> > +    '-Wl,-z,nodelete',
> > 
> > Similarly, the autotools build has a --disable-znodelete option. Do we have
> > a justification for dropping that?
> 
> The -z,nodelete flag was added by copy-pasting the fragment that added
> -Bsymbolic-functions, so it got the AC_ARG_ENABLE bit as well.
> 
> If our linker supports -z,nodelete then we should definitely use it; I don't
> see a particular reason to disable it, outside of platform-specific
> behaviour, and in that case we have a check.

I agree here too. Basically, I think the entire patch is good, but I want it to be good as the result of some reasoning (laid out here and in the commit message), not just because it’s been cargo-culted from elsewhere.

> > @@ +342,3 @@
> > +    '-Wl,-z,nodelete',
> > +    '-Wl,-z,relro',
> > +    '-Wl,-z,now',
> > 
> > I don’t think we currently build with relro or now. Can you split these out
> > into separate patches, so this patch essentially just makes the Meson build
> > match what’s currently done in autotools? Then these other ones can be
> > considered/tested/potentially-reverted-if-needed separately.
> 
> Good point, +1.

Even more than that, I think I’d like to see the commit which introduces -Bsymbolic and -z,nodelete to Meson to be introducing *exactly the same* behaviour as we have in autotools. So that means a couple of patches to the autotools build first to
 • drop the --disable-Bsymbolic and --disable-znodelete arguments;
 • change -Bsymbolic-functions to -Bsymbolic
Then another two patches to:
 • add -z,relro to Meson *and* autotools; then
 • add -z,now to Meson *and* autotools.

Before we push the final two patches,  I’d also like to see some quick performance measurements of, for example, starting up Evolution, to check how they impact startup linking performance. GLib has a lot of symbols, and most applications don’t use half of them.

(In reply to Emmanuele Bassi (:ebassi) from comment #10)
FTR, this has been filed at https://github.com/mesonbuild/meson/issues/3338 .

Created attachment 371347 [details] [review]
configure.ac: Remove --disable-Bsymbolic and --disable-znodelete options

There is no reason to disable those linker flags. If anyone complains we
can add them back.

Created attachment 371348 [details] [review]
Meson: Add -Wl,-z,nodelete and -Wl,-Bsymbolic-functions where supported

Disclaimer: I don't really understand what are the implications of those flags, I'm just making the patches based on comments here to keep things moving.

I changed the meson patch to add -Bsymbolic-functions instead of -Bsymbolic, and dropped -z,relro and -z,now to be on par with autotools. I think we should open a separate bug to discuss changing those flags, the goal here is really to have meson and autotools use the same flags.

Oh, and while working on this, I noticed that configure.ac also pass -Wl,--fatal-warnings when checking for -Wl,-z,nodelete. And indeed meson's has_link_argument('-Wl,-z,nodelete42') returns true. Fixed that in meson: https://github.com/mesonbuild/meson/pull/3467

Review of attachment 371347 [details] [review]:

--disable-Bsymbolic is documented in docs/reference/glib/building.xml; that documentation needs to be removed.

Review of attachment 371348 [details] [review]:

This looks good to push once Meson 0.46.0 is available in the fdo SDK (https://gitlab.com/freedesktop-sdk/freedesktop-sdk/merge_requests/210), thanks. Using accepted-commit_after_freeze for that.

We're actually still using https://github.com/flatpak/freedesktop-sdk-images/ to generate our flatpak runtimes. I will handle that one.

https://github.com/flatpak/freedesktop-sdk-images/pull/98

oh, one on gitlab.com, the other on github.com, can't wait to have gitlab.freedesktop.org...

The gitlab.com one will soon obsolete the github.com one, but I'm completely and totally fed up with gitlab.com due to its absurdly-frustrating use of recaptcha, to the point where I'm pretty sure we should not be using gitlab.com for any purpose... so I'm also looking forward to gitlab.freedesktop.org :)

(In reply to Philip Withnall from comment #23)
> Review of attachment 371347 [details] [review] [review]:
> 
> --disable-Bsymbolic is documented in docs/reference/glib/building.xml; that
> documentation needs to be removed.

Hmm, that doc says:
A side-effect of this is that it is no longer possible to override internal uses of GLib functions with LD_PRELOAD.

That made me remember, that's how refdbg works, isn't it? Their README[1] says to build glib with --disable-visibility which doesn't exist (anymore), I guess they mean --disable-Bsymbolic.

Are we sure we want to drop support for those kind of debug tools? I'm not against it, just want to be sure we know what we are doing ;-) Could as well just add an option for it in our meson_options.txt.

I would still remove --disable-znodelete because that one really has no use and is not even documented.

[1] http://refdbg.sourceforge.net/README

Oh, debian even has a package that builds with --disable-bsymbolic:
https://packages.debian.org/jessie/libglib2.0-0-refdbg

Created attachment 371374 [details] [review]
configure.ac: Remove --disable-znodelete options

There is no reason to disable this linker flags, and we are not going to
add this option in our meson build system, so remove it from autotools
too to be on par. If anyone complains with a valid use-case we can add
them back.

Created attachment 371375 [details] [review]
Meson: Add -Wl,-z,nodelete and -Wl,-Bsymbolic-functions where supported

Since this bug is really about getting meson and autotools on par, I added bsymbolic_functions option in meson. If we want to change flags/options then we could do a separate bug where we'll change both.

Meson 0.46 passed CI and got merged in https://gitlab.com/freedesktop-sdk/freedesktop-sdk/merge_requests/210.

Go for it

Review of attachment 371374 [details] [review]:

Review of attachment 371375 [details] [review]:

The commit message could do with an explanation about why we’re keeping --disable-bsymbolic (i.e. to support refdbg and similar use cases) so we don’t have to do this archaeology again in future.

That said, I’m not particularly convinced about refdbg. It’s not packaged at all on Fedora, and was last released 4 years ago. We should be able to come up with a more modern way of reference count debugging (probably using systemtap) — but that’s for another bug report. This patch looks good apart from the commit message.

Please push once you’ve expanded the commit message.

Please also file a bug about exploring whether to use z,relro and z,now.

Attachment 371374 [details] pushed as 9d12af9 - configure.ac: Remove --disable-znodelete options
Attachment 371375 [details] pushed as a67dc37 - Meson: Add -Wl,-z,nodelete and -Wl,-Bsymbolic-functions where supported

Opened bug #795700 for those who care discussing further those flags.

The meson change breaks "meson test resources"

You're right, reverting that patch fix that unit test.

I guess the difference with autotools is in meson we use add_project_link_arguments() which adds those flags on all targets, while un Makefile.am we add those flags only for libgio, libglib, libgmodule, libgobject and libgthread.

I guess we should do the same on meson?

Created attachment 371578 [details] [review]
Meson: Do not build tests with nodelete/Bsymbolic-functions

Review of attachment 371578 [details] [review]:

That seems like a really big hammer to use*. Is it not possible to just unset -z,nodelete when linking gio/tests/resources.c? It wants to unload a GModule and test that a resource is no longer found; nodelete is interfering with that.

*As in, moving to add_project_link_arguments() was a good step for maintainability, since it means we no longer have to remember to add glib_link_flags to everything.

I don't think it's currently possible. I made a PR on meson to ignore project arguments on some targets, that could be useful for this case:

https://github.com/mesonbuild/meson/pull/3520

In the meantime, as far as I know, my patch is the only way.

(In reply to Xavier Claessens from comment #44)
> I don't think it's currently possible. I made a PR on meson to ignore
> project arguments on some targets, that could be useful for this case:
> 
> https://github.com/mesonbuild/meson/pull/3520
> 
> In the meantime, as far as I know, my patch is the only way.

Can you override a previous -z argument to the linker by passing -z again? e.g. Is the linker command `ld -z nodelete,blah -z blah` equivalent to `ld -z blah`?

If not, then we’ll have to go with your patch; but please add a FIXME comment pointing to your Meson bug report, so that we can easily tidy it up in future.

(In reply to Philip Withnall from comment #45)
> (In reply to Xavier Claessens from comment #44)
> > I don't think it's currently possible. I made a PR on meson to ignore
> > project arguments on some targets, that could be useful for this case:
> > 
> > https://github.com/mesonbuild/meson/pull/3520
> > 
> > In the meantime, as far as I know, my patch is the only way.
> 
> Can you override a previous -z argument to the linker by passing -z again?
> e.g. Is the linker command `ld -z nodelete,blah -z blah` equivalent to `ld
> -z blah`?

I can’t find a way to override a previous -z argument. :-(

Review of attachment 371578 [details] [review]:

I’ll add a FIXME comment to your commit and push, with a bit of an expanded commit message too.

Tweaked and pushed to master.

Attachment 371578 [details] pushed as b6cb22f - Meson: Do not build tests with nodelete/Bsymbolic-functions