This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [PATCH 2/3] Simplify and inline get_uleb128 and get_sleb128
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Tue, 22 Apr 2014 16:55:41 +0200
- Subject: Re: [PATCH 2/3] Simplify and inline get_uleb128 and get_sleb128
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
>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