[PATCH] Add write_sleb129 and size_of_sleb128 for writing DW_FORM_implicit_const

[PATCH] Add write_sleb129 and size_of_sleb128 for writing DW_FORM_implicit_const

[Mark Wielaard]

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.


The DW_FORM_implicit_const value is a signed 64bit sleb128 number.

	* 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);			\
+	  valv >>= 7;						\
+	  more = !((((valv == 0) && ((c & 0x40) == 0))		\
+		    || ((valv == -1) && ((c & 0x40) != 0))));	\
+	  if (more)						\
+	    c |= 0x80;						\
+	  *ptr++ = c;						\
+	}							\
+      while (more);						\
+    }								\
+  while (0)
+
 /* Macro to skip a uleb128 or sleb128 number and update ptr to the end of the
    number.  */
 #define skip_leb128(ptr) \
@@ -8991,6 +9010,24 @@ size_of_uleb128 (uint64_t val)
   return size;
 }
 
+/* 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);
+      val >>= 7;
+      size++;
+    }
+  while (!(((val == 0) && ((b & 0x40) == 0))
+	   || ((val == -1) && ((b & 0x40) != 0))));
+  return size;
+}
+
 /* Hash table mapping original file IDs to new ids.  */
 static htab_t line_htab;
 /* Current new maximum file ID.  */
@@ -11011,7 +11048,7 @@ compute_abbrevs (DSO *dso)
 		      uint64_t value = arr[i]->attr[j].value;
 		      arr[i]->attr[j].value = line_htab_lookup (cu, value);
 		    }
-		  abbrev_size += size_of_uleb128 (arr[i]->attr[j].value);
+		  abbrev_size += size_of_sleb128 (arr[i]->attr[j].value);
 		}
 	    }
 	  abbrev_size += 2;
@@ -11104,7 +11141,7 @@ write_abbrev (void)
 	      write_uleb128 (ptr, arr[i]->attr[j].attr);
 	      write_uleb128 (ptr, arr[i]->attr[j].form);
 	      if (arr[i]->attr[j].form == DW_FORM_implicit_const)
-		write_uleb128 (ptr, arr[i]->attr[j].value);
+		write_sleb128 (ptr, arr[i]->attr[j].value);
 	    }
 	  *ptr++ = 0;
 	  *ptr++ = 0;
-- 
2.18.4

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

[Jakub Jelinek]

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

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

[Mark Wielaard]

Hi Jakub,

On Tue, 2020-09-29 at 10:24 +0200, Jakub Jelinek via Dwz wrote:
> 
> The parens above are unnecessary.
> [...]
> And this has even more unnecessary parens.
> [...]
> Otherwise LGTM.

Thanks simplified those and fixed the commit message.
Haven't pushed this because the DW_FORM_implicit_const support isn't
100% yet. The is the memory issue and I think the hashing/checksum
isn't 100% correct because in DWARF5 mode I still see some odr
failures. I'll get back to that after finishing the DWARF5 .debug_line
support. For now the patches can be found here:
https://code.wildebeest.org/git/user/mjw/dwz/log/?h=dwarf5-forms

Cheers,

Mark