Re: [PATCH v3] binutils, gdb: support zstd compressed debug sections

Alan Modra via Gdb-patches
2022-09-26T13:30:50Z

On Mon, Sep 26, 2022 at 12:20:27AM -0700, Fangrui Song wrote:
> On 2022-09-26, Alan Modra wrote:
> > Better to avoid the need..
> >
> > binutils/
> > * configure.ac (msgpack): Use "AS_IF" rather than "if".
> > * configure: Regenerate.
> > ld/
> > * configure.ac (jansson): Use "AS_IF" rather than "if".
> > * configure: Regenerate.
> >
>
> Thanks. I've changed the zstd patch to use AS_IF, too.

It probably wasn't necessary inside AC_DEFUN, but won't hurt either.

> BTW: I think the `!= xno` trick for ancient shells is not needed. `!=
> no` is used many times in binutils-gdb.

Quite possibly true nowadays. I'm not going to be the one that
removes them all. :-)

Your patch looks OK to me, apart from a few formatting nits. Here are
two examples:

> @@ -150,9 +163,10 @@ bfd_compress_section_contents (bfd *abfd, sec_ptr sec,
> sec->size = orig_uncompressed_size;
> if (decompress)
> {
> - if (!decompress_contents (uncompressed_buffer
> - + orig_compression_header_size,
> - zlib_size, buffer, buffer_size))
> + if (!decompress_contents (
> + sec->compress_status == DECOMPRESS_SECTION_ZSTD,
> + uncompressed_buffer + orig_compression_header_size,
> + zlib_size, buffer, buffer_size))
> {
> bfd_set_error (bfd_error_bad_value);
> bfd_release (abfd, buffer);

One of the formatting guidelines in
https://www.gnu.org/prep/standards/html_node/Formatting.html#Formatting
talks about emacs indenting of code. The above looks reasonable with
your indenting by hand, but auto-indent will turn it into

if (!decompress_contents (
sec->compress_status == DECOMPRESS_SECTION_ZSTD,
uncompressed_buffer + orig_compression_header_size,
zlib_size, buffer, buffer_size))

lining up function args with the open parenthesis. Inserting a couple
of temporary vars is the nicest solution to keeping line length
manageable.

bool is_z = sec->compress_status == DECOMPRESS_SECTION_ZSTD;
bfd_byte *p = uncompressed_buffer + orig_compression_header_size;
if (!decompress_contents (is_z, p, zlib_size, buffer, buffer_size))

> @@ -569,7 +604,8 @@ bfd_init_section_decompress_status (bfd *abfd, sec_ptr sec)
> sec->compressed_size = sec->size;
> sec->size = uncompressed_size;
> bfd_set_section_alignment (sec, uncompressed_alignment_power);
> - sec->compress_status = DECOMPRESS_SECTION_SIZED;
> + sec->compress_status = ch_type == ELFCOMPRESS_ZSTD ? DECOMPRESS_SECTION_ZSTD
> + : DECOMPRESS_SECTION_ZLIB;
>
> return true;
> }

The same emacs auto-indent will ruin the above. Write

sec->compress_status = (ch_type == ELFCOMPRESS_ZSTD
? DECOMPRESS_SECTION_ZSTD : DECOMPRESS_SECTION_ZLIB);

with unnecessary parentheses to guide indentation.
OK to commit with those things fixed.

--
Alan Modra
Australia Development Lab, IBM