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

[Alan Modra <amodra@gmail.com>]

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