Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

Create stripd version of shared library during build #28

Closed
wants to merge 1 commit into from

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Feb 22, 2022

What does this PR do?

Changes ffi-build.sh to create two versions of the shared library: one that has been stripd, and another which hasn't.

Motivation

By having the build generate both a stripd and an unstrip'd version of the shared library, we make it easier to package a stripd libddprof without needing access to all architectures and OS's used to build the binaries.

In particular for Ruby, it allows the Ruby packaging process to be just a straightforward unpacking + uploading to rubygems.org, without any kind of changes to the libddprof files.

It also makes it easier to validate the resulting libddprof distribution: it should match the exact files present in the release page on GitHub.

Finally, there was a bit of discussion if we should ship both stripd and unstrip'd, or only ship the stripd version but I like having both since it makes it easy to package a version with full debug info if we need to investigate an issue. (Totally open to switching to the other option)

How to test the change?

Exercise the stripd version of libddprof shared library by linking to, and using it.

By having the build generate both a `strip`d and an unstrip'd version
of the shared library, we make it easier to package a `strip`d libddprof
without needing access to all architectures and OS's used to build
the binaries.

In particular for Ruby, it allows the Ruby packaging process to be
just a straightforward unpacking + uploading to rubygems.org, without
any kind of changes to the libddprof files.

It also makes it easier to validate the resulting libddprof
distribution: it should match the exact files present in the release
page on GitHub.

Finally, there was a bit of discussion if we should ship both
`strip`d and unstrip'd, or only ship the `strip`d version but I like
having both since it makes it easy to package a version with full
debug info if we need to investigate an issue.
Copy link
Collaborator

@sanchda sanchda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-review edits, since I've since done some learning.
EDIT: this stuff is wrong
If you're already on an ELF-based system, strip should work regardless of the architecture of the target. What are the practicality hurdles to pushing strip one level, to the package maintainers et al? You made a not of it in the motivation, but some more discussion would help me understand...

Another idea is to ship split debuginfo, which would be my personal preference, but I won't claim to have a strong position on that preference.

EDIT: this is an opinion, so it isn't wrong, although it is maybe silly:
I'd approve outright, but I have the tiniest aesthetic concern about shipping so many variants of the same object code! Maybe we can approve this and also approve a move away from alpine/glibc-tagged builds? :)

@morrisonlevi
Copy link
Collaborator

Why does Ruby need a stripped and unstripped version? I don't understand.

@ivoanjo
Copy link
Member Author

ivoanjo commented Feb 23, 2022

As @sanchda pointed out, it's kinda weird but even on elf systems the strip seems to be platform-specific. The amd64 strip from Ubuntu refuses to strip the arm64 binary.

Another idea is to ship split debuginfo, which would be my personal preference, but I won't claim to have a strong position on that preference.

This is a very good point. I'll look into it, and possibly create a PR with it so we can evaluate which option we prefer.

Maybe we can approve this and also approve a move away from alpine/glibc-tagged builds? :)

One can dream ;D


Why does Ruby need a stripped and unstripped version? I don't understand.

You're right, it doesn't!

My thinking is that shipping both in the release tarball enables us to easily switch between the stripd version and one with debug info without needing to reproduce the build for a given/specific libddprof version. Otherwise, we may risk one of those "once I rebuilt it, the issue went away" issues.

Of course, the Ruby packaging would only include one or the other. I'm thinking if a customer runs into an issue, I can just repackage the version with debug info and ask them to switch.

I agree it's a heavy-handed approach. I'll look into David's suggestion of splitting the debuginfo instead and report back :)

ivoanjo added a commit that referenced this pull request Feb 23, 2022
By shipping the debug information in a separate file, downstream
packages (e.g. the Ruby packages) can easily remove the debug
information while packaging libddprof, but it's still very easy to
restore it.

This approach is documented in
<https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html>
and in the `objdump` man pages. Thanks @nsavoire for the tips.

The `strip`'d file retains the `build-id` information.

I've tested with `gdb` and it seems to work:

```
$ gdb libddprof_ffi.so
GNU gdb (Ubuntu 11.1-0ubuntu2) 11.1

Reading symbols from libddprof_ffi.so...
Reading symbols from /working/build-linux-amd64/lib/libddprof_ffi.so.debug...

(gdb) info functions
All defined functions:

File /cargo/registry/src/git.luolix.top-1ecc6299db9ec823/addr2line-0.13.0/src/lib.rs:
966:	static fn addr2line::Function::parse_children<gimli::read::endian_slice::EndianSlice<gimli::endianity::LittleEndian>>();
549:	static fn addr2line::ResUnit::parse_functions<gimli::read::endian_slice::EndianSlice<gimli::endianity::LittleEndian>>();
480:	static fn addr2line::ResUnit::parse_lines<gimli::read::endian_slice::EndianSlice<gimli::endianity::LittleEndian>>();
660:	static fn addr2line::name_attr<gimli::read::endian_slice::EndianSlice<gimli::endianity::LittleEndian>>();
1162:	static fn addr2line::{{impl}}::for_each_range::{{closure}}<gimli::read::endian_slice::EndianSlice<gimli::endianity::LittleEndian>,closure-0>();

File /cargo/registry/src/git.luolix.top-1ecc6299db9ec823/compiler_builtins-0.1.32/src/macros.rs:
262:	fn compiler_builtins::int::udiv::__udivmodti4();
269:	fn compiler_builtins::int::udiv::__umodti3::__umodti3();
...etc...
```

and if the `.debug` file is deleted:

```
(gdb) info functions
All defined functions:

Non-debugging symbols:
0x0000000000075d00  _init
0x0000000000075d30  __stack_chk_fail@plt
0x0000000000075d40  __tls_get_addr@plt
0x0000000000075d50  __fxstat64@plt
0x0000000000075d60  __gmon_start__@plt
0x0000000000075d70  _Unwind_Resume@plt
0x0000000000075d80  __cxa_finalize@plt
0x0000000000075dc0  deregister_tm_clones
0x0000000000075df0  register_tm_clones
0x0000000000075e30  __do_global_dtors_aux
0x0000000000075e70  frame_dummy
0x0000000000075ea5  std::error::<impl core::convert::From<E> for alloc::boxed::Box<dyn std::error::Error>>::from
0x0000000000075edd  std::error::Error::description
0x0000000000075eea  std::error::Error::cause
...etc...
```

This PR is an alternative to #28, which instead shipped two copies
of the shared library binaries (one `strip`d, and an untouched one).
ivoanjo added a commit that referenced this pull request Feb 23, 2022
By shipping the debug information in a separate file, downstream
packages (e.g. the Ruby packages) can easily remove the debug
information while packaging libddprof, but it's still very easy to
restore it.

This approach is documented in
<https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html>
and in the `objdump` man pages. Thanks @nsavoire for the tips.

The `strip`'d file retains the `build-id` information.

I've tested with `gdb` and it seems to work:

```
$ gdb libddprof_ffi.so
GNU gdb (Ubuntu 11.1-0ubuntu2) 11.1

Reading symbols from libddprof_ffi.so...
Reading symbols from /working/build-linux-amd64/lib/libddprof_ffi.so.debug...

(gdb) info functions
All defined functions:

File /cargo/registry/src/git.luolix.top-1ecc6299db9ec823/addr2line-0.13.0/src/lib.rs:
966:	static fn addr2line::Function::parse_children<gimli::read::endian_slice::EndianSlice<gimli::endianity::LittleEndian>>();
549:	static fn addr2line::ResUnit::parse_functions<gimli::read::endian_slice::EndianSlice<gimli::endianity::LittleEndian>>();
480:	static fn addr2line::ResUnit::parse_lines<gimli::read::endian_slice::EndianSlice<gimli::endianity::LittleEndian>>();
660:	static fn addr2line::name_attr<gimli::read::endian_slice::EndianSlice<gimli::endianity::LittleEndian>>();
1162:	static fn addr2line::{{impl}}::for_each_range::{{closure}}<gimli::read::endian_slice::EndianSlice<gimli::endianity::LittleEndian>,closure-0>();

File /cargo/registry/src/git.luolix.top-1ecc6299db9ec823/compiler_builtins-0.1.32/src/macros.rs:
262:	fn compiler_builtins::int::udiv::__udivmodti4();
269:	fn compiler_builtins::int::udiv::__umodti3::__umodti3();
...etc...
```

and if the `.debug` file is deleted:

```
(gdb) info functions
All defined functions:

Non-debugging symbols:
0x0000000000075d00  _init
0x0000000000075d30  __stack_chk_fail@plt
0x0000000000075d40  __tls_get_addr@plt
0x0000000000075d50  __fxstat64@plt
0x0000000000075d60  __gmon_start__@plt
0x0000000000075d70  _Unwind_Resume@plt
0x0000000000075d80  __cxa_finalize@plt
0x0000000000075dc0  deregister_tm_clones
0x0000000000075df0  register_tm_clones
0x0000000000075e30  __do_global_dtors_aux
0x0000000000075e70  frame_dummy
0x0000000000075ea5  std::error::<impl core::convert::From<E> for alloc::boxed::Box<dyn std::error::Error>>::from
0x0000000000075edd  std::error::Error::description
0x0000000000075eea  std::error::Error::cause
...etc...
```

This PR is an alternative to #28, which instead shipped two copies
of the shared library binaries (one `strip`d, and an untouched one).
@ivoanjo
Copy link
Member Author

ivoanjo commented Feb 23, 2022

Replaced with #29, closing!

@ivoanjo ivoanjo closed this Feb 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants