Re: [PATCH 2/3] Simplify and inline get_uleb128 and get_sleb128

[Petr Machata <pmachata@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1777 bytes --]

Mark Wielaard <mjw@redhat.com> writes:

> Sadly the neat trick triggers undefined behavior since we are trying to
> left shift a negative value. Even though it appears to work currently I
> am slightly afraid a compiler optimization might take advantage of this
> in the future (since it is undefined behavior it could just assume
> negative values won't occur) especially since this code is inlined in a
> lot of places, causing hard to diagnose errors.

Ouch.  Yeah, I agree, it is essentially a matter of time before this
whole thing is optimized away or something.

> diff --git a/libdw/memory-access.h b/libdw/memory-access.h
> index d0ee63c..c6e4bdc 100644
> --- a/libdw/memory-access.h
> +++ b/libdw/memory-access.h
> @@ -70,8 +70,9 @@ __libdw_get_uleb128 (const unsigned char **addrp)
>      unsigned char __b = *(addr)++;					      \
>      if (likely ((__b & 0x80) == 0))					      \
>        {									      \
> -	struct { signed int i:7; } __s = { .i = __b };			      \
> -	(var) |= (typeof (var)) __s.i << ((nth) * 7);			      \
> +	(var) |= (typeof (var)) (__b & 0x7f) << ((nth) * 7);		      \
> +	if ((((nth) + 1) < 8 * sizeof (var)) && (__b & 0x40))		      \
> +	  (var) |= -(((uint64_t) 1) << (((nth) + 1) * 7));		      \
>  	return (var);							      \
>        }									      \
>      (var) |= (typeof (var)) (__b & 0x7f) << ((nth) * 7);		      \

Wouldn't something like this get us off the hook as well?

-	(var) |= (typeof (var)) __s.i << ((nth) * 7);			      \
+	(var) |= (typeof (var))						      \
+	  (((uint64_t) (typeof (var)) __s.i) << ((nth) * 7));		      \

We are really only using the bitfield trick to avoid having to
sign-extend by hand, but we can shift unsigned without losing anything.

Thanks,
PM