Josh Stone <jistone(a)redhat.com> writes:
diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index a2e4b142a107..93396e404d97 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,17 @@
+2013-12-10 Josh Stone <jistone(a)redhat.com>
+
+ * memory-access.h (get_uleb128_rest_return): Removed.
+ (get_sleb128_rest_return): Removed
Period.
+static inline uint64_t
+__libdw_get_uleb128 (const unsigned char **addrp)
+{
+ uint64_t acc = 0;
+ get_uleb128_step (acc, *addrp, 0);
+ for (unsigned int i = 1; i < len_leb128(acc); ++i)
+ get_uleb128_step (acc, *addrp, i);
Is there a reason not to use for (i = 0; ...) instead of the pre-step
followed by a for (i = 1; ...)? The only difference would be for
len_leb128 of 0, where your code makes one step anyway, but that can't
happen. Then you could just expand get_uleb128_step inline, and get rid
of it altogether.
Also, the len_leb128 call needs space before (...).
+ /* Other implementations set VALUE to UINT_MAX in this
+ case. So we better do this as well. */
+ return UINT64_MAX;
+}
+
+#define get_uleb128(var, addr) \
do { \
+ (var) = __libdw_get_uleb128(&(addr)); \
+ (void)(var); \
} while (0)
Space before (...). Space after type cast operator.
Actually, could this actually be defined like this?
#define get_uleb128(var, addr) ((var) = __libdw_get_uleb128 (&(addr)))
Then you wouldn't need the cast as the only set-but-not-used warnings
would come from sites that actually set but not use, and those could be
changed to simply call __libdw_get_uleb128. But that's probably a
logically separate change.
+#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!
+ return (var); \
} \
+ (var) |= (typeof (var)) (__b & 0x7f) << ((nth) * 7); \
} while (0)
static inline int64_t
+__libdw_get_sleb128 (const unsigned char **addrp)
{
+ int64_t acc = 0;
+ for (unsigned int i = 0; i < len_leb128(acc); ++i)
Space before (...).
+ get_sleb128_step (acc, *addrp, i);
+ /* Other implementations set VALUE to INT_MAX in this
+ case. So we better do this as well. */
+ return INT64_MAX;
}
+
+#define get_sleb128(var, addr) \
+ do { \
+ (var) = __libdw_get_sleb128(&(addr)); \
+ (void)(var); \
+ } while (0)
Same comments as for get_uleb128.
This all looks fine to me. I find the new code much clearer than the
original, which seemed to wind every which way and was hard to follow.
Thanks,
PM