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

Support zstd-compressed ELF sections. #625

Closed
wants to merge 2 commits into from
Closed

Conversation

khuey
Copy link
Contributor

@khuey khuey commented May 24, 2024

zstd has been introduced as an alternative to zlib for the compression of debug sections.[0] Toolchain support is widely present at this time but lack of support in backtrace is a severe limitation on using this feature in Rust programs.

Unlike zlib compression, the most widely used Rust library for zstd is a set of bindings to the zstd C library. There does exist a Rust reimplementation (ruzstd) however the author claims it is significantly slower than the C library and it sees substantially less usage, so I have not used it here.

Using the C library poses challenges because it requires compiling native code. I made a few adjustments to the CI configuration to make that possible on all platforms, and pinned the zstd crate to a slightly older version due to gyscos/zstd-rs#271 breaking the crate on wasm targets on the very latest version. Additionally, I've disabled this entirely on illumos rather than try to get native code to compile successfully there.

[0] https://maskray.me/blog/2022-09-09-zstd-compressed-debug-sections

zstd has been introduced as an alternative to zlib for the compression of debug
sections.[0] Toolchain support is widely present at this time but lack of
support in backtrace is a severe limitation on using this feature in Rust
programs.

Unlike zlib compression, the most widely used Rust library for zstd is a set
of bindings to the zstd C library. There does exist a Rust reimplementation
(ruzstd) however the author claims it is significantly slower than the C
library and it sees substantially less usage, so I have not used it here.

Using the C library poses challenges because it requires compiling native
code. I made a few adjustments to the CI configuration to make that possible
on all platforms, and pinned the zstd crate to a slightly older version
due to gyscos/zstd-rs#271 breaking the crate on wasm targets on the very
latest version. Additionally, I've disabled this entirely on illumos rather
than try to get native code to compile successfully there.

[0] https://maskray.me/blog/2022-09-09-zstd-compressed-debug-sections
@khuey
Copy link
Contributor Author

khuey commented May 24, 2024

Perhaps @jclulow would be interested in figuring out how to get the zstd crate to build on illumos.

@@ -44,6 +44,9 @@ miniz_oxide = { version = "0.7.0", default-features = false }
addr2line = { version = "0.22.0", default-features = false }
libc = { version = "0.2.146", default-features = false }

[target.'cfg(not(any(all(windows, target_env = "msvc", not(target_vendor = "uwp")), target_os = "illumos")))'.dependencies]
zstd = { version = "= 0.13.0", default-features = false }
Copy link
Member

@bjorn3 bjorn3 May 24, 2024

Choose a reason for hiding this comment

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

Using the C implementation of zstd makes cross-compilation harder, especially for miri which currently is able to test for any supported target without installing any toolchain for the target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I discussed that in the commit message. I could disable this for miri too.

Cargo.toml Outdated
@@ -44,6 +44,9 @@ miniz_oxide = { version = "0.7.0", default-features = false }
addr2line = { version = "0.22.0", default-features = false }
libc = { version = "0.2.146", default-features = false }

[target.'cfg(not(any(all(windows, target_env = "msvc", not(target_vendor = "uwp")), target_os = "illumos")))'.dependencies]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this dependency added for uwp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not. Unless I screwed something up that part of the condition is copied from the section above. What's new here is that illumos is excluded as well.

Copy link
Member

Choose a reason for hiding this comment

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

The conditional evaluates as follows on x86_64-uwp-windows-msvc:

  • not(any(all(windows, target_env = "msvc", not(target_vendor = "uwp")), target_os = "illumos"))
    • any(all(windows, target_env = "msvc", not(target_vendor = "uwp")), target_os = "illumos")
      • all(windows, target_env = "msvc", not(target_vendor = "uwp"))
        • windows
        • target_env = "msvc"
        • not(target_vendor = "uwp")
          • target_vendor = "uwp"
      • target_os = "illumos"

So it will include zstd on x86_64-uwp-windows-msvc.

Copy link
Member

Choose a reason for hiding this comment

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

Why is Windows included at all? Surely even gnu does not use elf sections due to the binary format being different on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, you're right. This is what I get for commenting at 6:30 AM.

The uwp part was copied from the above section with miniz_oxide, addr2line, and libc. That exclusion was added in #543 without explanation.

The correct handling here would be for addr2line/libc to be gated on not using msvc (because mingw produces DWARF, not PDBs) and for miniz_oxide/zstd to be gated on not windows (because no Windows target uses ELF and these are for ELF compression).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I pushed a commit that cleans this up. Even if backtrace doesn't end up taking this PR it'll probably want something like that commit.

@pfmooney
Copy link

Perhaps @jclulow would be interested in figuring out how to get the zstd crate to build on illumos.

Out of curiosity, what failure(s) are you seeing in the build?

@khuey
Copy link
Contributor Author

khuey commented May 24, 2024

Perhaps @jclulow would be interested in figuring out how to get the zstd crate to build on illumos.

Out of curiosity, what failure(s) are you seeing in the build?

It dies complaining that gar is not available.

@pfmooney
Copy link

It dies complaining that gar is not available.

Which distribution are you attempting this on? On OmniOSCE, gar appears to be provided by pkg:/developer/gnu-binutils

@khuey
Copy link
Contributor Author

khuey commented May 24, 2024

It dies complaining that gar is not available.

Which distribution are you attempting this on? On OmniOSCE, gar appears to be provided by pkg:/developer/gnu-binutils

Based on https://github.com/rust-lang/backtrace-rs/blob/master/.github/workflows/main.yml#L220, I suppose it's attempting a cross-compile from standard Ubuntu to illumos.

@bjorn3
Copy link
Member

bjorn3 commented May 24, 2024

Unlike zlib compression, the most widely used Rust library for zstd is a set of bindings to the zstd C library. There does exist a Rust reimplementation (ruzstd) however the author claims it is significantly slower than the C library and it sees substantially less usage, so I have not used it here.

If the performance is still somewhat reasonable, I don't see it as too big of a deal to use it anyway. Backtrace symbolication is not on the performance critical path anyway (panics are supposed to be uncommon and only symbolize when RUST_BACKTRACE is set and errors with backtraces captured only symbolize when actually printed) and decompression should happen only once and cached for further symbolication queries.

@bjorn3
Copy link
Member

bjorn3 commented May 24, 2024

I just saw that the libzstd C library is licensed as BSD-3-Clause OR GPL-2.0. GPL-2.0 is definitively out for a standard library dependency. As for BSD-3-Clause, the rust compiler has some dependencies that use it, but it isn't in tidy's list of permitted licenses for the standard library. https://github.com/rust-lang/rust/blob/9e297bf54d31eb3b30067208ff9af4416945a2ed/src/tools/tidy/src/deps.rs#L12-L33 I will ask on zulip if it is fine to add to the list of allowed licenses.

Edit: https://rust-lang.zulipchat.com/#narrow/stream/231349-t-core.2Flicensing/topic/zstd.20dependency.20for.20the.20standard.20library

@Mark-Simulacrum
Copy link
Member

The README for ruzstd suggests a roughly 1.4x to 3.5x slowdown, and it seems significantly less complex to me -- it's really nice to have a pure-Rust standard library dependency tree, even on targets that usually have C compilers, for things like cross-compilation x.py check. I would be in favor of avoiding taking a dependency on real zstd in std, even if perhaps backtrace-rs does offer that (feature gated).

@luqmana
Copy link
Member

luqmana commented May 24, 2024

It dies complaining that gar is not available.

Which distribution are you attempting this on? On OmniOSCE, gar appears to be provided by pkg:/developer/gnu-binutils

Based on https://github.com/rust-lang/backtrace-rs/blob/master/.github/workflows/main.yml#L220, I suppose it's attempting a cross-compile from standard Ubuntu to illumos.

Yea, compiling on illumos itself works fine but cross-compiling would require a suitable illumos sysroot. One way is the approach used in the Rust repo to build it in docker:

0001-ci-build-x86_64-unknown-illumos-in-docker.patch
From 9d606a39eb9f342030d66a411262cb97a023da87 Mon Sep 17 00:00:00 2001
From: Luqman Aden <[email protected]>
Date: Fri, 24 May 2024 15:37:07 -0700
Subject: [PATCH] ci: build x86_64-unknown-illumos in docker

The zstd crate builds & links against the zstd c library. Crib the docker
image from the Rust repo for the illumos target.
---
 .github/workflows/main.yml                  |   2 +-
 Cargo.toml                                  |   2 -
 ci/docker/x86_64-unknown-illumos/Dockerfile |  34 ++++
 ci/illumos-toolchain.sh                     | 177 ++++++++++++++++++++
 crates/as-if-std/Cargo.toml                 |   2 -
 src/symbolize/gimli/elf.rs                  |   8 +-
 6 files changed, 215 insertions(+), 10 deletions(-)
 create mode 100644 ci/docker/x86_64-unknown-illumos/Dockerfile
 create mode 100644 ci/illumos-toolchain.sh

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index ca8a4b8..5309e76 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -210,6 +210,7 @@ jobs:
         - aarch64-linux-android
         - i686-linux-android
         - x86_64-linux-android
+        - x86_64-unknown-illumos
     steps:
     - uses: actions/checkout@v3
       with:
@@ -243,7 +244,6 @@ jobs:
         - wasm32-wasi
         - x86_64-unknown-fuchsia
         - x86_64-fortanix-unknown-sgx
-        - x86_64-unknown-illumos
     steps:
     - uses: actions/checkout@v3
       with:
diff --git a/Cargo.toml b/Cargo.toml
index 72d062c..c9af1e8 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -43,8 +43,6 @@ cpp_demangle = { default-features = false, version = "0.4.0", optional = true, f
 miniz_oxide = { version = "0.7.0", default-features = false }
 addr2line = { version = "0.22.0", default-features = false }
 libc = { version = "0.2.146", default-features = false }
-
-[target.'cfg(not(any(all(windows, target_env = "msvc", not(target_vendor = "uwp")), target_os = "illumos")))'.dependencies]
 zstd = { version = "= 0.13.0", default-features = false }
 
 [target.'cfg(not(all(windows, target_env = "msvc", not(target_vendor = "uwp"))))'.dependencies.object]
diff --git a/ci/docker/x86_64-unknown-illumos/Dockerfile b/ci/docker/x86_64-unknown-illumos/Dockerfile
new file mode 100644
index 0000000..31381f3
--- /dev/null
+++ b/ci/docker/x86_64-unknown-illumos/Dockerfile
@@ -0,0 +1,34 @@
+FROM ubuntu:20.04
+
+# Enable source repositories, which are disabled by default on Ubuntu >= 18.04
+RUN sed -i 's/^# deb-src/deb-src/' /etc/apt/sources.list
+
+RUN apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
+    automake \
+    bison \
+    bzip2 \
+    ca-certificates \
+    curl \
+    flex \
+    gawk \
+    gcc \
+    g++ \
+    libc6-dev \
+    libgmp-dev \
+    libmpfr-dev \
+    libmpc-dev \
+    make \
+    texinfo \
+    xz-utils
+
+COPY illumos-toolchain.sh /tmp/
+
+RUN bash /tmp/illumos-toolchain.sh x86_64 sysroot
+RUN bash /tmp/illumos-toolchain.sh x86_64 binutils
+RUN bash /tmp/illumos-toolchain.sh x86_64 gcc
+
+ENV CARGO_TARGET_X86_64_UNKNOWN_ILLUMOS_LINKER=x86_64-illumos-gcc \
+    # TODO: should actually run these tests
+    CARGO_TARGET_X86_64_UNKNOWN_ILLUMOS_RUNNER=echo \
+    CC=x86_64-illumos-gcc \
+    AR=x86_64-illumos-ar
\ No newline at end of file
diff --git a/ci/illumos-toolchain.sh b/ci/illumos-toolchain.sh
new file mode 100644
index 0000000..0b2c09b
--- /dev/null
+++ b/ci/illumos-toolchain.sh
@@ -0,0 +1,177 @@
+#!/bin/bash
+
+set -o errexit
+set -o pipefail
+set -o xtrace
+
+ARCH="$1"
+PHASE="$2"
+
+JOBS="$(getconf _NPROCESSORS_ONLN)"
+
+case "$ARCH" in
+x86_64)
+        SYSROOT_MACH='i386'
+        ;;
+*)
+        printf 'ERROR: unknown architecture: %s\n' "$ARCH"
+        exit 1
+esac
+
+BUILD_TARGET="$ARCH-pc-solaris2.10"
+
+#
+# The illumos and the Solaris build both use the same GCC-level host triple,
+# though different versions of GCC are used and with different configure
+# options.  To ensure as little accidental cross-pollination as possible, we
+# build the illumos toolchain in a specific directory tree and just symlink the
+# expected tools into /usr/local/bin at the end.  We omit /usr/local/bin from
+# PATH here for similar reasons.
+#
+PREFIX="/opt/illumos/$ARCH"
+export PATH="$PREFIX/bin:/usr/bin:/bin:/usr/sbin:/sbin"
+
+#
+# NOTE: The compiler version selected here is more specific than might appear.
+# GCC 7.X releases do not appear to cross-compile correctly for Solaris
+# targets, at least insofar as they refuse to enable TLS in libstdc++.  When
+# changing the GCC version in future, one must carefully verify that TLS is
+# enabled in all of the static libraries we intend to include in output
+# binaries.
+#
+GCC_VERSION='8.4.0'
+GCC_SUM='e30a6e52d10e1f27ed55104ad233c30bd1e99cfb5ff98ab022dc941edd1b2dd4'
+GCC_BASE="gcc-$GCC_VERSION"
+GCC_TAR="gcc-$GCC_VERSION.tar.xz"
+GCC_URL="https://ftp.gnu.org/gnu/gcc/$GCC_BASE/$GCC_TAR"
+
+SYSROOT_VER='20181213-de6af22ae73b-v1'
+SYSROOT_SUM='ee792d956dfa6967453cebe9286a149143290d296a8ce4b8a91d36bea89f8112'
+SYSROOT_TAR="illumos-sysroot-$SYSROOT_MACH-$SYSROOT_VER.tar.gz"
+SYSROOT_URL='https://github.com/illumos/sysroot/releases/download/'
+SYSROOT_URL+="$SYSROOT_VER/$SYSROOT_TAR"
+SYSROOT_DIR="$PREFIX/sysroot"
+
+BINUTILS_VERSION='2.40'
+BINUTILS_SUM='f8298eb153a4b37d112e945aa5cb2850040bcf26a3ea65b5a715c83afe05e48a'
+BINUTILS_BASE="binutils-$BINUTILS_VERSION"
+BINUTILS_TAR="$BINUTILS_BASE.tar.bz2"
+BINUTILS_URL="https://ftp.gnu.org/gnu/binutils/$BINUTILS_TAR"
+
+
+download_file() {
+        local file="$1"
+        local url="$2"
+        local sum="$3"
+
+        while :; do
+                if [[ -f "$file" ]]; then
+                        if ! h="$(sha256sum "$file" | awk '{ print $1 }')"; then
+                                printf 'ERROR: reading hash\n' >&2
+                                exit 1
+                        fi
+
+                        if [[ "$h" == "$sum" ]]; then
+                                return 0
+                        fi
+
+                        printf 'WARNING: hash mismatch: %s != expected %s\n' \
+                            "$h" "$sum" >&2
+                        rm -f "$file"
+                fi
+
+                printf 'Downloading: %s\n' "$url"
+                if ! curl -f -L -o "$file" "$url"; then
+                        rm -f "$file"
+                        sleep 1
+                fi
+        done
+}
+
+
+case "$PHASE" in
+sysroot)
+        download_file "/tmp/$SYSROOT_TAR" "$SYSROOT_URL" "$SYSROOT_SUM"
+        mkdir -p "$SYSROOT_DIR"
+        cd "$SYSROOT_DIR"
+        tar -xzf "/tmp/$SYSROOT_TAR"
+        rm -f "/tmp/$SYSROOT_TAR"
+        ;;
+
+binutils)
+        download_file "/tmp/$BINUTILS_TAR" "$BINUTILS_URL" "$BINUTILS_SUM"
+        mkdir -p /ws/src/binutils
+        cd /ws/src/binutils
+        tar -xjf "/tmp/$BINUTILS_TAR"
+        rm -f "/tmp/$BINUTILS_TAR"
+
+        mkdir -p /ws/build/binutils
+        cd /ws/build/binutils
+        "/ws/src/binutils/$BINUTILS_BASE/configure" \
+            --prefix="$PREFIX" \
+            --target="$BUILD_TARGET" \
+            --program-prefix="$ARCH-illumos-" \
+            --with-sysroot="$SYSROOT_DIR"
+
+        make -j "$JOBS"
+
+        mkdir -p "$PREFIX"
+        make install
+
+        cd /
+        rm -rf /ws/src/binutils /ws/build/binutils
+        ;;
+
+gcc)
+        download_file "/tmp/$GCC_TAR" "$GCC_URL" "$GCC_SUM"
+        mkdir -p /ws/src/gcc
+        cd /ws/src/gcc
+        tar -xJf "/tmp/$GCC_TAR"
+        rm -f "/tmp/$GCC_TAR"
+
+        mkdir -p /ws/build/gcc
+        cd /ws/build/gcc
+        export CFLAGS='-fPIC'
+        export CXXFLAGS='-fPIC'
+        export CXXFLAGS_FOR_TARGET='-fPIC'
+        export CFLAGS_FOR_TARGET='-fPIC'
+        "/ws/src/gcc/$GCC_BASE/configure" \
+            --prefix="$PREFIX" \
+            --target="$BUILD_TARGET" \
+            --program-prefix="$ARCH-illumos-" \
+            --with-sysroot="$SYSROOT_DIR" \
+            --with-gnu-as \
+            --with-gnu-ld \
+            --disable-nls \
+            --disable-libgomp \
+            --disable-libquadmath \
+            --disable-libssp \
+            --disable-libvtv \
+            --disable-libcilkrts \
+            --disable-libada \
+            --disable-libsanitizer \
+            --disable-libquadmath-support \
+            --disable-shared \
+            --enable-tls
+
+        make -j "$JOBS"
+
+        mkdir -p "$PREFIX"
+        make install
+
+        #
+        # Link toolchain commands into /usr/local/bin so that cmake and others
+        # can find them:
+        #
+        (cd "$PREFIX/bin" && ls -U) | grep "^$ARCH-illumos-" |
+            xargs -t -I% ln -s "$PREFIX/bin/%" '/usr/local/bin/'
+
+        cd /
+        rm -rf /ws/src/gcc /ws/build/gcc
+        ;;
+
+*)
+        printf 'ERROR: unknown phase "%s"\n' "$PHASE" >&2
+        exit 100
+        ;;
+esac
diff --git a/crates/as-if-std/Cargo.toml b/crates/as-if-std/Cargo.toml
index 488fffd..14d60f6 100644
--- a/crates/as-if-std/Cargo.toml
+++ b/crates/as-if-std/Cargo.toml
@@ -19,8 +19,6 @@ libc = { version = "0.2.146", default-features = false }
 [target.'cfg(not(all(windows, target_env = "msvc", not(target_vendor = "uwp"))))'.dependencies]
 miniz_oxide = { version = "0.7.0", optional = true, default-features = false }
 addr2line = { version = "0.22.0", optional = true, default-features = false }
-
-[target.'cfg(not(any(all(windows, target_env = "msvc", not(target_vendor = "uwp")), target_os = "illumos")))'.dependencies]
 zstd = { version = "= 0.13.0", optional = true, default-features = false }
 
 [target.'cfg(not(all(windows, target_env = "msvc", not(target_vendor = "uwp"))))'.dependencies.object]
diff --git a/src/symbolize/gimli/elf.rs b/src/symbolize/gimli/elf.rs
index 20f2d3f..8c64a90 100644
--- a/src/symbolize/gimli/elf.rs
+++ b/src/symbolize/gimli/elf.rs
@@ -7,9 +7,9 @@ use super::{gimli, Context, Endian, EndianSlice, Mapping, Stash, Vec};
 use alloc::sync::Arc;
 use core::convert::{TryFrom, TryInto};
 use core::str;
-#[cfg(not(target_os = "illumos"))]
-use object::elf::ELFCOMPRESS_ZSTD;
-use object::elf::{ELFCOMPRESS_ZLIB, ELF_NOTE_GNU, NT_GNU_BUILD_ID, SHF_COMPRESSED};
+use object::elf::{
+    ELFCOMPRESS_ZLIB, ELFCOMPRESS_ZSTD, ELF_NOTE_GNU, NT_GNU_BUILD_ID, SHF_COMPRESSED,
+};
 use object::read::elf::{CompressionHeader, FileHeader, SectionHeader, SectionTable, Sym};
 use object::read::StringTable;
 use object::{BigEndian, Bytes, NativeEndian};
@@ -188,7 +188,6 @@ impl<'a> Object<'a> {
                     decompress_zlib(data.0, buf)?;
                     return Some(buf);
                 }
-                #[cfg(not(target_os = "illumos"))]
                 ELFCOMPRESS_ZSTD => {
                     let size = usize::try_from(header.ch_size(self.endian)).ok()?;
                     let buf = stash.allocate(size);
@@ -315,7 +314,6 @@ fn decompress_zlib(input: &[u8], output: &mut [u8]) -> Option<()> {
     }
 }
 
-#[cfg(not(target_os = "illumos"))]
 fn decompress_zstd(input: &[u8], output: &mut [u8]) -> Option<()> {
     zstd::stream::copy_decode(input, output).ok()
 }
-- 
2.25.1

But that does increase overall ci time having to build gcc & friends. A pure rust solution would simplify things.

The existing dependency cfgs are a mess. Rework them based on a few principles.

1. Windows targets use PDBs if compiled with MSVC, or DWARF if compiled with the GNU toolchain. Gate addr2line and object, which is only used with addr2line, on not(msvc).
2. Windows targets never use ELF. Gate miniz_oxide and zstd, which are used for ELF compression, on not(windows).
3. libc is used by the addr2line code, Fuschia specific code (which implies DWARF), and libunwind specific code (which implies DWARF), so gate it on not(msvc) as well.
4. The Windows UWP target only allows a reduced set of APIs. That set of APIs excludes some used by addr2line, so additionally gate all the DWARF related crates on not(uwp).
@khuey
Copy link
Contributor Author

khuey commented May 25, 2024

There's an alternative PR using a Rust reimplementation of zstd (the ruzstd crate) in #626. Pick your poison 😄

@workingjubilee
Copy link
Member

I have been very vocal about not significantly raising the maintenance burden of this crate, so it would be out-of-character for me to accept this. I will review #626.

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

Successfully merging this pull request may close these issues.

7 participants