Deliver libddprof_ffi as a shared library by nsavoire · Pull Request #22 · DataDog/libddprof

nsavoire · 2022-02-08T13:35:23Z

What does this PR do?

Deliver libddprof_ffi as a shared library
Activate LTO and reduce number of codegen units to 1
Add a simple exporter example executable

Motivation

Some clients prefer having a shared library rather than a static one.

r1viollet · 2022-02-09T10:11:01Z

 [lib]
-crate-type = ["lib", "staticlib"]
+# LTO is ignored if "lib" is added as crate type
+# cf. https://github.com/rust-lang/rust/issues/51009


nice referencing this

r1viollet · 2022-02-09T10:16:47Z

    "x86_64-apple-darwin")
        expected_native_static_libs=" -framework Security -liconv -lSystem -lresolv -lc -lm -liconv"
        native_static_libs="${expected_native_static_libs}"
+        shared_library_suffix=".dylib"


Thanks for checking the mac os builds !

r1viollet · 2022-02-09T12:25:33Z

 [profile.release]
 debug = 1 # line tables only
+lto = true
+codegen-units = 1


Is this something we want to put in the cargo config, or could we not make sure to have it as a command line argument in the CI ?
I'm thinking we dan't want to slow all of our build times. Only the build time to generate the github release.
WDYT ?

As discussed, locally we will use the develop profile. We can leave the release profile optimised for size.

morrisonlevi · 2022-02-09T17:10:25Z

@@ -46,6 +48,10 @@ sed < ddprof_ffi.pc.in "s/@DDProf_FFI_VERSION@/${version}/g" \
    | sed "s/@DDProf_FFI_LIBRARIES@/${native_static_libs}/g" \
    > "$destdir/lib/pkgconfig/ddprof_ffi.pc"


The concept of "native static libs" doesn't apply to a shared object. There will be dependencies, of course, but this isn't how we should be populating them for the .so.

Thanks for pointing this out. I did put them in Libs.private when I had still hope to be able to make a single pkg-config file for both shared and static library, but since I could not do it, it's better to completely remove them for the shared library.

r1viollet

LGTM !

morrisonlevi · 2022-02-11T17:53:24Z

The shared library artifact for alpine is suspicious, particularly the runpath:

# readelf -d ./libddprof-x86_64-alpine-linux-musl/lib/libddprof_ffi.so

Dynamic section at offset 0x4cf8f8 contains 24 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libc.musl-x86_64.so.1]
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/:$ORIGIN/../../../../../../../../../../usr/lib/rustlib/x86_64-alpine-linux-musl/lib:/usr/lib/rustlib/x86_64-alpine-linux-musl/lib]

nsavoire · 2022-02-11T20:37:23Z

The shared library artifact for alpine is suspicious, particularly the runpath:

# readelf -d ./libddprof-x86_64-alpine-linux-musl/lib/libddprof_ffi.so

Dynamic section at offset 0x4cf8f8 contains 24 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libc.musl-x86_64.so.1]
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/:$ORIGIN/../../../../../../../../../../usr/lib/rustlib/x86_64-alpine-linux-musl/lib:/usr/lib/rustlib/x86_64-alpine-linux-musl/lib]

Hum, indeed this is strange. But I don't expect this to be an issue, the runpath here is not useful to find dependencies since libddprof_ffi.so depends only on libgcc/libc.
Adding rpath/runpath to cdylib seems specific to alpine-musl:

We could remove runpath from the lib just to be extra cautious. WDYT @morrisonlevi ?

* Reduce number of codegen units to further reduce ouput library size. * Deliver shared library alongside static library

Create ddprof_ffi-static.pc for linking with static library

…ddprof_ffi-static.pc for static linking

…sary for dynamic linking

Rust adds some weird RPATH/RUNPATH to cdylibs on alpine-musl (cf. https://git.alpinelinux.org/aports/tree/community/rust/alpine-target.patch,https://git.alpinelinux.org/aports/tree/community/rust/need-rpath.patch).

ivoanjo

Thanks for this! I left two tiny notes, and I'll try to wire this up in Ruby and report back with the experience :)

ivoanjo · 2022-02-14T12:22:52Z

+static ddprof_ffi_ByteSlice to_byteslice(const char *s)
+{
+    return {.ptr = (uint8_t *)s,
+            .len = strlen(s)};
+}
+
+static ddprof_ffi_Slice_c_char to_slice_c_char(const char *s)
+{
+    return {.ptr = s,
+            .len = strlen(s)};
+}


Not related to this PR, but this looks to me as a weird sharp edge in the libddprof API.

It's weird to have both ddprof_ffi_ByteSlice and ddprof_ffi_Slice_c_char when they are effectively the same (?).

Also, I had to write similar helpers for using libddprof, so perhaps it would make sense to export these (possibly as macros?) in ffi.h as well :)

You usually don't want to encourage people to transport strings without an actual size associated to it. strlen is also not something you want encourage, so I don't know how I feel about exposing these types of APIs.
I guess we can also mention this :-)

Hmmm... I'm guessing the "don't want to encourage" may be related to performance -- is that the case?

If so, would it make sense to call it something like to_slice_slow? I guess for a bunch of hardcoded strings ("ruby", "wall-clock", "nanoseconds", "service", ...) we just use them once so the cost is trivial so we could have the ease of use instead :)

I agree there is room for improvements concerning the APIs, we could provide some helpers function like to_slice_xxx on top of FFI headers. Maybe even a more friendly C++ interface.

It's strange that slices for type/unit/filename/build_id/name/system_name/... are different from slices used for /tags/filenames.

But this would be subject of another pull request ;)

I don't feel super strongly about this, but I would usually avoid relying on C style null terminated strings. Though it is not clear how we could avoid these conversions at boundaries or how to have a common types.

morrisonlevi

Did we check Alpine builds? I remember the compiler telling me it can't make shared libs on Alpine.

nsavoire · 2022-02-15T16:38:54Z

Yes, there is a test in CI that try linking why shared library.
x86_64-unknown-linux-musl refuses to create cdylibs but x86_64-alpine-linux-musl has specific patches to allow it.

nsavoire requested review from morrisonlevi and r1viollet February 8, 2022 13:35

r1viollet reviewed Feb 9, 2022

View reviewed changes

Comment thread examples/ffi/CMakeLists.txt

r1viollet reviewed Feb 9, 2022

View reviewed changes

morrisonlevi reviewed Feb 9, 2022

View reviewed changes

r1viollet approved these changes Feb 10, 2022

View reviewed changes

nsavoire added 7 commits February 11, 2022 21:42

Build ddprof-ffi as a shared library

0366c81

Add exporter example

1d3b254

Activate LTO

d0575dd

* Reduce number of codegen units to further reduce ouput library size. * Deliver shared library alongside static library

Update license file

9962404

Make ddprof_ffi.pc link with shared library by default

aa16b91

Create ddprof_ffi-static.pc for linking with static library

Make dynamic linking the default for libddprof_ffi.pc and provide lib…

6041f3d

…ddprof_ffi-static.pc for static linking

Remove native static libs from ddprof_ffi.pc since they are not neces…

e1542ff

…sary for dynamic linking

nsavoire force-pushed the nsavoire/deliver_shared_library branch 2 times, most recently from b386c52 to a3e8035 Compare February 14, 2022 10:14

nsavoire added 2 commits February 14, 2022 11:35

Optimize for size instead of speed

282da24

nsavoire force-pushed the nsavoire/deliver_shared_library branch from a3e8035 to 06174b3 Compare February 14, 2022 10:36

ivoanjo reviewed Feb 14, 2022

View reviewed changes

Fix OSX -> macOS

68b2c75

morrisonlevi approved these changes Feb 15, 2022

View reviewed changes

nsavoire merged commit ea737a1 into main Feb 15, 2022

nsavoire deleted the nsavoire/deliver_shared_library branch February 15, 2022 16:50

		@@ -46,6 +48,10 @@ sed < ddprof_ffi.pc.in "s/@DDProf_FFI_VERSION@/${version}/g" \
		\| sed "s/@DDProf_FFI_LIBRARIES@/${native_static_libs}/g" \
		> "$destdir/lib/pkgconfig/ddprof_ffi.pc"

Uh oh!

Conversation

nsavoire commented Feb 8, 2022 • edited by ivoanjo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r1viollet left a comment

Choose a reason for hiding this comment

Uh oh!

morrisonlevi commented Feb 11, 2022

Uh oh!

nsavoire commented Feb 11, 2022

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

morrisonlevi left a comment

Choose a reason for hiding this comment

Uh oh!

nsavoire commented Feb 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nsavoire commented Feb 8, 2022 •

edited by ivoanjo

Loading