From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x52f.google.com (mail-pg1-x52f.google.com [IPv6:2607:f8b0:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id 10E903858404 for ; Fri, 14 Oct 2022 03:35:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 10E903858404 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=google.com Received: by mail-pg1-x52f.google.com with SMTP id q9so3237142pgq.8 for ; Thu, 13 Oct 2022 20:35:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=ZISp1+VWLIxZYpN3kaIuKRIyEPFB8ruc2ka3Ci0enUA=; b=OmDpkO+rny/iTVCqZsy79N9ufYcvH+ubIdo2oqjSjtm8ACfQ+ZkC7YAT4KO1fhTWLV ZoT9GGMep0LFtcqY1EFU8pMdIx++7HOgLziUlzH9KQi8ZKOTuWv3JdKg1olHGz+4uO8G XTrgZIA+RI7bQDE30FFNJjLEDebF4sQdZUKR2vsaRVwy9sul8lgWH9J4oWBA1mY4P94g AEVIhu2IQyQVH49Z/Ded2Ikf9KWEzCTbsgg5rIQtPw28jTDP0KE8wXMvdFnJYeRohG2q L3uZxUBU3IssbLAzVi5+OGjz0+YwSZsZwJfkG2/sjQrRQU4M3HJHODbLWEeO/LWiVTPA oGoA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ZISp1+VWLIxZYpN3kaIuKRIyEPFB8ruc2ka3Ci0enUA=; b=rcsSnkUADA4RfbKBYamP4sp+Tak29CkYsCHW2AsOnTWD7/ZGzdP+K1RZGcq01JJlPr A9YUngDECVahoZ2PGZakf45c7es9fjgm7f1MdFSy63qbS/0zlsr9F2o+XDp7/GMaMJPq XiMvKbGboM3McrESCZn9Am6c5cbSamskMD0lndmTh/k5CF3DamENvoryst9iers9YS/W QrllV25rWYgHo13LfUw26/Uda3UWwyxZWDmiYsYqFMOadxXJ+W2wEIPT68jnUBdjPhyN waxvAMYcv7pmsYEiv/Fgs1/iSg+2GikDNvJ/DQsYZU3Siv1mYugDQdtArIoEIhVH3W+W nUdA== X-Gm-Message-State: ACrzQf2Ez2NpGNXEjnYcGrZCAcDGMSoezqdlhdtveqjsS5bGxAdwBIDh kB+I2OhGvpFnGX1uRwgWCcySACHww0TtYg== X-Google-Smtp-Source: AMsMyM4mj2jV2Yj4x9oWIgIklPILFPAoRk1C5kVRFsm6hsdvLvw44VMdGPa/dZBwY9jPO4q/Sstj6A== X-Received: by 2002:aa7:88d6:0:b0:563:9fe9:5da8 with SMTP id k22-20020aa788d6000000b005639fe95da8mr2884294pff.74.1665718515891; Thu, 13 Oct 2022 20:35:15 -0700 (PDT) Received: from google.com ([2620:15c:2ce:200:9ffc:cb04:293c:acec]) by smtp.gmail.com with ESMTPSA id j9-20020a17090a840900b0020d51aefb82sm486257pjn.19.2022.10.13.20.35.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 13 Oct 2022 20:35:14 -0700 (PDT) Date: Thu, 13 Oct 2022 20:35:11 -0700 From: Fangrui Song To: Martin =?utf-8?B?TGnFoWth?= Cc: binutils@sourceware.org, Alan Modra Subject: Re: [PATCH] readelf: support zstd compressed debug sections [PR 29640] Message-ID: <20221014033511.vqanebcqikmi5k37@google.com> References: <20221001062057.681440-1-maskray@google.com> <45fca661-a0e8-f0f8-78d6-8d90783de6d7@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <45fca661-a0e8-f0f8-78d6-8d90783de6d7@suse.cz> X-Spam-Status: No, score=-26.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,ENV_AND_HDR_SPF_MATCH,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,USER_IN_DEF_DKIM_WL,USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2022-10-13, Martin Liška wrote: >On 10/1/22 08:20, Fangrui Song wrote: >> --- >> binutils/Makefile.am | 6 +-- >> binutils/Makefile.in | 6 +-- >> binutils/readelf.c | 112 +++++++++++++++++++++++++++---------------- >> 3 files changed, 78 insertions(+), 46 deletions(-) >> >> diff --git a/binutils/Makefile.am b/binutils/Makefile.am >> index 751fbacce12..b249af721ae 100644 >> --- a/binutils/Makefile.am >> +++ b/binutils/Makefile.am >> @@ -54,8 +54,8 @@ DEBUGINFOD_LIBS = @DEBUGINFOD_LIBS@ >> WARN_CFLAGS = @WARN_CFLAGS@ >> WARN_CFLAGS_FOR_BUILD = @WARN_CFLAGS_FOR_BUILD@ >> NO_WERROR = @NO_WERROR@ >> -AM_CFLAGS = $(WARN_CFLAGS) $(ZLIBINC) >> -AM_CFLAGS_FOR_BUILD = $(WARN_CFLAGS_FOR_BUILD) $(ZLIBINC) >> +AM_CFLAGS = $(WARN_CFLAGS) $(ZLIBINC) $(ZSTD_CFLAGS) >> +AM_CFLAGS_FOR_BUILD = $(WARN_CFLAGS_FOR_BUILD) $(ZLIBINC) $(ZSTD_CFLAGS) >> LIBICONV = @LIBICONV@ >> >> # these two are almost the same program >> @@ -256,7 +256,7 @@ objcopy_SOURCES = objcopy.c not-strip.c rename.c $(WRITE_DEBUG_SRCS) $(BULIBS) >> strings_SOURCES = strings.c $(BULIBS) >> >> readelf_SOURCES = readelf.c version.c unwind-ia64.c dwarf.c demanguse.c $(ELFLIBS) >> -readelf_LDADD = $(LIBCTF_NOBFD) $(LIBINTL) $(LIBIBERTY) $(ZLIB) $(DEBUGINFOD_LIBS) $(MSGPACK_LIBS) >> +readelf_LDADD = $(LIBCTF_NOBFD) $(LIBINTL) $(LIBIBERTY) $(ZLIB) $(ZSTD_LIBS) $(DEBUGINFOD_LIBS) $(MSGPACK_LIBS) >> >> elfedit_SOURCES = elfedit.c version.c $(ELFLIBS) >> elfedit_LDADD = $(LIBINTL) $(LIBIBERTY) >> diff --git a/binutils/Makefile.in b/binutils/Makefile.in >> index 6de4e239408..7d9039e0f2f 100644 >> --- a/binutils/Makefile.in >> +++ b/binutils/Makefile.in >> @@ -651,8 +651,8 @@ am__skipyacc = >> # case both are empty. >> ZLIB = @zlibdir@ -lz >> ZLIBINC = @zlibinc@ >> -AM_CFLAGS = $(WARN_CFLAGS) $(ZLIBINC) >> -AM_CFLAGS_FOR_BUILD = $(WARN_CFLAGS_FOR_BUILD) $(ZLIBINC) >> +AM_CFLAGS = $(WARN_CFLAGS) $(ZLIBINC) $(ZSTD_CFLAGS) >> +AM_CFLAGS_FOR_BUILD = $(WARN_CFLAGS_FOR_BUILD) $(ZLIBINC) $(ZSTD_CFLAGS) >> >> # these two are almost the same program >> AR_PROG = ar >> @@ -790,7 +790,7 @@ size_SOURCES = size.c $(BULIBS) >> objcopy_SOURCES = objcopy.c not-strip.c rename.c $(WRITE_DEBUG_SRCS) $(BULIBS) >> strings_SOURCES = strings.c $(BULIBS) >> readelf_SOURCES = readelf.c version.c unwind-ia64.c dwarf.c demanguse.c $(ELFLIBS) >> -readelf_LDADD = $(LIBCTF_NOBFD) $(LIBINTL) $(LIBIBERTY) $(ZLIB) $(DEBUGINFOD_LIBS) $(MSGPACK_LIBS) >> +readelf_LDADD = $(LIBCTF_NOBFD) $(LIBINTL) $(LIBIBERTY) $(ZLIB) $(ZSTD_LIBS) $(DEBUGINFOD_LIBS) $(MSGPACK_LIBS) >> elfedit_SOURCES = elfedit.c version.c $(ELFLIBS) >> elfedit_LDADD = $(LIBINTL) $(LIBIBERTY) >> strip_new_SOURCES = objcopy.c is-strip.c rename.c $(WRITE_DEBUG_SRCS) $(BULIBS) >> diff --git a/binutils/readelf.c b/binutils/readelf.c >> index 351571c8abb..04cda213f33 100644 >> --- a/binutils/readelf.c >> +++ b/binutils/readelf.c >> @@ -44,6 +44,9 @@ >> #include >> #include >> #include >> +#ifdef HAVE_ZSTD >> +#include >> +#endif >> #include >> >> #if defined HAVE_MSGPACK >> @@ -15170,48 +15173,56 @@ get_section_contents (Elf_Internal_Shdr * section, Filedata * filedata) >> _("section contents")); >> } >> >> -/* Uncompresses a section that was compressed using zlib, in place. */ >> +/* Uncompresses a section that was compressed using zlib/zstd, in place. */ >> >> static bool >> -uncompress_section_contents (unsigned char **buffer, >> - uint64_t uncompressed_size, >> - uint64_t *size) >> +uncompress_section_contents (bool is_zstd, unsigned char **buffer, >> + uint64_t uncompressed_size, uint64_t *size) >> { >> uint64_t compressed_size = *size; >> unsigned char *compressed_buffer = *buffer; >> - unsigned char *uncompressed_buffer; >> + unsigned char *uncompressed_buffer = xmalloc (uncompressed_size); >> z_stream strm; >> int rc; >> >> - /* It is possible the section consists of several compressed >> - buffers concatenated together, so we uncompress in a loop. */ >> - /* PR 18313: The state field in the z_stream structure is supposed >> - to be invisible to the user (ie us), but some compilers will >> - still complain about it being used without initialisation. So >> - we first zero the entire z_stream structure and then set the fields >> - that we need. */ >> - memset (& strm, 0, sizeof strm); >> - strm.avail_in = compressed_size; >> - strm.next_in = (Bytef *) compressed_buffer; >> - strm.avail_out = uncompressed_size; >> - uncompressed_buffer = (unsigned char *) xmalloc (uncompressed_size); >> + if (is_zstd) >> + { >> +#ifdef HAVE_ZSTD >> + size_t ret = ZSTD_decompress (uncompressed_buffer, uncompressed_size, >> + compressed_buffer, compressed_size); >> + if (ZSTD_isError (ret)) >> + goto fail; >> +#endif >> + } >> + else >> + { >> + /* It is possible the section consists of several compressed >> + buffers concatenated together, so we uncompress in a loop. */ >> + /* PR 18313: The state field in the z_stream structure is supposed >> + to be invisible to the user (ie us), but some compilers will >> + still complain about it being used without initialisation. So >> + we first zero the entire z_stream structure and then set the fields >> + that we need. */ >> + memset (&strm, 0, sizeof strm); >> + strm.avail_in = compressed_size; >> + strm.next_in = (Bytef *)compressed_buffer; >> + strm.avail_out = uncompressed_size; >> >> - rc = inflateInit (& strm); >> - while (strm.avail_in > 0) >> - { >> - if (rc != Z_OK) >> - break; >> - strm.next_out = ((Bytef *) uncompressed_buffer >> - + (uncompressed_size - strm.avail_out)); >> - rc = inflate (&strm, Z_FINISH); >> - if (rc != Z_STREAM_END) >> - break; >> - rc = inflateReset (& strm); >> + rc = inflateInit (&strm); >> + while (strm.avail_in > 0) >> + { >> + if (rc != Z_OK) >> + break; >> + strm.next_out = ((Bytef *)uncompressed_buffer >> + + (uncompressed_size - strm.avail_out)); >> + rc = inflate (&strm, Z_FINISH); >> + if (rc != Z_STREAM_END) >> + break; >> + rc = inflateReset (&strm); >> + } >> + if (inflateEnd (&strm) != Z_OK || rc != Z_OK || strm.avail_out != 0) >> + goto fail; >> } >> - if (inflateEnd (& strm) != Z_OK >> - || rc != Z_OK >> - || strm.avail_out != 0) >> - goto fail; >> >> *buffer = uncompressed_buffer; >> *size = uncompressed_size; >> @@ -15254,6 +15265,7 @@ dump_section_as_strings (Elf_Internal_Shdr * section, Filedata * filedata) >> { >> uint64_t new_size = num_bytes; >> uint64_t uncompressed_size = 0; >> + bool is_zstd = false; >> >> if ((section->sh_flags & SHF_COMPRESSED) != 0) >> { >> @@ -15266,7 +15278,13 @@ dump_section_as_strings (Elf_Internal_Shdr * section, Filedata * filedata) >> by get_compression_header. */ >> goto error_out; >> >> - if (chdr.ch_type != ELFCOMPRESS_ZLIB) >> + if (chdr.ch_type == ELFCOMPRESS_ZLIB) >> + ; >> +#ifdef HAVE_ZSTD >> + else if (chdr.ch_type == ELFCOMPRESS_ZSTD) >> + is_zstd = true; >> +#endif >> + else >> { >> warn (_("section '%s' has unsupported compress type: %d\n"), >> printable_section_name (filedata, section), chdr.ch_type); >> @@ -15295,8 +15313,8 @@ dump_section_as_strings (Elf_Internal_Shdr * section, Filedata * filedata) >> >> if (uncompressed_size) >> { >> - if (uncompress_section_contents (& start, >> - uncompressed_size, & new_size)) >> + if (uncompress_section_contents (is_zstd, &start, uncompressed_size, >> + &new_size)) >> num_bytes = new_size; >> else >> { >> @@ -15470,6 +15488,7 @@ dump_section_as_bytes (Elf_Internal_Shdr *section, >> { >> uint64_t new_size = section_size; >> uint64_t uncompressed_size = 0; >> + bool is_zstd = false; >> >> if ((section->sh_flags & SHF_COMPRESSED) != 0) >> { >> @@ -15482,7 +15501,13 @@ dump_section_as_bytes (Elf_Internal_Shdr *section, >> by get_compression_header. */ >> goto error_out; >> >> - if (chdr.ch_type != ELFCOMPRESS_ZLIB) >> + if (chdr.ch_type == ELFCOMPRESS_ZLIB) >> + ; >> +#ifdef HAVE_ZSTD >> + else if (chdr.ch_type == ELFCOMPRESS_ZSTD) >> + is_zstd = true; >> +#endif >> + else >> { >> warn (_("section '%s' has unsupported compress type: %d\n"), >> printable_section_name (filedata, section), chdr.ch_type); >> @@ -15511,8 +15536,8 @@ dump_section_as_bytes (Elf_Internal_Shdr *section, >> >> if (uncompressed_size) >> { >> - if (uncompress_section_contents (& start, uncompressed_size, >> - & new_size)) >> + if (uncompress_section_contents (is_zstd, &start, uncompressed_size, >> + &new_size)) >> { >> section_size = new_size; >> } >> @@ -15848,6 +15873,7 @@ load_specific_debug_section (enum dwarf_section_display_enum debug, >> unsigned char *start = section->start; >> uint64_t size = sec->sh_size; >> uint64_t uncompressed_size = 0; >> + bool is_zstd = false; >> >> if ((sec->sh_flags & SHF_COMPRESSED) != 0) >> { >> @@ -15869,7 +15895,13 @@ load_specific_debug_section (enum dwarf_section_display_enum debug, >> by get_compression_header. */ >> return false; >> >> - if (chdr.ch_type != ELFCOMPRESS_ZLIB) >> + if (chdr.ch_type == ELFCOMPRESS_ZLIB) >> + ; >> +#ifdef HAVE_ZSTD >> + else if (chdr.ch_type == ELFCOMPRESS_ZSTD) >> + is_zstd = true; >> +#endif >> + else >> { >> warn (_("section '%s' has unsupported compress type: %d\n"), >> section->name, chdr.ch_type); >> @@ -15898,7 +15930,7 @@ load_specific_debug_section (enum dwarf_section_display_enum debug, >> >> if (uncompressed_size) >> { >> - if (uncompress_section_contents (&start, uncompressed_size, >> + if (uncompress_section_contents (is_zstd, &start, uncompressed_size, >> &size)) >> { >> /* Free the compressed buffer, update the section buffer > >Hi. > >I noticed the following issue (dunno if caused by objcopy or the patches readelf) with this patch: > >$ echo 'int main() {return 0;}' | gcc -O2 -g -c -o a.o -x c - >$ /home/marxin/Programming/binutils/objdir/ld/../binutils/objcopy -O elf32-x86-64 a.o a-x32.o --compress-debug-sections=zstd >$ /home/marxin/Programming/binutils/objdir/binutils/readelf -wi a-x32.o >/dev/null >readelf: Error: Unable to decompress section .debug_info >readelf: Error: No comp units in .debug_info section ? > >while zlib is fine: > >$ /home/marxin/Programming/binutils/objdir/ld/../binutils/objcopy -O elf32-x86-64 a.o a-x32.o --compress-debug-sections=zlib >$ /home/marxin/Programming/binutils/objdir/binutils/readelf -wi a-x32.o >/dev/null > >Can you please take a look? objcopy -O elf32-x86-64 a.o a-x32.o --compress-debug-sections=zstd uses ch_type=1 (ELFCOMPRESS_ZLIB), but I don't know where goes wrong. Converting EI_CLASS looks very hacky to me.