* [PATCH] libdw: dwarf_formsdata should return a signed value
@ 2018-01-14 2:22 Petr Machata
2018-01-14 16:28 ` Mark Wielaard
0 siblings, 1 reply; 5+ messages in thread
From: Petr Machata @ 2018-01-14 2:22 UTC (permalink / raw)
To: elfutils-devel; +Cc: Petr Machata
The function dwarf_formsdata is used for decoding signed values, but
except for the variable-length DW_FORM_sdata, it uses unsigned
primitives to decode the value. This is not a problem for 64-bit values,
but the smaller values come decoded wrong. Fix by changing to signed
primitives for decoding the fixed-length forms.
Add a test case that uses dwarf_aggregate_size to determine an array
size whose lower bound is -1, encoded using DW_FORM_data1, and upper
bound 255 with DW_FORM_data2. When the -1 is decoded wrongly, it comes
back as 255, and the array size is 1. The correct array size should be
257.
Signed-off-by: Petr Machata <pmachata@gmail.com>
---
libdw/ChangeLog | 7 ++++
libdw/dwarf_formsdata.c | 8 ++---
tests/run-aggregate-size.sh | 8 ++++-
tests/testfile-sizes4.o.bz2 | Bin 0 -> 387 bytes
tests/testfile-sizes4.s | 77 ++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 95 insertions(+), 5 deletions(-)
create mode 100644 tests/testfile-sizes4.o.bz2
create mode 100644 tests/testfile-sizes4.s
Regarding testfile-sizes4.s, that's a product of a TCL script written using
dwarf.exp from GDB. I can post it as well (actually would prefer it like
that),but I think you bounced it before already (I found a TCL script foranother
test case from way back in my elfutils directory. I seem to recall you
didn'tlike this approach for whatever reason.)
diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 7cfc7825..479dd42b 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,10 @@
+2018-01-14 Petr Machata <pmachata@gmail.com>
+
+ * dwarf_formsdata.c (dwarf_formsdata):
+ <DW_FORM_data1>: Cast to signed char.
+ <DW_FORM_data2,4,8>: Use read_*sbyte_unaligned instead of
+ read_*ubyte_unaligned.
+
2017-12-26 Mark Wielaard <mark@klomp.org>
* libdwP.h (struct Dwarf_Abbrev): Pack struct. Remove attrcnt,
diff --git a/libdw/dwarf_formsdata.c b/libdw/dwarf_formsdata.c
index e7deaee1..bc2b508d 100644
--- a/libdw/dwarf_formsdata.c
+++ b/libdw/dwarf_formsdata.c
@@ -53,25 +53,25 @@ dwarf_formsdata (Dwarf_Attribute *attr, Dwarf_Sword *return_sval)
__libdw_seterrno (DWARF_E_INVALID_DWARF);
return -1;
}
- *return_sval = *attr->valp;
+ *return_sval = (signed char) *attr->valp;
break;
case DW_FORM_data2:
if (datap + 2 > endp)
goto invalid;
- *return_sval = read_2ubyte_unaligned (attr->cu->dbg, attr->valp);
+ *return_sval = read_2sbyte_unaligned (attr->cu->dbg, attr->valp);
break;
case DW_FORM_data4:
if (datap + 4 > endp)
goto invalid;
- *return_sval = read_4ubyte_unaligned (attr->cu->dbg, attr->valp);
+ *return_sval = read_4sbyte_unaligned (attr->cu->dbg, attr->valp);
break;
case DW_FORM_data8:
if (datap + 8 > endp)
goto invalid;
- *return_sval = read_8ubyte_unaligned (attr->cu->dbg, attr->valp);
+ *return_sval = read_8sbyte_unaligned (attr->cu->dbg, attr->valp);
break;
case DW_FORM_sdata:
diff --git a/tests/run-aggregate-size.sh b/tests/run-aggregate-size.sh
index 6d8aa240..08d57bbf 100755
--- a/tests/run-aggregate-size.sh
+++ b/tests/run-aggregate-size.sh
@@ -63,7 +63,9 @@
#
# gcc -std=c99 -g -c -o testfile-sizes3.o sizes.c
-testfiles testfile-sizes1.o testfile-sizes2.o testfile-sizes3.o
+# The file testfile-size4.o is hand-crafted.
+
+testfiles testfile-sizes1.o testfile-sizes2.o testfile-sizes3.o testfile-sizes4.o
testrun_compare ${abs_builddir}/aggregate_size -e testfile-sizes1.o <<\EOF
c size 1
@@ -104,4 +106,8 @@ f size 4
b size 4
EOF
+testrun_compare ${abs_builddir}/aggregate_size -e testfile-sizes4.o <<\EOF
+v size 257
+EOF
+
exit 0
diff --git a/tests/testfile-sizes4.o.bz2 b/tests/testfile-sizes4.o.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..046e0a23429c166c659349f628c39170bbb4818f
GIT binary patch
literal 387
zcmV-}0et>KT4*^jL0KkKS*U1%zyJYNfAs(V>Th(1qytN(LIL;ppJ2d1006)MKmY&;
zumMI1hJq6+dsOu{DWf1XWM}{Y05m;ErkaNtGHApxG{8W>nqZ6*6HPP#VFZaN$V1Wr
zpk!zPpa99EKn(_(N{z;abVA6Etg=-jpBQVd%dq;e6Q>9sa+xWbp{9draa^JPJRN<z
zxTM=LEHNo0y4X>G7UMVI%lII?kh^@`&;bZ`8l9vj(N&GCdoUne%}m2_-C-sJeC$dc
zRs=NKY&V)*jSAC3*!qNUJ2&v)o8u3XuT?+XU(%w3Ol_&e+S<9PJno*e2s+^yC2Wuv
zYYL>0n@d&qy3D-8C5BTLMaZa%@P*vOUdIVvr$m7B7*vxZ#l?7usFf6{7O7%ox7xzP
zn4E{qj4D#xi1vKslj!Or-mJ>1XagNuBz3(2vJWi57V$TVwSIoYVz!`!_C;<?CRIGg
hmYOX-mb_;`4XN-OZz-5%<Y!O#yOJrwgoQ&y4glQWvNiw!
literal 0
HcmV?d00001
diff --git a/tests/testfile-sizes4.s b/tests/testfile-sizes4.s
new file mode 100644
index 00000000..a2430215
--- /dev/null
+++ b/tests/testfile-sizes4.s
@@ -0,0 +1,77 @@
+ .section .debug_info
+.Lcu1_begin:
+ .4byte .Lcu1_end - .Lcu1_start
+.Lcu1_start:
+ .2byte 4 /* Version */
+ .4byte .Labbrev1_begin /* Abbrevs */
+ .byte 8 /* Pointer size */
+ .uleb128 2 /* Abbrev (DW_TAG_compile_unit) */
+ .uleb128 3 /* Abbrev (DW_TAG_variable) */
+ .ascii "v\0"
+ .4byte .Llabel1 - .Lcu1_begin
+.Llabel1:
+ .uleb128 4 /* Abbrev (DW_TAG_array_type) */
+ .4byte .Llabel2 - .Lcu1_begin
+ .uleb128 5 /* Abbrev (DW_TAG_subrange_type) */
+ .byte -1
+ .2byte 255
+ .byte 0x0 /* Terminate children */
+.Llabel2:
+ .uleb128 6 /* Abbrev (DW_TAG_base_type) */
+ .byte 1
+ .byte 0x0 /* Terminate children */
+.Lcu1_end:
+ .section .note.gnu.build-id, "a", %note
+ .4byte 4
+ .4byte 8
+ .4byte 3
+ .ascii "GNU\0"
+ .byte 0x01
+ .byte 0x02
+ .byte 0x03
+ .byte 0x04
+ .byte 0x05
+ .byte 0x06
+ .byte 0x07
+ .byte 0x08
+ .section .debug_abbrev
+.Labbrev1_begin:
+ .uleb128 2 /* Abbrev start */
+ .uleb128 0x11 /* DW_TAG_compile_unit */
+ .byte 1 /* has_children */
+ .byte 0x0 /* Terminator */
+ .byte 0x0 /* Terminator */
+ .uleb128 3 /* Abbrev start */
+ .uleb128 0x34 /* DW_TAG_variable */
+ .byte 0 /* has_children */
+ .uleb128 0x03 /* DW_AT_name */
+ .uleb128 0x08 /* DW_FORM_string */
+ .uleb128 0x49 /* DW_AT_type */
+ .uleb128 0x13 /* DW_FORM_ref4 */
+ .byte 0x0 /* Terminator */
+ .byte 0x0 /* Terminator */
+ .uleb128 4 /* Abbrev start */
+ .uleb128 0x01 /* DW_TAG_array_type */
+ .byte 1 /* has_children */
+ .uleb128 0x49 /* DW_AT_type */
+ .uleb128 0x13 /* DW_FORM_ref4 */
+ .byte 0x0 /* Terminator */
+ .byte 0x0 /* Terminator */
+ .uleb128 5 /* Abbrev start */
+ .uleb128 0x21 /* DW_TAG_subrange_type */
+ .byte 0 /* has_children */
+ .uleb128 0x22 /* DW_AT_lower_bound */
+ .uleb128 0x0b /* DW_FORM_data1 */
+ .uleb128 0x2f /* DW_AT_upper_bound */
+ .uleb128 0x05 /* DW_FORM_data2 */
+ .byte 0x0 /* Terminator */
+ .byte 0x0 /* Terminator */
+ .uleb128 6 /* Abbrev start */
+ .uleb128 0x24 /* DW_TAG_base_type */
+ .byte 0 /* has_children */
+ .uleb128 0x0b /* DW_AT_byte_size */
+ .uleb128 0x0b /* DW_FORM_data1 */
+ .byte 0x0 /* Terminator */
+ .byte 0x0 /* Terminator */
+ .byte 0x0 /* Terminator */
+ .byte 0x0 /* Terminator */
--
2.14.3
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libdw: dwarf_formsdata should return a signed value
2018-01-14 2:22 [PATCH] libdw: dwarf_formsdata should return a signed value Petr Machata
@ 2018-01-14 16:28 ` Mark Wielaard
[not found] ` <CAKfwACU9zq_s_W-0CierBK1PW8HDunTbZkL6-A6kRZb5SCK50A@mail.gmail.com>
0 siblings, 1 reply; 5+ messages in thread
From: Mark Wielaard @ 2018-01-14 16:28 UTC (permalink / raw)
To: Petr Machata; +Cc: elfutils-devel
Hi Petr,
On Sun, Jan 14, 2018 at 03:22:05AM +0100, Petr Machata wrote:
> The function dwarf_formsdata is used for decoding signed values, but
> except for the variable-length DW_FORM_sdata, it uses unsigned
> primitives to decode the value. This is not a problem for 64-bit values,
> but the smaller values come decoded wrong. Fix by changing to signed
> primitives for decoding the fixed-length forms.
>
> Add a test case that uses dwarf_aggregate_size to determine an array
> size whose lower bound is -1, encoded using DW_FORM_data1, and upper
> bound 255 with DW_FORM_data2. When the -1 is decoded wrongly, it comes
> back as 255, and the array size is 1. The correct array size should be
> 257.
Thanks for the patch and testcase. The change is obviously correct.
I wonder why we never noticed before. Applied to master including adding
the new test files to EXTRA_DIST to make sure they get into the dist
tar ball.
Cheers,
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 831532b2..758f20e6 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,9 @@
+2018-01-14 Petr Machata <pmachata@gmail.com>
+
+ * testfile-sizes4.o.bz2: New test file.
+ * testfile-sizes4.s: New test source.
+ * run-aggregate-size.sh: Check testfile-sizes4.o v size 257.
+
2017-12-23 Mark Wielaard <mark@klomp.org>
* backtrace-subr.sh (check_native_core): Use a lock file and try
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 64cb5bd9..1fce4474 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -333,7 +333,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
run-deleted.sh run-linkmap-cut.sh linkmap-cut-lib.so.bz2 \
linkmap-cut.bz2 linkmap-cut.core.bz2 \
run-aggregate-size.sh testfile-sizes1.o.bz2 testfile-sizes2.o.bz2 \
- testfile-sizes3.o.bz2 run-peel-type.sh \
+ testfile-sizes3.o.bz2 testfile-sizes4.o.bz2 testfile-sizes4.s \
+ run-peel-type.sh \
run-readelf-A.sh testfileppc32attrs.o.bz2 \
testfilesparc64attrs.o.bz2 testfileppc64attrs.o.bz2 \
testfile-debug-types.bz2 \
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-01-15 19:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-14 2:22 [PATCH] libdw: dwarf_formsdata should return a signed value Petr Machata
2018-01-14 16:28 ` Mark Wielaard
[not found] ` <CAKfwACU9zq_s_W-0CierBK1PW8HDunTbZkL6-A6kRZb5SCK50A@mail.gmail.com>
2018-01-15 8:24 ` Mark Wielaard
2018-01-15 11:34 ` Petr Machata
2018-01-15 19:28 ` Mark Wielaard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).