On Thu, 2013-12-12 at 11:06 -0800, Josh Stone wrote: > On 12/12/2013 04:13 AM, Petr Machata wrote: > > Josh Stone <[email protected]> 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 <[email protected]> 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 <[email protected]> --- 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 <[email protected]> + + * memory-access.h (get_sleb128_step): Remove undefined behavior + of left shifting a signed value. Add explicit sign extension. + 2014-04-13 Mark Wielaard <[email protected]> * 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
