Re: dwarf_aggregate_size() seems to fall over on pointer types

Re: dwarf_aggregate_size() seems to fall over on pointer types

[Josh Stone]

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

On 10/01/2014 11:34 AM, Conrad Rad wrote:
> What was the issue with MIPS pointers, by the way?

https://sourceware.org/ml/systemtap/2014-q3/msg00071.html
(this was on elfutils-devel too, but that archive breaks this thread
across July/August)

So 64-bit MIPS may have some CU address_size == 4.  I would guess that
any pointer/ref byte_size is also 4 in that case, as Mark has assumed in
his patch, but I'm not sure.

Re: dwarf_aggregate_size() seems to fall over on pointer types

[Mark Wielaard]

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

On Thu, Oct 02, 2014 at 08:50:20AM -0400, Conrad Rad wrote:
> If you're going to tag me for Tested-by, it's "Conrad Meyer." (Google
> just doesn't let me use my real name for email and a pseudonym for all
> their other services, so it's a compromise.)

Changed.

> Looks good to me, thanks Mark. Maybe document the architecture the
> object files were produced on / the tests are in reference to? The
> sizes seem hardcoded for 64-bit pointer / 64-bit long architectures.

Yes, added that to the testcase documentation.

> I don't know the proper way to run elfutils tests, but I gave it a go:
> 
> $ export srcdir=`pwd`; ./run-aggregate-size.sh
> rmdir: failed to remove ‘test-20508’: Directory not empty
> $ echo $?
> 1
> $ ls test-20508
> testfile-sizes1.o
> 
> 
> No other output. I'm probably doing it wrong, but I didn't find any
> documentation in the tests/ directory.

The easiest way to run one test is:
make check TESTS=run-aggregate-size.sh

Pushed the fix with one addition. Added the new testfiles to EXTRA_DIST.

Thanks,

Mark

Re: dwarf_aggregate_size() seems to fall over on pointer types

[Conrad Rad]

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

On Thu, Oct 2, 2014 at 8:05 AM, Mark Wielaard <mjw@redhat.com> wrote:
> Oops. That shows why I shouldn't submit patches without testcases...
> Thanks for testing and fixing.
>
> Here is the fixed up patch plus some testcases to show it actually
> works.
>
> Cheers,
>
> Mark


If you're going to tag me for Tested-by, it's "Conrad Meyer." (Google
just doesn't let me use my real name for email and a pseudonym for all
their other services, so it's a compromise.)

Looks good to me, thanks Mark. Maybe document the architecture the
object files were produced on / the tests are in reference to? The
sizes seem hardcoded for 64-bit pointer / 64-bit long architectures.
Not a big deal.

I don't know the proper way to run elfutils tests, but I gave it a go:

$ export srcdir=`pwd`; ./run-aggregate-size.sh
rmdir: failed to remove ‘test-20508’: Directory not empty
$ echo $?
1
$ ls test-20508
testfile-sizes1.o


No other output. I'm probably doing it wrong, but I didn't find any
documentation in the tests/ directory.

Thanks,
Conrad

Re: dwarf_aggregate_size() seems to fall over on pointer types

[Mark Wielaard]

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

On Wed, 2014-10-01 at 15:26 -0400, Conrad Rad wrote:
> On Wed, Oct 1, 2014 at 3:23 PM, Conrad Rad <cse.cem@gmail.com> wrote:
> > This should be 0, 0x8. The size goes in the *size, return value of
> > dwarf_aggregate_size() should be zero on success, not 8. The patch is
> > not good as is.
> >
> With the fixed patch (attached), it works:

Oops. That shows why I shouldn't submit patches without testcases...
Thanks for testing and fixing.

Here is the fixed up patch plus some testcases to show it actually
works.

Cheers,

Mark

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-libdw-dwarf_aggregate_size-return-CU-address_size-fo.patch --]
[-- Type: text/x-patch, Size: 11987 bytes --]

From 8b75c34913ced8257b57397043380bc4c23d7067 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Thu, 2 Oct 2014 14:00:47 +0200
Subject: [PATCH] libdw: dwarf_aggregate_size return CU address_size for
 sizeless pointer/refs.

Tested-by: Conrad Rad <cse.cem@gmail.com>
Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdw/ChangeLog              |   6 ++++
 libdw/dwarf_aggregate_size.c |  10 +++++-
 tests/ChangeLog              |  10 ++++++
 tests/Makefile.am            |   5 +--
 tests/aggregate_size.c       |  83 +++++++++++++++++++++++++++++++++++++++++++
 tests/run-aggregate-size.sh  |  66 ++++++++++++++++++++++++++++++++++
 tests/testfile-sizes1.o.bz2  | Bin 0 -> 1012 bytes
 tests/testfile-sizes2.o.bz2  | Bin 0 -> 1283 bytes
 8 files changed, 177 insertions(+), 3 deletions(-)
 create mode 100644 tests/aggregate_size.c
 create mode 100755 tests/run-aggregate-size.sh
 create mode 100644 tests/testfile-sizes1.o.bz2
 create mode 100644 tests/testfile-sizes2.o.bz2

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 410b31a..29edc46 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,9 @@
+2014-10-02  Mark Wielaard  <mjw@redhat.com>
+
+	* dwarf_aggregate_size.c (aggregate_size): Return CU address_size
+	for sizeless DW_TAG_pointer_type, DW_TAG_reference_type or
+	DW_TAG_rvalue_reference_type.
+
 2014-09-12  Petr Machata  <pmachata@redhat.com>
 
 	* memory-access.h (read_ubyte_unaligned_inc): Allow only 4- and
diff --git a/libdw/dwarf_aggregate_size.c b/libdw/dwarf_aggregate_size.c
index 07c53a2..5d23541 100644
--- a/libdw/dwarf_aggregate_size.c
+++ b/libdw/dwarf_aggregate_size.c
@@ -1,5 +1,5 @@
 /* Compute size of an aggregate type from DWARF.
-   Copyright (C) 2010 Red Hat, Inc.
+   Copyright (C) 2010, 2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -205,6 +205,14 @@ aggregate_size (Dwarf_Die *die, Dwarf_Word *size, Dwarf_Die *type_mem)
 
     case DW_TAG_array_type:
       return array_size (die, size, &attr_mem, type_mem);
+
+    /* Assume references and pointers have pointer size if not given an
+       explicit DW_AT_byte_size.  */
+    case DW_TAG_pointer_type:
+    case DW_TAG_reference_type:
+    case DW_TAG_rvalue_reference_type:
+      *size = die->cu->address_size;
+      return 0;
     }
 
   /* Most types must give their size directly.  */
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 96320fa..b691007 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,13 @@
+2014-10-02  Mark Wielaard  <mjw@redhat.com>
+
+	* Makefile.am (check_PROGRAMS): Add aggregate_size.c.
+	(TESTS, EXTRA_DIST): Add run-aggregate-size.sh.
+	(aggregate_size_LDADD): New variable.
+	* aggregate_size.c: New file.
+	* run-aggregate-size.sh: New test.
+	* testfile-sizes1.o.bz2: New test file.
+	* testfile-sizes2.o.bz2: Likewise.
+
 2014-09-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	Support NT_FILE for locating files.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index ebf4f71..133501f 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -50,7 +50,7 @@ check_PROGRAMS = arextract arsymtest newfile saridx scnnames sectiondump \
 		  test-elf_cntl_gelf_getshdr dwflsyms dwfllines \
 		  dwfl-report-elf-align varlocs backtrace backtrace-child \
 		  backtrace-data backtrace-dwarf debuglink debugaltlink \
-		  buildid deleted deleted-lib.so
+		  buildid deleted deleted-lib.so aggregate_size
 
 asm_TESTS = asm-tst1 asm-tst2 asm-tst3 asm-tst4 asm-tst5 \
 	    asm-tst6 asm-tst7 asm-tst8 asm-tst9
@@ -110,7 +110,7 @@ TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \
 	run-backtrace-core-aarch64.sh \
 	run-backtrace-demangle.sh run-stack-d-test.sh run-stack-i-test.sh \
 	run-readelf-dwz-multi.sh run-allfcts-multi.sh run-deleted.sh \
-	run-linkmap-cut.sh
+	run-linkmap-cut.sh run-aggregate-size.sh
 
 if !BIARCH
 export ELFUTILS_DISABLE_BIARCH = 1
@@ -412,6 +412,7 @@ buildid_LDADD = $(libdw) $(libelf)
 deleted_LDADD = ./deleted-lib.so
 deleted_lib_so_LDFLAGS = -shared -rdynamic
 deleted_lib_so_CFLAGS = -fPIC
+aggregate_size_LDADD = $(libdw) $(libelf)
 
 if GCOV
 check: check-am coverage
diff --git a/tests/aggregate_size.c b/tests/aggregate_size.c
new file mode 100644
index 0000000..930eafa
--- /dev/null
+++ b/tests/aggregate_size.c
@@ -0,0 +1,83 @@
+/* Test program for dwarf_aggregate_size. Prints size of top-level vars.
+   Copyright (C) 2014 Red Hat, Inc.
+   This file is part of elfutils.
+
+   This file is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   elfutils is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include <assert.h>
+#include <argp.h>
+#include <inttypes.h>
+#include <fcntl.h>
+#include ELFUTILS_HEADER(dw)
+#include ELFUTILS_HEADER(dwfl)
+#include <stdio.h>
+#include <unistd.h>
+#include <dwarf.h>
+
+void
+print_var_type_size (Dwarf_Die *var)
+{
+  Dwarf_Attribute attr_mem;
+  Dwarf_Die type_mem;
+  Dwarf_Die *type;
+  const char *name = dwarf_diename (var);
+
+  type = dwarf_formref_die (dwarf_attr (var, DW_AT_type, &attr_mem),
+			    &type_mem);
+  if (type != NULL)
+    {
+      Dwarf_Word size;
+      if (dwarf_aggregate_size (type, &size) < 0)
+        printf ("%s no size: %s\n", name, dwarf_errmsg (-1));
+      else
+	printf ("%s size %" PRIu64 "\n", name, size);
+    }
+  else
+    printf ("%s has no type.\n", name);
+}
+
+int
+main (int argc, char *argv[])
+{
+
+  int remaining;
+  Dwfl *dwfl;
+  (void) argp_parse (dwfl_standard_argp (), argc, argv, 0, &remaining,
+                     &dwfl);
+  assert (dwfl != NULL);
+
+  Dwarf_Die *cu = NULL;
+  Dwarf_Addr dwbias;
+  while ((cu = dwfl_nextcu (dwfl, cu, &dwbias)) != NULL)
+    {
+      Dwarf_Die die_mem;
+      Dwarf_Die *die = &die_mem;
+      dwarf_child (cu, &die_mem);
+
+      while (1)
+	{
+	  if (dwarf_tag (die) == DW_TAG_variable)
+	    print_var_type_size (die);
+
+	  if (dwarf_siblingof (die, &die_mem) != 0)
+	    break;
+	}
+    }
+
+  dwfl_end (dwfl);
+}
diff --git a/tests/run-aggregate-size.sh b/tests/run-aggregate-size.sh
new file mode 100755
index 0000000..82a5d53
--- /dev/null
+++ b/tests/run-aggregate-size.sh
@@ -0,0 +1,66 @@
+#! /bin/sh
+# Copyright (C) 2014 Red Hat, Inc.
+# This file is part of elfutils.
+#
+# This file is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# elfutils is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+. $srcdir/test-subr.sh
+
+# char c;
+# int i;
+# long l;
+#
+# void *v;
+#
+# struct s
+# {
+#   char *a;
+#   int i;
+# } s;
+#
+# char ca[16];
+# int ia[32];
+# void *va[64];
+# struct s sa[8];
+
+# gcc -g -c -o testfile-sizes1.o sizes.c
+# clang -g -c -o testfile-sizes2.o sizes.c
+
+testfiles testfile-sizes1.o testfile-sizes2.o
+
+testrun_compare ${abs_builddir}/aggregate_size -e testfile-sizes1.o <<\EOF
+c size 1
+i size 4
+l size 8
+v size 8
+s size 16
+ca size 16
+ia size 128
+va size 512
+sa size 128
+EOF
+
+testrun_compare ${abs_builddir}/aggregate_size -e testfile-sizes2.o <<\EOF
+c size 1
+i size 4
+l size 8
+v size 8
+s size 16
+ca size 16
+ia size 128
+va size 512
+sa size 128
+EOF
+
+exit 0
diff --git a/tests/testfile-sizes1.o.bz2 b/tests/testfile-sizes1.o.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..479ecb20149dfd1a2520aead7fe3562e77b6681b
GIT binary patch
literal 1012
zcmV<Q0}K2@T4*^jL0KkKSvxE!LjVJ_|NsC0|L^Ya^<UrZ(h0xs-|*rjz(@!HKtKRU
z1PBNKI7rX~L%>KYZHzP_G$thvH5!l9^rw<~8Bb4C(@mt)6DC7LY5)&VWN5_GY3gYW
z10fNqsp+PgMDk?B37{UJ0009(0000000E$I00000000I<KmY&$000008~^|S00001
zkk9}C0000000KlwsivvoX{Mtln@EO+CPtV?qJE&7G&DU;G#X@K0j6kW7(s_|uDZrJ
z?_<WWB4j2?I28KJUiR$xIQeMP7_`SqUOj0zY;&kTLb%36243z4OA=LlbqA^BlmPP)
z!?hh-FIsG8V<OF{=}{Bt;HSTzaiUOX9vD|>!OW!q!$X-^>QdI(q^Z@BwaNXgP8;6;
zkLb_Lw`%`6hI}0fNk%_cEd(PwORv-6TR|Fc>v1qs%Ly%9+So1W{uiWMm?`A>JidEz
zf6#<z@*Fadkq;C;A11n-y!pNxgk0NB?0c*hSYw%~lO&{7g6c34g`iT<ue49=3w+Yw
zw1b|-Ru`IX^g!5z8!a1bM`Moy)Y*>)*Ah&0zX2g@LK{N2uLuz>ENn(*64PimA2s_A
zw7@KBCdAkk;|>I+ohAXXiU1C4G&eBq*TXqyXtmN7t5)o9Cwpzus494&RR(kv-nK~q
zk~a3|@00nhjwL5w?k~xngt8+C0kn#!Lf^*t?&W#hiPJ%tV<^Kcp#YICqS_44pAMh)
zb`ZZXKt2YG`}jtViVb;0<j_GVS~kE03`mBxwKSBavczCvm5%Y2Ql=PK#)}wLw_`zZ
zaR{-)hr2ATlw@o6=uw<Tg=MWp5HeVJT>l;Xlmj-wmT@q-g?tH+o#GLdhU+_m^WMqm
z>i;uZ)?)@pENC>#2n(q)VTeI33L~QL#T8*j7hKSe7&9H2-D4lDr5ps5y@OLTR`@ye
z#~0ja(Yb}pw{Fyp>C@RP1}xRmDPpp|$ZrFoq0voqhJ%Nz7IrHp6498ogKYf(I6<(6
zNPsK=+3A-6`%QpQkg+P-iQj{q3e5SYB;^&C8lsrCwGl(-N*9HS&{2uxBfk+SM*#d1
zIC%ssCN0~mwzV$kS$@xFUVO(q<MBhQt-(XrdQTP#QXT6mF@Ju23^RcqV&kQzbB!V(
zrxQCK#c#p~aKZ|f?8XGDQlY)AU;&y4N?;=cEy){li%85J<ca3ZSjDA5x1eeCgT-Vi
z)U;wqh|9VpWMeQiVi`(#_1t7zxF|MAVJmPnh)e)<#~?if=c7pyzdXrAzyu(mRnZe9
i8HIFhv3m1DWV-I^u!uiZ&Q4MT{x0N-aG@Y}SWt$D(ZqrP

literal 0
HcmV?d00001

diff --git a/tests/testfile-sizes2.o.bz2 b/tests/testfile-sizes2.o.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..7bd7b47f74254dec3f4084abc0467294d7f3b6d6
GIT binary patch
literal 1283
zcmV+e1^oI#T4*^jL0KkKSxt<?MgRqj|NsC0|Ly<l{r~^x+Ts82|9Rk{;4%XhK@4C^
zNEkGM^9axcT3b90R?KqU!-FWMo~fzi%4VjXOoo7)Q_^~RlhkRALjWhJFc1crs2+l5
zgCz8xk&O)ongV%D2~$asOpFmdOsDA(Xahh2lS4o~5NH4Z0000000E!_O%REYr|KuE
z>UmFUAZQu`LqIeEqd*NFkN^Mx0017Qk5B+;g8)Veh|?w!ki;;ICIChekjbV)Mn)je
zz)VaDh|z$InJ@_wgr~Ao+L5MB83uzu0imE|WB@b(0000041fRupcvA<9{7|9$`Zyt
z1_hT6bPy=2G3zKaUd-t#y)KtaX5D@l-qIc{nGPeyir%PGfn{9x!E5Hh+sNpL7BroS
zQ#xy$gm%Sz=b_TTvGp~^R7|-!wU66=S$EN(*hf~V{VAk|mlmwhLQnz?yda0@gMsC|
z!NP5{&ce~UN)g2fWGUA3aQMAneoGVYhb`^sotUKJ!i|&F<O0&)+G}50#9A#CIw{?%
z@R5$Y4%+t@Zw54(I$?;5IH{wQ#y}cdj^!#|)@J)YZ-tuev#@-RWw8@h;OAHtQ#GJ-
z#8Ct}Uz3$!K_gT9oi)c{L>0;dq_AKXoR9%}y6_ix&D*MKhJcV7p+tj<@Cc;4xw*c@
zSY1@=UeJ=DV8#p;Xds-484PIa{w!oLKek&;)WJQcHcbmsmTduG(i*ma7CDI^wP+Nr
zAeoCk)HItAts7#7gaIq<w1Q?OBO60#UOTuIW{i6P)?h*=X$iEFGlfqShy^P^n@x#M
z(1>WsL<4C6*pS*l+8ansiLqa>`ZD!}^A13?y?=*#>|hc{ejzHxmdKK2tZqC@!6_+6
zkfMUCC}NbALknsoRg|D(L=f>tb`OHQ5>he*hW_~gmh7!(>C@wNKYi7~kX)K~GL4y-
zhVdYsNUY#h*qa-L4?ZW#UuFW-r$V4KOO2A_p(4pfAuEzmN*+3~Y7v$L{U@SyB_TnA
z$#8?h%^}hV0g<1RmZ&5J5XHS1C4nTFfSa*k3uSLjdo?m9VN#d`=!FwOJ9mKsA&68e
zAr68iGBHHq2AXSHu~Mwpnkr`(?am}fiX<r7*)VYNV1O}5p9&$!Go}ytqbWp6g)zp*
zt!a>eNqDq~w67S)6sMxP#Y!bW1*UZ5q`}}qV_%-&A}9pUR79Xc2Yp4;Bnu%yC5R&h
zRRSp)z<}T+CCw{?A9ZiNZ2cJFmuv7peSGB1TuBv!R5r1r#6CsF>ePzE5lAAGM&Rj&
z$8j4w3c^V&|3h9^u6hTWOfCoEDsQe?+O>KyQ<PWdA2bqgu!>@J<FG%!pw}_PO)+q?
zB)C|5X_n~1D*jrscxP6*3M&@^CX}|esRrKuYjQ$DE&_nOo|5>iF$D0XZ1+Ie_Pk5r
zLVd^-Wuge62y(JeNTw0VR;Z!?Ym<}<#o;OUk{i$25p;n#p$SR|?7%9e!V9;r-4W#!
zvNoy`$S_C76t1nS53l%xr6{G0424DoO0tsMjnTrA#pYH?$cX4c%c(+kOgcRR4g<>=
zgNn9RaZpe!uS8a>Sbh|iY;+5<doh#XjgF-)2`0T35Khw+7eK5lRdnd8Dg_sJ^Roxu
tK~8DmoLu#E8DNU$CCsH+!H$>+J?MfNjc5BA08sz<yOJrwgo12l8Ze>lK<fYi

literal 0
HcmV?d00001

-- 
1.8.3.1

Re: dwarf_aggregate_size() seems to fall over on pointer types

[Roland McGrath]

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

> +    /* Assume references and pointers have pointer size if not given an
> +       explicit DW_AT_byte_size.  */
> +    case DW_TAG_pointer_type:
> +    case DW_TAG_reference_type:
> +    case DW_TAG_rvalue_reference_type:
> +      return die->cu->address_size;

This seems reasonable to me.

Re: dwarf_aggregate_size() seems to fall over on pointer types

[Conrad Rad]

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

On Wed, Oct 1, 2014 at 3:23 PM, Conrad Rad <cse.cem@gmail.com> wrote:
> On Wed, Oct 1, 2014 at 3:10 PM, Conrad Rad <cse.cem@gmail.com> wrote:
>> On Wed, Oct 1, 2014 at 2:33 PM, Mark Wielaard <mjw@redhat.com> wrote:
>>> On Wed, Oct 01, 2014 at 10:46:52AM -0700, Josh Stone wrote:
>>> How about the attached patch? (I haven't yet looked at the MIPS issue,
>>> but think it reasonable to require the producer to add an explicit
>>> DW_AT_byte_size if the assumption of address size is not correct.)
>>
>> On my repro, it fixes the pointer member, but not the array-of-pointer member.
>
> Oh, wow, I misread. No, that's wrong.
>
>> struct b {
>>         char *d;
>>         void *e[3];
>> ...
>> };
>>
>> $ ./repro a.out:
>> ...
>>         agg_size(b. (type  ) d): 8, 0xffffffff
>
> This should be 0, 0x8. The size goes in the *size, return value of
> dwarf_aggregate_size() should be zero on success, not 8. The patch is
> not good as is.
>
> Thanks,
> Conrad

With the fixed patch (attached), it works:

        agg_size(b.d): 0, 0x8
        agg_size(b.e): 0, 0x18

Thanks,
Conrad

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: a.patch --]
[-- Type: text/x-patch, Size: 1658 bytes --]

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 410b31a..e6716d6 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,5 +1,11 @@
+2014-10-01  Mark Wielaard  <mjw@redhat.com>
+
+	* dwarf_aggregate_size.c (aggregate_size): Return CU address_size
+	for sizeless DW_TAG_pointer_type, DW_TAG_reference_type or
+	DW_TAG_rvalue_reference_type.
+
 2014-09-12  Petr Machata  <pmachata@redhat.com>
 
 	* memory-access.h (read_ubyte_unaligned_inc): Allow only 4- and
 	8-byte quantities.  Consequently, rename to...
 	(read_addr_unaligned_inc): ... this.
diff --git a/libdw/dwarf_aggregate_size.c b/libdw/dwarf_aggregate_size.c
index 07c53a2..5d23541 100644
--- a/libdw/dwarf_aggregate_size.c
+++ b/libdw/dwarf_aggregate_size.c
@@ -1,7 +1,7 @@
 /* Compute size of an aggregate type from DWARF.
-   Copyright (C) 2010 Red Hat, Inc.
+   Copyright (C) 2010, 2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
    it under the terms of either
 
@@ -203,10 +203,18 @@ aggregate_size (Dwarf_Die *die, Dwarf_Word *size, Dwarf_Die *type_mem)
       return aggregate_size (get_type (die, &attr_mem, type_mem),
 			     size, type_mem); /* Tail call.  */
 
     case DW_TAG_array_type:
       return array_size (die, size, &attr_mem, type_mem);
+
+    /* Assume references and pointers have pointer size if not given an
+       explicit DW_AT_byte_size.  */
+    case DW_TAG_pointer_type:
+    case DW_TAG_reference_type:
+    case DW_TAG_rvalue_reference_type:
+      *size = die->cu->address_size;
+      return 0;
     }
 
   /* Most types must give their size directly.  */
   return -1;
 }

Re: dwarf_aggregate_size() seems to fall over on pointer types

[Conrad Rad]

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

On Wed, Oct 1, 2014 at 3:10 PM, Conrad Rad <cse.cem@gmail.com> wrote:
> On Wed, Oct 1, 2014 at 2:33 PM, Mark Wielaard <mjw@redhat.com> wrote:
>> On Wed, Oct 01, 2014 at 10:46:52AM -0700, Josh Stone wrote:
>> How about the attached patch? (I haven't yet looked at the MIPS issue,
>> but think it reasonable to require the producer to add an explicit
>> DW_AT_byte_size if the assumption of address size is not correct.)
>
> On my repro, it fixes the pointer member, but not the array-of-pointer member.

Oh, wow, I misread. No, that's wrong.

> struct b {
>         char *d;
>         void *e[3];
> ...
> };
>
> $ ./repro a.out:
> ...
>         agg_size(b. (type  ) d): 8, 0xffffffff

This should be 0, 0x8. The size goes in the *size, return value of
dwarf_aggregate_size() should be zero on success, not 8. The patch is
not good as is.

Thanks,
Conrad

Re: dwarf_aggregate_size() seems to fall over on pointer types

[Conrad Rad]

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

On Wed, Oct 1, 2014 at 2:33 PM, Mark Wielaard <mjw@redhat.com> wrote:
> On Wed, Oct 01, 2014 at 10:46:52AM -0700, Josh Stone wrote:
> How about the attached patch? (I haven't yet looked at the MIPS issue,
> but think it reasonable to require the producer to add an explicit
> DW_AT_byte_size if the assumption of address size is not correct.)

On my repro, it fixes the pointer member, but not the array-of-pointer member.

struct b {
        char *d;
        void *e[3];
...
};

$ ./repro a.out:
...
        agg_size(b. (type  ) d): 8, 0xffffffff
...
        agg_size(b. (type  ) e): -1, 0xffffffff
...

Thanks,
Conrad

Re: dwarf_aggregate_size() seems to fall over on pointer types

[Conrad Rad]

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

On Wed, Oct 1, 2014 at 1:46 PM, Josh Stone <jistone@redhat.com> wrote:
> On 10/01/2014 10:15 AM, Conrad Rad wrote:
>> I've compiled a small example program, repro, and output[2]:
>> http://pastie.org/9610702
>>
>> dwarf_aggregate_size() has no trouble with most member types, or even
>> pointer types on the output from GCC. However, it returns an error on
>> Clang's output.
>>
>> Perhaps Clang is just eliding pointer size information, assuming
>> consumers will consult the Elf machine class and infer?
>
> Indeed, clang doesn't seem to output DW_AT_byte_size on any
> DW_TAG_pointer_type, but gcc does.
>
> FWIW, the DWARF4 Appendix A does *not* list DW_AT_byte_size as being
> applicable to DW_TAG_pointer_type, nor DW_TAG_reference_type, nor
> DW_TAG_rvalue_reference_type.  However, that is just "informative", and
> producers are free to omit some of the suggestions as well as add their
> own, so it's not an error that gcc produces DW_AT_byte_size.
>
>> Am I missing something? Is this a Clang bug or an elfutils bug?
>
> I don't think it's strictly a bug in either.  Clang is not required to
> produce byte_size, and elfutils is just reporting what it knows.
>
> Still, it might be nice if elfutils made that machine-class inference
> for you.  (But I worry about things like that mips pointer issue
> reported recently.)

It'd be nice. If it's just pointers and arrays of pointers, I can
cobble a fallback together. Members that have struct types with
pointer members should get their own size information emitted by both
compilers, I guess? (Both generate size information for 'struct b'.)

What was the issue with MIPS pointers, by the way?

Thanks,
Conrad

Re: dwarf_aggregate_size() seems to fall over on pointer types

[Mark Wielaard]

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

On Wed, Oct 01, 2014 at 10:46:52AM -0700, Josh Stone wrote:
> > Am I missing something? Is this a Clang bug or an elfutils bug?
> 
> I don't think it's strictly a bug in either.  Clang is not required to
> produce byte_size, and elfutils is just reporting what it knows.
> 
> Still, it might be nice if elfutils made that machine-class inference
> for you.  (But I worry about things like that mips pointer issue
> reported recently.)

Agreed. It seems reasonable for a consumer to assume that if any of
the reference/pointer types don't have an explicit size then they
should assume the type has address size. The DWARF spec does explicitly
call out DW_AT_address_class for pointer types. But that isn't really
helpful since it then only defines DW_ADDR_none and leaves any other
value as architecture defined.

How about the attached patch? (I haven't yet looked at the MIPS issue,
but think it reasonable to require the producer to add an explicit
DW_AT_byte_size if the assumption of address size is not correct.)

Thanks,

Mark

[-- Attachment #2: 0001-libdw-dwarf_aggregate_size-return-CU-address_size-fo.patch --]
[-- Type: text/plain, Size: 1827 bytes --]

>From b29f1741856a179fd6a426534d19b1ed4807f6c1 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Wed, 1 Oct 2014 20:29:14 +0200
Subject: [PATCH] libdw: dwarf_aggregate_size return CU address_size for
 sizeless pointer/refs.

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdw/ChangeLog              | 6 ++++++
 libdw/dwarf_aggregate_size.c | 9 ++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 410b31a..e6716d6 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,9 @@
+2014-10-01  Mark Wielaard  <mjw@redhat.com>
+
+	* dwarf_aggregate_size.c (aggregate_size): Return CU address_size
+	for sizeless DW_TAG_pointer_type, DW_TAG_reference_type or
+	DW_TAG_rvalue_reference_type.
+
 2014-09-12  Petr Machata  <pmachata@redhat.com>
 
 	* memory-access.h (read_ubyte_unaligned_inc): Allow only 4- and
diff --git a/libdw/dwarf_aggregate_size.c b/libdw/dwarf_aggregate_size.c
index 07c53a2..cfc2639 100644
--- a/libdw/dwarf_aggregate_size.c
+++ b/libdw/dwarf_aggregate_size.c
@@ -1,5 +1,5 @@
 /* Compute size of an aggregate type from DWARF.
-   Copyright (C) 2010 Red Hat, Inc.
+   Copyright (C) 2010, 2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -205,6 +205,13 @@ aggregate_size (Dwarf_Die *die, Dwarf_Word *size, Dwarf_Die *type_mem)
 
     case DW_TAG_array_type:
       return array_size (die, size, &attr_mem, type_mem);
+
+    /* Assume references and pointers have pointer size if not given an
+       explicit DW_AT_byte_size.  */
+    case DW_TAG_pointer_type:
+    case DW_TAG_reference_type:
+    case DW_TAG_rvalue_reference_type:
+      return die->cu->address_size;
     }
 
   /* Most types must give their size directly.  */
-- 
1.9.3

Re: dwarf_aggregate_size() seems to fall over on pointer types

[Josh Stone]

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

On 10/01/2014 10:15 AM, Conrad Rad wrote:
> Hi all,
> 
> First, I'm not super familiar with DWARF, and perhaps this is a bug in
> the DWARF information emitted by Clang (3.4). It's definitely a
> difference between GCC and Clang DWARF output.
> 
> I've been fiddling with a pahole-alike tool[0] that sits directly on
> elfutils (as opposed to libdwarves, which hasn't been updated for some
> time and doesn't handle some of the tags Clang emits). I've observed
> that dwarf_aggregate_size() doesn't seem to like types that are
> pointers or arrays of pointers[1].
> 
> I've compiled a small example program, repro, and output[2]:
> http://pastie.org/9610702
> 
> dwarf_aggregate_size() has no trouble with most member types, or even
> pointer types on the output from GCC. However, it returns an error on
> Clang's output.
> 
> Perhaps Clang is just eliding pointer size information, assuming
> consumers will consult the Elf machine class and infer?

Indeed, clang doesn't seem to output DW_AT_byte_size on any
DW_TAG_pointer_type, but gcc does.

FWIW, the DWARF4 Appendix A does *not* list DW_AT_byte_size as being
applicable to DW_TAG_pointer_type, nor DW_TAG_reference_type, nor
DW_TAG_rvalue_reference_type.  However, that is just "informative", and
producers are free to omit some of the suggestions as well as add their
own, so it's not an error that gcc produces DW_AT_byte_size.

> Am I missing something? Is this a Clang bug or an elfutils bug?

I don't think it's strictly a bug in either.  Clang is not required to
produce byte_size, and elfutils is just reporting what it knows.

Still, it might be nice if elfutils made that machine-class inference
for you.  (But I worry about things like that mips pointer issue
reported recently.)

dwarf_aggregate_size() seems to fall over on pointer types

[Conrad Rad]

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

Hi all,

First, I'm not super familiar with DWARF, and perhaps this is a bug in
the DWARF information emitted by Clang (3.4). It's definitely a
difference between GCC and Clang DWARF output.

I've been fiddling with a pahole-alike tool[0] that sits directly on
elfutils (as opposed to libdwarves, which hasn't been updated for some
time and doesn't handle some of the tags Clang emits). I've observed
that dwarf_aggregate_size() doesn't seem to like types that are
pointers or arrays of pointers[1].

I've compiled a small example program, repro, and output[2]:
http://pastie.org/9610702

dwarf_aggregate_size() has no trouble with most member types, or even
pointer types on the output from GCC. However, it returns an error on
Clang's output.

Perhaps Clang is just eliding pointer size information, assuming
consumers will consult the Elf machine class and infer?

Am I missing something? Is this a Clang bug or an elfutils bug?

Thanks much,
Conrad

[0]: https://github.com/cemeyer/structhole/
[1]: https://github.com/cemeyer/structhole/issues/1
[2]: http://pastie.org/9610702