Bug 83115 - [PATCH] link dbus dynamically against shared libdbus
Summary: [PATCH] link dbus dynamically against shared libdbus
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: medium enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-08-26 22:35 UTC by bertrand
Modified: 2015-02-24 13:50 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
dbus dynamic linking (12.58 KB, patch)
2014-08-26 22:35 UTC, bertrand
Details | Splinter Review
Second iteration of the patch (14.18 KB, patch)
2014-08-29 17:51 UTC, bertrand
Details | Splinter Review
[patch 1 v3] Link dbus-daemon and dbus-daemon-lauch-helper against libdbus (14.47 KB, patch)
2014-09-08 10:24 UTC, bertrand
Details | Splinter Review
[patch 2] Partially revert previous commit so the tests can build (4.77 KB, patch)
2014-09-11 16:09 UTC, Simon McVittie
Details | Splinter Review
[patch 3] Remove support for using dbus-glib for the regression tests (10.50 KB, patch)
2014-09-11 16:10 UTC, Simon McVittie
Details | Splinter Review
[patch 4] Link libdbus-internal to the shared libdbus-1 under CMake, too (2.85 KB, patch)
2014-09-11 16:59 UTC, Simon McVittie
Details | Splinter Review
[1/6] Remove some redundant inclusions (980 bytes, patch)
2014-09-22 19:11 UTC, Simon McVittie
Details | Splinter Review
[patch 6] Explicitly export all symbols needed to compile the tests for Windows (75.58 KB, patch)
2014-09-22 19:17 UTC, Simon McVittie
Details | Splinter Review
[2/6] sysdeps: try to avoid re-including config.h (818 bytes, patch)
2014-09-22 19:18 UTC, Simon McVittie
Details | Splinter Review
[patch 4] Link libdbus-internal to the shared libdbus-1 under CMake, too (rebased) (2.01 KB, patch)
2015-02-12 22:14 UTC, Ralf Habacker
Details | Splinter Review
[patch 6] Explicitly export all symbols needed to compile the tests for Windows (rebased) (76.61 KB, patch)
2015-02-12 22:15 UTC, Ralf Habacker
Details | Splinter Review
[PATCH] Export private functions required when enabled verbose mode. (1.63 KB, patch)
2015-02-19 20:45 UTC, Ralf Habacker
Details | Splinter Review
[3/6] tests: always use libdbus-internal for main loop, never dbus-glib (9.66 KB, patch)
2015-02-20 15:25 UTC, Simon McVittie
Details | Splinter Review
[4/6] Add DBUS_PRIVATE_EXPORT decoration to symbols used by dbus-daemon or tests (88.09 KB, patch)
2015-02-20 15:27 UTC, Simon McVittie
Details | Splinter Review
[5/6] On Unix platforms with gcc (or compatible), hide non-exported symbols (5.82 KB, patch)
2015-02-20 15:28 UTC, Simon McVittie
Details | Splinter Review
[6/6] Link dbus-daemon and dbus-daemon-lauch-helper against libdbus (13.14 KB, patch)
2015-02-20 15:35 UTC, Simon McVittie
Details | Splinter Review
Fix static linking with mingw (1.95 KB, patch)
2015-02-20 16:15 UTC, Simon McVittie
Details | Splinter Review
cmake: check for the necessary symbols for test-segfault.c (1.86 KB, patch)
2015-02-20 18:01 UTC, Simon McVittie
Details | Splinter Review
Remove checks for dbus-glib from configure.ac (2.09 KB, patch)
2015-02-20 21:17 UTC, Simon McVittie
Details | Splinter Review
tests: simplify Makefile.am now that libdbus is always dynamically linked (3.83 KB, patch)
2015-02-20 21:18 UTC, Simon McVittie
Details | Splinter Review
0001-Add-versioned-symbol-support-to-cmake-build-system-f.patch (1.19 KB, patch)
2015-02-20 22:14 UTC, Ralf Habacker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description bertrand 2014-08-26 22:35:58 UTC
Created attachment 105309 [details] [review]
dbus dynamic linking

dbus-daemon and dbus-daemon-launch-helper are statically linked against libdbus-internal.
This patch combine libdbus and libdbus-internal and changes the Makefiles to link dynamically dbus-daemon and dbus-daemon-launch-helper.

The private symbols are versioned under PRIVATE_@DBUS_VERSION@.

This patch is a follow up of the discussion on the mailing list (starting at: http://lists.freedesktop.org/archives/dbus/2014-June/016212.html).
Comment 1 Daniel Macks 2014-08-27 01:26:16 UTC
--version-script is a linux or gcc-ism, not portable to other compilers. For examplem it's never been supported on darwin (neither when we had gcc or now with clang).
Comment 2 Simon McVittie 2014-08-27 10:29:57 UTC
I hope to have time to do proper review later, but for now:

As I said on the mailing list, I am very skeptical about the benefit/cost ratio of doing this. Saving ~300K on-disk/in-memory, but in exchange, potentially breaking non-mainstream platforms altogether, or worse, having third-party applications start using and depending on ABI that we never intended to be public, stable or thread-safe? That sounds like a poor deal to me.

I would like to veto merging this change without broad agreement (i.e. someone persuading me that I'm wrong, or the rest of the D-Bus maintainers jointly overruling me).
Comment 3 David Zeuthen (not reading bugmail) 2014-08-27 17:28:50 UTC
> Saving ~300K on-disk/in-memory, 

For our (ChromeOS) purposes this is actually a substantial saving and why we invested energy in preparing the patch. I believe the same is true for many other projects targeting embedded systems.

> but in exchange, potentially breaking non-mainstream platforms altogether, or
> worse, having third-party applications start using and depending on ABI that
> we never intended to be public, stable or thread-safe? 

Sure, for most Linux distros the trade-off is not worth it.

How about making this a build-time option with a big fat warning? That way distributors can enable it as they see fit.
Comment 4 Simon McVittie 2014-08-27 20:40:14 UTC
(In reply to comment #3)
> How about making this a build-time option with a big fat warning? That way
> distributors can enable it as they see fit.

Non-default configurations do not, in general, work. I've spent a significant amount of time in the past on *removing* build options from D-Bus (expat vs libxml, userdb cache vs no userdb cache, etc.), usually with a commit message like "it turns out this has been broken since 2008".

If we do this, then the new way should be the only way. I think that means proper detection of whether version-scripts can work, versioning dbus_* as ABI, and versioning _dbus_* (and any other symbols that might leak out) as not-ABI.

Turning dbus-*-util.c (and the other files that are not in the public libdbus) into a static library that depends on libdbus-1.so.3 and is linked into dbus-daemon, and possibly dbus-daemon-launch-helper too, would probably make sense. (It could keep the old libdbus-internal.la name.) Otherwise, libdbus-1.so.3 (which is in clients' process space) will actually be larger than it is now, and will include code that is explicitly not thread-safe.

I suspect that dbus-daemon-launch-helper uses virtually nothing from dbus-*-util.c.
Comment 5 bertrand 2014-08-29 17:51:20 UTC
Created attachment 105441 [details] [review]
Second iteration of the patch
Comment 6 bertrand 2014-08-29 18:16:29 UTC
Simon,
I added a script to detect correctly the support of version-script.
libdbus-internal was kept to contain dbus-utils* as suggested.

I tested this patch on both linux and OS X: the original patch failed as expected. The second version compiles and all the test run on both platforms.

Thanks for your feedback! :)
Comment 7 Simon McVittie 2014-09-04 15:03:02 UTC
Comment on attachment 105441 [details] [review]
Second iteration of the patch

Review of attachment 105441 [details] [review]:
-----------------------------------------------------------------

::: bus/Makefile.am
@@ +133,4 @@
>  	$(LAUNCH_HELPER_SOURCES)
>  
>  dbus_daemon_launch_helper_test_LDADD=		\
> +	$(top_builddir)/dbus/libdbus-1.la \

Oh, does dbus-daemon-launch-helper not actually need anything from what I called category (c) on the mailing list, at all? That's good to know.

::: configure.ac
@@ +1387,5 @@
>  
> +### Detect if ld supports --version-script
> +
> +gl_LD_VERSION_SCRIPT
> +AM_CONDITIONAL(HAVE_LD_VERSION_SCRIPT, test x$have_ld_version_script = xyes)

Underquoted.

AM_CONDITIONAL([HAVE_LD_VERSION_SCRIPT], [test "x$have_ld_version_script" = xyes])

::: dbus/Version.in
@@ +1,1 @@
> +Base {

If we're going to do versioned symbols, we should do them properly, by making this LIBDBUS_1_3, or LIBDBUS_1_@SOVERSION@ (where @SOVERSION@ would be a new AC_SUBST for `expr $LT_CURRENT - $LT_AGE` - e.g. see MCP_ABI_VERSION in telepathy-mission-control's configure.ac).

@@ +3,5 @@
> +    dbus_*;
> +  local:
> +    *;
> +};
> +PRIVATE_@DBUS_VERSION@ {

I'd prefer LIBDBUS_PRIVATE_@DBUS_VERSION@.

::: m4/ld-version-script.m4
@@ +8,5 @@
> +
> +# FIXME: The test below returns a false positive for mingw
> +# cross-compiles, 'local:' statements does not reduce number of
> +# exported symbols in a DLL.  Use --disable-ld-version-script to work
> +# around the problem.

Is there any newer version of this m4 which fixes that?
Comment 8 bertrand 2014-09-08 10:24:41 UTC
Created attachment 105887 [details] [review]
[patch 1 v3] Link dbus-daemon and dbus-daemon-lauch-helper against libdbus
Comment 9 bertrand 2014-09-08 10:27:43 UTC
(In reply to comment #7)
> Comment on attachment 105441 [details] [review] [review]
> Second iteration of the patch
> 
> Review of attachment 105441 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: bus/Makefile.am
> @@ +133,4 @@
> >  	$(LAUNCH_HELPER_SOURCES)
> >  
> >  dbus_daemon_launch_helper_test_LDADD=		\
> > +	$(top_builddir)/dbus/libdbus-1.la \
> 
> Oh, does dbus-daemon-launch-helper not actually need anything from what I
> called category (c) on the mailing list, at all? That's good to know.
> 
> ::: configure.ac
> @@ +1387,5 @@
> >  
> > +### Detect if ld supports --version-script
> > +
> > +gl_LD_VERSION_SCRIPT
> > +AM_CONDITIONAL(HAVE_LD_VERSION_SCRIPT, test x$have_ld_version_script = xyes)
> 
> Underquoted.
Fixed

> AM_CONDITIONAL([HAVE_LD_VERSION_SCRIPT], [test "x$have_ld_version_script" =
> xyes])
> 
> ::: dbus/Version.in
> @@ +1,1 @@
> > +Base {
> 
> If we're going to do versioned symbols, we should do them properly, by
> making this LIBDBUS_1_3, or LIBDBUS_1_@SOVERSION@ (where @SOVERSION@ would
> be a new AC_SUBST for `expr $LT_CURRENT - $LT_AGE` - e.g. see
> MCP_ABI_VERSION in telepathy-mission-control's configure.ac).
> 
> @@ +3,5 @@
> > +    dbus_*;
> > +  local:
> > +    *;
> > +};
> > +PRIVATE_@DBUS_VERSION@ {
> 
> I'd prefer LIBDBUS_PRIVATE_@DBUS_VERSION@.

Done
> ::: m4/ld-version-script.m4
> @@ +8,5 @@
> > +
> > +# FIXME: The test below returns a false positive for mingw
> > +# cross-compiles, 'local:' statements does not reduce number of
> > +# exported symbols in a DLL.  Use --disable-ld-version-script to work
> > +# around the problem.
> 
> Is there any newer version of this m4 which fixes that?

I doubled checked and couldn't find an updated script. All the project I found are using either this script or a tweaked version of this one (no warning).
Comment 10 Simon McVittie 2014-09-11 14:19:01 UTC
(In reply to comment #7)
> Oh, does dbus-daemon-launch-helper not actually need anything from what I
> called category (c) on the mailing list, at all? That's good to know.

It doesn't, but dbus-daemon-launch-helper-test, test-bus-launch-helper, test-bus and test-bus-system do.
Comment 11 Simon McVittie 2014-09-11 14:24:53 UTC
(In reply to comment #10)
> (In reply to comment #7)
> > Oh, does dbus-daemon-launch-helper not actually need anything from what I
> > called category (c) on the mailing list, at all?
> 
> It doesn't

That's not true, it does need _dbus_spawn_async_with_babysitter() (and you added the libdbus-internal.la dependency).
Comment 12 Simon McVittie 2014-09-11 16:08:59 UTC
Comment on attachment 105887 [details] [review]
[patch 1 v3] Link dbus-daemon and dbus-daemon-lauch-helper against libdbus

Review of attachment 105887 [details] [review]:
-----------------------------------------------------------------

::: bus/Makefile.am
@@ +97,4 @@
>  	main.c
>  
>  dbus_daemon_LDADD=					\
> +	$(top_builddir)/dbus/libdbus-1.la	\

This added line is not strictly necessary, because Libtool understands linking with a static Libtool library like libdbus-internal.la to come with an implicit "... and everything in its LIBADD".

@@ +122,4 @@
>  	$(LAUNCH_HELPER_SOURCES)
>  
>  dbus_daemon_launch_helper_LDADD=		\
> +	$(top_builddir)/dbus/libdbus-1.la \

likewise this one

@@ +133,4 @@
>  	$(LAUNCH_HELPER_SOURCES)
>  
>  dbus_daemon_launch_helper_test_LDADD=		\
> +	$(top_builddir)/dbus/libdbus-1.la \

This line needs to be reverted: dbus_daemon_launch_helper_test_LDADD must contain $(top_builddir)/dbus/libdbus-internal.la, because, like the real (setuid) dbus-daemon-launch-helper, it calls _dbus_spawn_async_with_babysitter().

@@ +147,4 @@
>  	$(LAUNCH_HELPER_SOURCES)
>  
>  test_bus_launch_helper_LDADD=		\
> +	$(top_builddir)/dbus/libdbus-1.la \

Similarly, this line needs to be reverted, because the regression test for the launch helper calls "-util" functions.

(Did you run, or even compile, the regression tests?)

@@ +192,4 @@
>  	utils.h					\
>  	test-system.c
>  
> +test_bus_system_LDADD=$(top_builddir)/dbus/libdbus-1.la $(DBUS_BUS_LIBS)

This one too.

@@ +197,5 @@
>  test_bus_SOURCES=				\
>  	$(BUS_SOURCES)				\
>  	test-main.c
>  
> +test_bus_LDADD=$(top_builddir)/dbus/libdbus-1.la $(DBUS_BUS_LIBS)

And this one.

::: test/Makefile.am
@@ +23,5 @@
>  	$(top_builddir)/dbus/libdbus-1.la \
> +	$(top_builddir)/dbus/libdbus-internal.la	\
> +	$(NULL)
> +
> +if DBUS_WITH_DBUS_GLIB

The "with dbus-glib" configuration now fails to compile. There were previously two categories of tests:

- tests that link dbus-glib and the shared libdbus if possible,
  or use libdbus-internal if not

- tests that always use libdbus-internal

Tests in the latter category are now compiled without -DDBUS_TEST_USE_INTERNAL where they previously had that flag, resulting in failure to compile, because they can't find DBusLoop.

However, now that libdbus is always shared, the major reason to prefer linking dbus-glib (and the shared libdbus) - ability to run the tests on an installed system and actually be testing the installed copy of libdbus - has gone away. So we can simplify by getting rid of the optional dbus-glib dependency.

@@ +89,3 @@
>  test_spawn_SOURCES = spawn-test.c
> +test_spawn_CPPFLAGS = $(testutils_shared_if_possible_cppflags)
> +test_spawn_LDADD = $(top_builddir)/dbus/libdbus-1.la

Needs to be reverted. test-spawn must link libdbus-internal because it calls _dbus_spawn_async_with_babysitter().
Comment 13 Simon McVittie 2014-09-11 16:09:41 UTC
Created attachment 106146 [details] [review]
[patch 2] Partially revert previous commit so the tests can build

This brings back the old $(static_cppflags) but renames it to
$(internal_cppflags).

Known regression: compilation with dbus-glib enabled is broken.
Comment 14 Simon McVittie 2014-09-11 16:10:35 UTC
Created attachment 106147 [details] [review]
[patch 3] Remove support for using dbus-glib for the regression  tests

This was an annoying circular dependency, and doesn't actually work
any more now that tests are relying on being able to see private
symbols in the just-compiled libdbus. Use DBusLoop unconditionally
instead.

---

I would be OK with applying Attachment #105887 [details] to master if Attachment #106146 [details] and this one were also applied.
Comment 15 Simon McVittie 2014-09-11 16:21:10 UTC
David, you seem to have an interest in this, and you're still on the reviewer list. Perhaps you could review my follow-up patches? (Any review you can do on other waiting-for-review bugs would also be appreciated.)

Things that remain "statically linked" with these patches:

* dbus-daemon embeds (what remains of) libdbus-internal
* dbus-daemon-launch-helper embeds libdbus-internal (but will only be using
  a small part of it, and the linker should be able to discard the rest)
* various tests embed libdbus-internal

The tests I don't care about, and I don't think you should either: they're not installed on production or size-sensitive systems.

If someone wants to write a follow-up patch that selectively moved the parts of libdbus-internal used by dbus-daemon-launch-helper into libdbus-1, and do the research to see whether it's worth it (i.e. check the compiled, stripped sizes of dbus-daemon, d-d-l-h and libdbus-1 before and after), I would review that. My prediction is "it won't really matter either way", but I could be wrong.

When we get to a point where dbus-daemon is the only "production" component that embeds libdbus-internal, it will become impossible to save more space by sharing libdbus-internal.
Comment 16 Simon McVittie 2014-09-11 16:59:57 UTC
Created attachment 106150 [details] [review]
[patch 4] Link libdbus-internal to the shared libdbus-1 under CMake,  too
Comment 17 Ralf Habacker 2014-09-17 14:44:44 UTC
> Created attachment 105309 [details] [review] [review]
> dbus dynamic linking
> 
> dbus-daemon and dbus-daemon-launch-helper are statically linked against
> libdbus-internal.
> This patch combine libdbus and libdbus-internal and changes the Makefiles to
> link dynamically dbus-daemon and dbus-daemon-launch-helper.
> 
> The private symbols are versioned under PRIVATE_@DBUS_VERSION@.
> 

(In reply to comment #16)
> Created attachment 106150 [details] [review] [review]
> Link libdbus-internal to the shared libdbus-1 under CMake,  too

The currently dbus supported way of exporting symbols from a dll with msvc requires to decorate related functions with the DBUS_EXPORT macro.

Exporting private function need also such a decoration.
Comment 18 Simon McVittie 2014-09-17 16:02:27 UTC
(In reply to comment #17)
> The currently dbus supported way of exporting symbols from a dll with msvc
> requires to decorate related functions with the DBUS_EXPORT macro.
> 
> Exporting private function need also such a decoration.

Ugh, good point. So landing this stuff would be a regression on Windows, until someone decorates all the functions that need exporting with DBUS_EXPORT, or another macro that has the same effect.

(I think this would actually fail in the same way with mingw? The declspec stuff is about how Windows DLLs are linked, not about the MSVC compiler in particular.)
Comment 19 Simon McVittie 2014-09-17 16:26:34 UTC
(In reply to comment #18)
> (I think this would actually fail in the same way with mingw?

Yes, it does, for instance:

./.libs/libdbus-internal.a(dbus-message-factory.o): In function `dbus_message_data_free':
/home/smcv/src/fdo/dbus/dbus/dbus-message-factory.c:1242: undefined reference to `_dbus_string_free'
Comment 20 Simon McVittie 2014-09-22 19:10:31 UTC
adjusting title for better searches, I keep looking for it via "shared"
Comment 21 Simon McVittie 2014-09-22 19:11:39 UTC
Created attachment 106687 [details] [review]
[1/6] Remove some redundant inclusions

Both these files included dbus-test.h already.
Comment 22 Simon McVittie 2014-09-22 19:17:37 UTC
Created attachment 106688 [details] [review]
[patch 6] Explicitly export all symbols needed to compile the tests  for Windows

---

This addresses the problem that Ralf pointed out, but it's a pretty noisy change.

I used a new DBUS_PRIVATE_EXPORT macro (functionally identical to DBUS_EXPORT) instead of re-using DBUS_EXPORT, to make the intention clearer.

If we go this way, when compiling with gcc for non-Windows, I'm vaguely tempted to use -fvisibility=hidden and define DBUS_EXPORT and DBUS_PRIVATE_EXPORT to __attribute__((visibility("default"))) while compiling the library, which have essentially the same result as what we have to do on Windows anyway. I'm not sure how that interacts with versioned symbols though, and in any case, it can wait.
Comment 23 Simon McVittie 2014-09-22 19:18:30 UTC
Created attachment 106689 [details] [review]
[2/6] sysdeps: try to avoid re-including config.h

Re-including config.h after we have already included glib.h breaks
the GLIB_VERSION_MAX_ALLOWED macro, and every .c file should be
including config.h anyway.

---

Last one for now.
Comment 24 Simon McVittie 2014-10-09 10:48:50 UTC
(In reply to Simon McVittie from comment #15)
> David, you seem to have an interest in this, and you're still on the
> reviewer list. Perhaps you could review my follow-up patches? (Any review
> you can do on other waiting-for-review bugs would also be appreciated.)

Ping? If you're going to push for a change sufficiently enthusiastically to convince me that it's worthwhile despite my initial misgivings, it would be helpful for you to follow through on it when I find and fix its implementation bugs :-)

Do any other maintainers have an opinion for/against this, or comments on my patches?
Comment 25 David Zeuthen (not reading bugmail) 2014-10-09 20:27:56 UTC
> Ping? If you're going to push for a change sufficiently enthusiastically to
> convince me that it's worthwhile despite my initial misgivings, it would be
> helpful for you to follow through on it when I find and fix its implementation
> bugs :-)

Sure ... but before I do that, let me just check whether the idea is to squash Bertrand's original patch and your following six patches together and commit it as one?  (for e.g. bisecting, that might be helpful)
Comment 26 Simon McVittie 2014-10-10 10:32:41 UTC
(In reply to David Zeuthen from comment #25)
> Sure ... but before I do that, let me just check whether the idea is to
> squash Bertrand's original patch and your following six patches together and
> commit it as one?  (for e.g. bisecting, that might be helpful)

If you want, but I generally prefer small-enough-to-be-comprehensible commits over strict bisectability (which is why 2 and 3 are separate).

Squashing in patches 2 and 3 is enough to be bisectable on Linux with Autotools; 5 and 6 are necessary to be bisectable on Windows with Autotools; 4 might be necessary when building with cmake; and 5 and 7 can go to the beginning if desired, I just happened to notice those existing issues in my mingw build environment, which has an older GLib.
Comment 27 Simon McVittie 2014-12-18 15:24:40 UTC
(In reply to Simon McVittie from comment #24)
> Ping? If you're going to push for a change sufficiently enthusiastically to
> convince me that it's worthwhile despite my initial misgivings, it would be
> helpful for you to follow through on it when I find and fix its
> implementation bugs

This still stands. Do you want this change or don't you?

> Do any other maintainers have an opinion for/against this, or comments on my
> patches?

Also still welcome.

> before I do that, let me just check whether the idea is to
> squash Bertrand's original patch and your following six patches together and
> commit it as one?

Please assume the answer is "whichever will get someone to review it". I have a weak preference for not squashing together these changes but could go either way.
Comment 28 Ralf Habacker 2015-02-10 18:58:31 UTC
I tried to apply the patches and recognized that they are outdated and need to be rebased.
Comment 29 Simon McVittie 2015-02-10 19:51:38 UTC
(In reply to Ralf Habacker from comment #28)
> I tried to apply the patches and recognized that they are outdated and need
> to be rebased.

Not necessarily surprising; a lot has happened in 4 months. Rebasing.
Comment 30 Ralf Habacker 2015-02-12 22:14:50 UTC
Created attachment 113423 [details] [review]
[patch 4] Link libdbus-internal to the shared libdbus-1 under CMake, too (rebased)
Comment 31 Ralf Habacker 2015-02-12 22:15:36 UTC
Created attachment 113424 [details] [review]
[patch 6] Explicitly export all symbols needed to compile the tests for Windows (rebased)
Comment 32 Simon McVittie 2015-02-13 10:18:58 UTC
(In reply to Ralf Habacker from comment #30)
> [patch 4] Link libdbus-internal to the shared libdbus-1 under CMake, too
> (rebased)

Thanks! Do the rest apply unaltered? I'll see what happens...
Comment 33 Ralf Habacker 2015-02-13 10:22:16 UTC
(In reply to Simon McVittie from comment #32)
> (In reply to Ralf Habacker from comment #30)
> > [patch 4] Link libdbus-internal to the shared libdbus-1 under CMake, too
> > (rebased)
> 
> Thanks! Do the rest apply unaltered?
no
Comment 34 Simon McVittie 2015-02-13 11:14:05 UTC
(In reply to Ralf Habacker from comment #33)
> > Thanks! Do the rest apply unaltered?
>
> no

OK, I'll look into rebasing the whole series when I get a chance, using your updated versions where appropriate.
Comment 35 Ralf Habacker 2015-02-13 12:21:17 UTC
Comment on attachment 113424 [details] [review]
[patch 6] Explicitly export all symbols needed to compile the tests for Windows (rebased)

Review of attachment 113424 [details] [review]:
-----------------------------------------------------------------

This patch decorates only currently-in-use functions with a potential to break windows builds if executables requires additional internal functions. It should be considered to decorate *all* internal functions at once.
Comment 36 Simon McVittie 2015-02-18 20:29:44 UTC
(In reply to Ralf Habacker from comment #35)
> This patch decorates only currently-in-use functions with a potential to
> break windows builds if executables requires additional internal functions.
> It should be considered to decorate *all* internal functions at once.

That's going to be a huge diff. I'd rather stick to "someone tries the Windows builds somewhat frequently" - I have a mingw build environment back up now.

(In reply to Simon McVittie from comment #22)
> If we go this way, when compiling with gcc for non-Windows, I'm vaguely
> tempted to use -fvisibility=hidden and define DBUS_EXPORT and
> DBUS_PRIVATE_EXPORT to __attribute__((visibility("default"))) while
> compiling the library, which have essentially the same result as what we
> have to do on Windows anyway.

If I can make this work, then anything that would break Windows would also break Unix gcc builds, and intra-library calls to non-exported symbols would become somewhat faster on Unix (because the compiler can generate less pessimal code for them if it doesn't have to think about the possibility of external callers or symbol-interposing).
Comment 37 Ralf Habacker 2015-02-19 20:45:51 UTC
Created attachment 113673 [details] [review]
[PATCH] Export private functions required when enabled verbose mode.

 dbus/dbus-connection-internal.h | 1 +
 dbus/dbus-internals.h           | 5 +++++
 2 files changed, 6 insertions(+)
Comment 38 Simon McVittie 2015-02-20 12:55:37 UTC
(In reply to Ralf Habacker from comment #37)
> [PATCH] Export private functions required when enabled verbose mode.

Thanks, I'll look into this when I've done the necessary rebase for this stuff to work again.
Comment 39 Ralf Habacker 2015-02-20 15:04:14 UTC
Comment on attachment 106150 [details] [review]
[patch 4] Link libdbus-internal to the shared libdbus-1 under CMake,  too

Review of attachment 106150 [details] [review]:
-----------------------------------------------------------------

::: cmake/dbus/CMakeLists.txt
@@ +288,5 @@
> +    if("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux")
> +        set(SOVERSION ${DBUS_LIBRARY_MAJOR})
> +        configure_file(${DBUS_DIR}/Version.in ${CMAKE_CURRENT_BINARY_DIR}/Version)
> +        set_target_properties(dbus-1 PROPERTIES LINK_FLAGS -Wl,--version-script=${CMAKE_CURRENT_BINARY_DIR}/Version)
> +    endif("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux")

This hunk is not included in attachment 113424 [details] [review] because Version.in is included in a not yet rebased autotools patch.
Comment 40 Simon McVittie 2015-02-20 15:25:35 UTC
Created attachment 113686 [details] [review]
[3/6] tests: always use libdbus-internal for main loop, never  dbus-glib

This gets rid of a potential circular dependency, which is annoying
when bootstrapping. It is nice to have the regression tests use
the shared libdbus, but we're about to make it possible to
do that anyway, even though some of them use internal symbols.

---

This should not be applied until the next few patches are also ready.
Comment 41 Simon McVittie 2015-02-20 15:27:12 UTC
Created attachment 113687 [details] [review]
[4/6] Add DBUS_PRIVATE_EXPORT decoration to symbols used by  dbus-daemon or tests

The rules are:

* symbols in libdbus-1 with neither decoration are private to libdbus-1
* symbols in libdbus-1 with DBUS_EXPORT are public API
* symbols in libdbus-1 with DBUS_PRIVATE_EXPORT are private to the
  dbus source package, but may be used by other programs in the dbus
  source tree, including tests
* symbols in libdbus-internal must not have DBUS_EXPORT or
  DBUS_PRIVATE_EXPORT, and should be used by as few things as possible

Thanks to Ralf Habacker for his contributions to this rather
large commit.

---

This incorporates Attachment #113424 [details] and Attachment #113673 [details], thanks for those!

The first bullet point is not enforced on Unix platforms by this commit, but will be enforced by the next one.
Comment 42 Simon McVittie 2015-02-20 15:28:19 UTC
Created attachment 113688 [details] [review]
[5/6] On Unix platforms with gcc (or compatible), hide  non-exported symbols

This changes the Linux behaviour to match the default situation
on Windows: symbols without DBUS_EXPORT or DBUS_PRIVATE_EXPORT
decoration are internal to libdbus-1, and cannot be used by
other programs, even within the dbus source tree.

This means the compiler/linker can optimize calls to those functions
by avoiding indirection through the PLT, which should improve
performance a little. However, the primary purpose of doing this is
that it means developers building libdbus on Linux are considerably
less likely to break it on Windows by mistake.

I'm deliberately not adding -fvisbility=hidden in CMake because the
complexity of doing so is unnecessary: Autotools is the recommended
way to build dbus for Unix, and the one Unix developers are going
to use in practice, unless they are specifically checking that they
haven't broken the CMake build.
Comment 43 Simon McVittie 2015-02-20 15:35:18 UTC
Created attachment 113690 [details] [review]
[6/6] Link dbus-daemon and dbus-daemon-lauch-helper against  libdbus

From: Bertrand SIMONNET <bsimonnet@chromium.org>

The shared can be used by dbus-daemon and dbus-daemon-launch-helper by exporting
the private symbols needed, reducing the size of dbus by about 500k.
The private symbols are exposed under the version
LIBDBUS_PRIVATE_@VERSION_NUMBER@.

[Altered by Simon McVittie and Ralf Habacker to clear up some
problematic linking.]

---

Tests pass on Linux with Autotools and CMake. Also compile-tested with mingw-w64 Win32 cross-compiler under Autotools and CMake.
Comment 44 Simon McVittie 2015-02-20 16:15:56 UTC
Created attachment 113691 [details] [review]
Fix static linking with mingw

Now that we're normally linking libdbus-1 dynamically, we need to
use DBUS_STATIC_BUILD_CPPFLAGS in every Makefile that would normally
link it dynamically, but might link it statically if we are only
building static libraries.

---

Yes, this is quite a corner-case.
Comment 45 Simon McVittie 2015-02-20 16:21:30 UTC
If we are going to do this, could someone please review the patches in the near future, so that they don't bit-rot and need to be rebased again in another 4 months? This is the sort of change that cannot be done without touching lots of places in the source tree, so it will conflict with virtually anything else we do.
Comment 46 Ralf Habacker 2015-02-20 16:57:08 UTC
Comment on attachment 106687 [details] [review]
[1/6] Remove some redundant inclusions

Review of attachment 106687 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 47 Ralf Habacker 2015-02-20 16:57:59 UTC
Comment on attachment 106689 [details] [review]
[2/6] sysdeps: try to avoid re-including config.h

Review of attachment 106689 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 48 Simon McVittie 2015-02-20 17:56:52 UTC
(In reply to Ralf Habacker from comment #39)
> > +        set_target_properties(dbus-1 PROPERTIES LINK_FLAGS -Wl,--version-
> 
> This hunk is not included in attachment 113424 [details] [review] because
> Version.in is included in a not yet rebased autotools patch.

By the same reasoning as not using -fvisibility=hidden (see 5/6), I don't think it's mandatory to have the CMake build do this (and I haven't done so here). We could add it afterwards, if you want.
Comment 49 Simon McVittie 2015-02-20 18:01:09 UTC
Created attachment 113692 [details] [review]
cmake: check for the necessary symbols for test-segfault.c

If we don't check for them, and you have core dumps enabled, then
running this test under cmake is really annoying, because it leaves
lots of core dumps none of which are actually a problem.

The equivalent Autotools change (which added the actual code that
this relies on) is commit ae50d46, from Bug #83772.
Comment 50 Ralf Habacker 2015-02-20 18:29:22 UTC
Comment on attachment 113686 [details] [review]
[3/6] tests: always use libdbus-internal for main loop, never  dbus-glib

Review of attachment 113686 [details] [review]:
-----------------------------------------------------------------

it simplifies code - looks good
Comment 51 Ralf Habacker 2015-02-20 18:30:50 UTC
Comment on attachment 113687 [details] [review]
[4/6] Add DBUS_PRIVATE_EXPORT decoration to symbols used by  dbus-daemon or tests

Review of attachment 113687 [details] [review]:
-----------------------------------------------------------------

cross compiles using i686-w64-mingw32 without any problems - looks good
Comment 52 Ralf Habacker 2015-02-20 18:51:17 UTC
Comment on attachment 113688 [details] [review]
[5/6] On Unix platforms with gcc (or compatible), hide  non-exported symbols

Review of attachment 113688 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 53 Ralf Habacker 2015-02-20 18:52:08 UTC
Comment on attachment 113690 [details] [review]
[6/6] Link dbus-daemon and dbus-daemon-lauch-helper against  libdbus

Review of attachment 113690 [details] [review]:
-----------------------------------------------------------------

Compiled with autotools and got from objdump -T libdbus-1.so
... 
Version definitions:
1 0x01 0x0fcf1b93 libdbus-1.so.3
2 0x00 0x07f9f1a3 LIBDBUS_1_3
3 0x00 0x0cb450e3 LIBDBUS_PRIVATE_1.9.13
... 
0000000000018540 g    DF .text  0000000000000072  LIBDBUS_1_3 dbus_connection_set_max_received_size
0000000000011d30 g    DF .text  000000000000001d  LIBDBUS_PRIVATE_1.9.13 _dbus_auth_set_context
0000000000022e10 g    DF .text  000000000000006f  LIBDBUS_PRIVATE_1.9.13 _dbus_message_loader_unref
000000000002de10 g    DF .text  00000000000000d6  LIBDBUS_PRIVATE_1.9.13 _dbus_warn
000000000002d630 g    DF .text  0000000000000009  LIBDBUS_PRIVATE_1.9.13 _dbus_hash_iter_get_value

looks good
Comment 54 Ralf Habacker 2015-02-20 18:55:24 UTC
Comment on attachment 113692 [details] [review]
cmake: check for the necessary symbols for test-segfault.c

Review of attachment 113692 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 55 Ralf Habacker 2015-02-20 18:57:14 UTC
Comment on attachment 113691 [details] [review]
Fix static linking with mingw

Review of attachment 113691 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 56 Simon McVittie 2015-02-20 21:17:07 UTC
Committed all those, thanks!

I'll leave this open for a couple of bits of cleanup.
Comment 57 Simon McVittie 2015-02-20 21:17:51 UTC
Created attachment 113697 [details] [review]
Remove checks for dbus-glib from configure.ac
Comment 58 Simon McVittie 2015-02-20 21:18:08 UTC
Created attachment 113698 [details] [review]
tests: simplify Makefile.am now that libdbus is always  dynamically linked

testutils_shared_if_possible_cppflags is now just a copy of AM_CPPFLAGS,
which is the default and does not need to be given explicitly, so
those lines can be removed.

Similarly, testutils_shared_if_possible_libs is just the
libdbus-testutils.la convenience library, so expand it and
remove the unnecessary variable.
Comment 59 Ralf Habacker 2015-02-20 22:14:47 UTC
Created attachment 113702 [details] [review]
0001-Add-versioned-symbol-support-to-cmake-build-system-f.patch
Comment 60 Ralf Habacker 2015-02-20 22:19:51 UTC
(In reply to Simon McVittie from comment #48)
> We could add it afterwards, if you want.

yes, it is better to keep binaries in sync between autotools and cmake. see attachment 113702 [details] [review]
Comment 61 Simon McVittie 2015-02-20 22:22:24 UTC
Comment on attachment 113702 [details] [review]
0001-Add-versioned-symbol-support-to-cmake-build-system-f.patch

Review of attachment 113702 [details] [review]:
-----------------------------------------------------------------

Sure, feel free to merge it.
Comment 62 Ralf Habacker 2015-02-20 22:29:12 UTC
(In reply to Simon McVittie from comment #61)
> Comment on attachment 113702 [details] [review] [review]
> 0001-Add-versioned-symbol-support-to-cmake-build-system-f.patch
> 
> Review of attachment 113702 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Sure, feel free to merge it.

Because you wrote the patch I committed it with a review from me.
Comment 63 Ralf Habacker 2015-02-20 22:31:08 UTC
Comment on attachment 113698 [details] [review]
tests: simplify Makefile.am now that libdbus is always  dynamically linked

Review of attachment 113698 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 64 Ralf Habacker 2015-02-20 22:33:56 UTC
Comment on attachment 113697 [details] [review]
Remove checks for dbus-glib from configure.ac

Review of attachment 113697 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 65 Simon McVittie 2015-02-24 12:18:30 UTC
All fixed in git for 1.9.14, thanks.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.