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

[Mark Wielaard <mjw@redhat.com>]

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

On Thu, 2013-12-12 at 11:06 -0800, Josh Stone wrote:
> On 12/12/2013 04:13 AM, Petr Machata wrote:
> > Josh Stone <jistone@redhat.com> writes:
> >> +#define get_sleb128_step(var, addr, nth)			  \
> >>    do {							  \
> >> +    unsigned char __b = *(addr)++;				  \
> >> +    if (likely ((__b & 0x80) == 0))				  \
> >>        {							  \
> >> +	   struct { signed int i:7; } __s = { .i = __b };	  \
> >> +	   (var) |= (typeof (var)) __s.i << ((nth) * 7);	  \
> > 
> > Oh, the bitfield trick is clever!
> 
> I should give credit, I found that trick here:
> http://graphics.stanford.edu/~seander/bithacks.html#FixedSignExtend
> 
> The former code was trying to sign-extend after the value was shifted
> and combined, which as a variable width is harder.  I really like that
> this way the compiler is fully aware that this is a sign extension,
> rather than being a side effect of ORing bits or left-right shifts.

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.

The attached patch is very much not clever, but does what is intended in
a well-defined way (it is basically what the DWARF spec gives as pseudo
code). Does anybody see a better way?

Thanks,

Mark

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-libdw-get_sleb128_step-Remove-undefined-behavior.patch --]
[-- Type: text/x-patch, Size: 1707 bytes --]

>From 0eedb6486806dba9c454edcc249238e096961e09 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Tue, 22 Apr 2014 16:43:11 +0200
Subject: [PATCH] libdw (get_sleb128_step): Remove undefined behavior.

As pointed out by gcc -fsanitize=undefined left shifting a negative value
is undefined. Replace it with an explicit sign extension step.

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdw/ChangeLog       | 5 +++++
 libdw/memory-access.h | 5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 49d70af..e561e00 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,8 @@
+2014-04-22  Mark Wielaard  <mjw@redhat.com>
+
+	* memory-access.h (get_sleb128_step): Remove undefined behavior
+	of left shifting a signed value. Add explicit sign extension.
+
 2014-04-13  Mark Wielaard  <mjw@redhat.com>
 
 	* Makefile.am: Remove !MUDFLAP conditions.
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);		      \
-- 
1.9.0