Re: [PATCH] Add write_sleb129 and size_of_sleb128 for writing DW_FORM_implicit_const

[Jakub Jelinek <jakub@redhat.com>]

On Mon, Sep 28, 2020 at 09:36:47AM +0200, Mark Wielaard wrote:
> We were writing out the DW_FORM_implicit_const value as uleb128,
> but it should be written out as sleb128 (as we already read it).
> I'll merge this into my experimental DW_FORM_implicit_const patch.

Besides s/129/128 in the title as mentioned by Florian:

> 	* dwz.c (write_sleb128): New macro.
> 	(size_of_sleb128): New function.
> ---
>  dwz.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/dwz.c b/dwz.c
> index d730f98..e772954 100644
> --- a/dwz.c
> +++ b/dwz.c
> @@ -399,6 +399,25 @@ typedef struct
>      }					\
>    while (0)
>  
> +#define write_sleb128(ptr, val)					\
> +  do								\
> +    {								\
> +      int64_t valv = (val);					\
> +      bool more;						\
> +      do							\
> +	{							\
> +	  unsigned char c = (valv & 0x7f);			\

The parens above are unnecessary.

> +	  valv >>= 7;						\
> +	  more = !((((valv == 0) && ((c & 0x40) == 0))		\
> +		    || ((valv == -1) && ((c & 0x40) != 0))));	\

And this has even more unnecessary parens.
Either
	  more = ((valv != 0 || (c & 0x40) != 0)		\
		  && (valv != -1 || (c & 0x40) == 0));		\
or perhaps
	  more = (valv + (uint64_t) 1 > 1			\
		  || ((c ^ valv) & 0x40) != 0);			\
?

> +/* Return number of bytes needed to encode VAL using
> +   sleb128.  */
> +static unsigned int
> +size_of_sleb128 (int64_t val)
> +{
> +  unsigned int size = 0;
> +  unsigned char b;
> +  do
> +    {
> +      b = (val & 0x7f);

Ditto.

> +      val >>= 7;
> +      size++;
> +    }
> +  while (!(((val == 0) && ((b & 0x40) == 0))
> +	   || ((val == -1) && ((b & 0x40) != 0))));

Ditto.

Otherwise LGTM.

	Jakub