From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 452493858D1E for ; Mon, 19 Dec 2022 14:21:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 452493858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.cz Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 73DED37D6E; Mon, 19 Dec 2022 14:21:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1671459679; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=XWonwWcUe636gEnlC0Dk5S3Ye7Xa4Wl6JipesNdAlEY=; b=QVPAqXu5suuwHUjDnq7PfUQUBZWRmSihOCYXgHVxmf6Z96NXK7uTJHdhg09An+KK/nOSgM 8UnwRXqHg97sYkBj1USRylK0rubP5ETpi8lO2G7vAwkPLQy1SBrzrRnsZQJ+zhH4mBrwsx YE1qhK1tsmMjlzocTb1cxPCpADsvXhw= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1671459679; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=XWonwWcUe636gEnlC0Dk5S3Ye7Xa4Wl6JipesNdAlEY=; b=1huG3aubtxojUyozpLRIPLA6w/r+/KWDcqqzYOE4SB0kZi43faO/uzYOSx0FUS3TJ4S3xu flXkmD6PcbIVVUBQ== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 62CA013910; Mon, 19 Dec 2022 14:21:19 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id e2YzF19zoGM/NgAAMHmgww (envelope-from ); Mon, 19 Dec 2022 14:21:19 +0000 Message-ID: <53071eba-0cbc-fb7e-c78c-dfc52cc2843f@suse.cz> Date: Mon, 19 Dec 2022 15:21:19 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 Subject: Re: [PATCHv2] support ZSTD compression algorithm Content-Language: en-US To: Mark Wielaard Cc: elfutils-devel@sourceware.org References: <24d7165b-b8ac-ea5d-a046-aec2203696a9@suse.cz> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,TXREP 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: Hi. >> + if (use_zstd) >> +#ifdef USE_ZSTD >> + return __libelf_compress_zstd (scn, hsize, ei_data, orig_size, >> + orig_addralign, new_size, force, >> + data, next_data, out_buf, out_size, >> + block); >> +#else >> + return NULL; > > Should this also call __libelf_seterrno (ELF_E_UNKNOWN_COMPRESSION_TYPE) ? Yes, will fix that. > >> +#endif >> + else >> + return __libelf_compress_zlib (scn, hsize, ei_data, orig_size, >> + orig_addralign, new_size, force, >> + data, next_data, out_buf, out_size, >> + block); >> +} >> + >> +void * >> +internal_function >> +__libelf_decompress_zlib (void *buf_in, size_t size_in, size_t size_out) >> { >> /* Catch highly unlikely compression ratios so we don't allocate >> some giant amount of memory for nothing. The max compression >> @@ -218,7 +360,7 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t size_out) >> return NULL; >> } >> - /* Malloc might return NULL when requestion zero size. This is highly >> + /* Malloc might return NULL when requesting zero size. This is highly >> unlikely, it would only happen when the compression was forced. >> But we do need a non-NULL buffer to return and set as result. >> Just make sure to always allocate at least 1 byte. */ > > OK, typo fix. > >> @@ -260,6 +402,51 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t size_out) >> return buf_out; >> } >> +#ifdef USE_ZSTD >> +void * >> +internal_function >> +__libelf_decompress_zstd (void *buf_in, size_t size_in, size_t size_out) >> +{ >> + /* Malloc might return NULL when requesting zero size. This is highly >> + unlikely, it would only happen when the compression was forced. >> + But we do need a non-NULL buffer to return and set as result. >> + Just make sure to always allocate at least 1 byte. */ >> + void *buf_out = malloc (size_out ?: 1); >> + if (unlikely (buf_out == NULL)) >> + { >> + __libelf_seterrno (ELF_E_NOMEM); >> + return NULL; >> + } >> + >> + size_t ret = ZSTD_decompress (buf_out, size_out, buf_in, size_in); >> + if (ZSTD_isError (ret)) >> + { >> + free (buf_out); >> + __libelf_seterrno (ELF_E_DECOMPRESS_ERROR); >> + return NULL; >> + } >> + else >> + return buf_out; >> +} >> +#endif > > OK. > >> +void * >> +internal_function >> +__libelf_decompress (int chtype, void *buf_in, size_t size_in, size_t size_out) >> +{ >> + if (chtype == ELFCOMPRESS_ZLIB) >> + return __libelf_decompress_zlib (buf_in, size_in, size_out); >> + else >> + { >> +#ifdef USE_ZSTD >> + return __libelf_decompress_zstd (buf_in, size_in, size_out); >> +#else >> + __libelf_seterrno (ELF_E_DECOMPRESS_ERROR); > > Maybe ELF_E_UNKNOWN_COMPRESSION_TYPE ? Yes, will fix that. > >> + return NULL; >> +#endif >> + } >> +} >> + >> void * >> internal_function >> __libelf_decompress_elf (Elf_Scn *scn, size_t *size_out, size_t *addralign) >> @@ -268,7 +455,7 @@ __libelf_decompress_elf (Elf_Scn *scn, size_t *size_out, size_t *addralign) >> if (gelf_getchdr (scn, &chdr) == NULL) >> return NULL; >> - if (chdr.ch_type != ELFCOMPRESS_ZLIB) >> + if (chdr.ch_type != ELFCOMPRESS_ZLIB && chdr.ch_type != ELFCOMPRESS_ZSTD) >> { >> __libelf_seterrno (ELF_E_UNKNOWN_COMPRESSION_TYPE); >> return NULL; > > Should the chdr.ch_type != ELFCOMPRESS_ZSTD be guarded by #ifdef USE_ZSTD ? Yes, will fix that. >> + else >> + error (0, 0, "Couldn't get chdr for section %zd", ndx); > > Shouldn't this error be fatal? What do you use for fatal errors? > >> @@ -1378,7 +1404,7 @@ main (int argc, char **argv) >> N_("Place (de)compressed output into FILE"), >> 0 }, >> { "type", 't', "TYPE", 0, >> - N_("What type of compression to apply. TYPE can be 'none' (decompress), 'zlib' (ELF ZLIB compression, the default, 'zlib-gabi' is an alias) or 'zlib-gnu' (.zdebug GNU style compression, 'gnu' is an alias)"), >> + N_("What type of compression to apply. TYPE can be 'none' (decompress), 'zlib' (ELF ZLIB compression, the default, 'zlib-gabi' is an alias), 'zlib-gnu' (.zdebug GNU style compression, 'gnu' is an alias) or 'zstd'"), >> 0 }, >> { "name", 'n', "SECTION", 0, >> N_("SECTION name to (de)compress, SECTION is an extended wildcard pattern (defaults to '.?(z)debug*')"), > > I would say or 'zstd' (ELF ZSTD compression)" to match the 'zlib; type > description. Works for me. Cheers, Martin > >> diff --git a/src/readelf.c b/src/readelf.c >> index cc3e0229..451f8400 100644 >> --- a/src/readelf.c >> +++ b/src/readelf.c >> @@ -1238,13 +1238,17 @@ get_visibility_type (int value) >> static const char * >> elf_ch_type_name (unsigned int code) >> { >> - if (code == 0) >> - return "NONE"; >> - >> - if (code == ELFCOMPRESS_ZLIB) >> - return "ZLIB"; >> - >> - return "UNKNOWN"; >> + switch (code) >> + { >> + case 0: >> + return "NONE"; >> + case ELFCOMPRESS_ZLIB: >> + return "ZLIB"; >> + case ELFCOMPRESS_ZSTD: >> + return "ZSTD"; >> + default: >> + return "UNKNOWN"; >> + } >> } >> /* Print the section headers. */ > > OK. > >> diff --git a/tests/run-compress-test.sh b/tests/run-compress-test.sh >> index a6a298f5..3f9c990e 100755 >> --- a/tests/run-compress-test.sh >> +++ b/tests/run-compress-test.sh >> @@ -61,6 +61,30 @@ testrun_elfcompress_file() >> echo "uncompress $elfcompressedfile -> $elfuncompressedfile" >> testrun ${abs_top_builddir}/src/elfcompress -v -t none -o ${elfuncompressedfile} ${elfcompressedfile} >> testrun ${abs_top_builddir}/src/elfcmp ${uncompressedfile} ${elfuncompressedfile} >> + >> + outputfile="${infile}.gabi.zstd" >> + tempfiles "$outputfile" >> + echo "zstd compress $elfcompressedfile -> $outputfile" >> + testrun ${abs_top_builddir}/src/elfcompress -v -t zstd -o ${outputfile} ${elfcompressedfile} >> + testrun ${abs_top_builddir}/src/elfcmp ${uncompressedfile} ${outputfile} >> + echo "checking compressed section header" $outputfile >> + testrun ${abs_top_builddir}/src/readelf -Sz ${outputfile} | grep "ELF ZSTD" >/dev/null >> + >> + zstdfile="${infile}.zstd" >> + tempfiles "$zstdfile" >> + echo "zstd compress $uncompressedfile -> $zstdfile" >> + testrun ${abs_top_builddir}/src/elfcompress -v -t zstd -o ${zstdfile} ${elfuncompressedfile} >> + testrun ${abs_top_builddir}/src/elfcmp ${uncompressedfile} ${zstdfile} >> + echo "checking compressed section header" $zstdfile >> + testrun ${abs_top_builddir}/src/readelf -Sz ${zstdfile} | grep "ELF ZSTD" >/dev/null >> + >> + zstdgnufile="${infile}.zstd.gnu" >> + tempfiles "$zstdgnufile" >> + echo "zstd re-compress to GNU ZLIB $zstdfile -> $zstdgnufile" >> + testrun ${abs_top_builddir}/src/elfcompress -v -t zlib-gnu -o ${zstdgnufile} ${zstdfile} >> + testrun ${abs_top_builddir}/src/elfcmp ${uncompressedfile} ${zstdgnufile} >> + echo "checking .zdebug section name" $zstdgnufile >> + testrun ${abs_top_builddir}/src/readelf -S ${zstdgnufile} | grep ".zdebug" >/dev/null >> } >> testrun_elfcompress() > > You might add these to a separate run test file or pass some ZSTD flag > through the test environment to conditionally run these new tests. > > Cheers, > > Mark