* Re: [PATCH] dwarf-reader: Workaround libdw dwarf_location_expression bug
2021-12-15 16:00 ` [PATCH] dwarf-reader: Workaround libdw dwarf_location_expression bug Mark J. Wielaard
@ 2021-12-15 18:56 ` Giuliano Procida
2021-12-15 19:25 ` Mark J. Wielaard
2021-12-15 19:47 ` Thomas Schwinge
2021-12-17 18:47 ` Dodji Seketeli
2 siblings, 1 reply; 7+ messages in thread
From: Giuliano Procida @ 2021-12-15 18:56 UTC (permalink / raw)
To: Mark J. Wielaard; +Cc: Giuliano Procida via Libabigail
Hi Mark.
What's the relationship, if any, between this and the patch I posted on 24
Nov to address Clang DWARF 5 addresses?
I would check, but I'm currently on holiday. :-)
Regards,
Giuliano
On Wed, 15 Dec 2021, 20:21 Mark J. Wielaard, <mark@klomp.org> wrote:
> From: Mark Wielaard <mark@klomp.org>
>
> elfutils libdw before 0.184 would not correctly handle a
> DW_AT_data_member_location when encoded as a DW_FORM_implicit const
> in dwarf_location_expression.
>
> Work around this by first trying to read a data_member_location as a
> constant value and only try to get it as a DWARF expression if that
>
> * src/abg-dwarf-reader.cc (die_constant_data_member_location):
> New function.
> (die_member_offset): Use die_constant_data_member_location
> before calling die_location_expr and eval_quickly.
>
> Signed-off-by: Mark Wielaard <mark@klomp.org>
> ---
> src/abg-dwarf-reader.cc | 72 +++++++++++++++++++++++++++++++++--------
> 1 file changed, 58 insertions(+), 14 deletions(-)
>
>
> https://code.wildebeest.org/git/user/mjw/libabigail/commit/?h=data_member_location
>
> diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
> index ec92f3f8..3f716944 100644
> --- a/src/abg-dwarf-reader.cc
> +++ b/src/abg-dwarf-reader.cc
> @@ -8693,6 +8693,37 @@ read_and_convert_DW_at_bit_offset(const Dwarf_Die*
> die,
> return true;
> }
>
> +/// Get the value of the DW_AT_data_member_location of the given DIE
> +/// attribute as an constant.
> +///
> +/// @param die the DIE to read the attribute from.
> +///
> +/// @param offset the attribute as a constant value. This is set iff
> +/// the function returns true.
> +///
> +/// @return true if the attribute exists and has a constant value. In
> +/// that case the offset is set to the value.
> +static bool
> +die_constant_data_member_location(const Dwarf_Die *die,
> + int64_t& offset)
> +{
> + if (!die)
> + return false;
> +
> + Dwarf_Attribute attr;
> + if (!dwarf_attr(const_cast<Dwarf_Die*>(die),
> + DW_AT_data_member_location,
> + &attr))
> + return false;
> +
> + Dwarf_Word val;
> + if (dwarf_formudata(&attr, &val) != 0)
> + return false;
> +
> + offset = val;
> + return true;
> +}
> +
> /// Get the offset of a struct/class member as represented by the
> /// value of the DW_AT_data_member_location attribute.
> ///
> @@ -8758,21 +8789,34 @@ die_member_offset(const read_context& ctxt,
> return true;
> }
>
> - // Otherwise, let's see if the DW_AT_data_member_location attribute and,
> - // optionally, the DW_AT_bit_offset attributes are present.
> - if (!die_location_expr(die, DW_AT_data_member_location, &expr,
> &expr_len))
> - return false;
> -
> - // The DW_AT_data_member_location attribute is present.
> - // Let's evaluate it and get its constant
> - // sub-expression and return that one.
> - if (!eval_quickly(expr, expr_len, offset))
> - {
> - bool is_tls_address = false;
> - if (!eval_last_constant_dwarf_sub_expr(expr, expr_len,
> - offset, is_tls_address,
> - ctxt.dwarf_expr_eval_ctxt()))
> + // First try to read DW_AT_data_member_location as a plain constant.
> + // We do this because the generic method using die_location_expr
> + // might hit a bug in elfutils libdw dwarf_location_expression only
> + // fixed in elfutils 0.184+. The bug only triggers if the attribute
> + // is expressed as a (DWARF 5) DW_FORM_implicit_constant. But we
> + // handle all constants here because that is more consistent (and
> + // slightly faster in the general case where the attribute isn't a
> + // full DWARF expression).
> + if (!die_constant_data_member_location(die, offset))
> + {
> + // Otherwise, let's see if the DW_AT_data_member_location
> + // attribute and, optionally, the DW_AT_bit_offset attributes
> + // are present.
> + if (!die_location_expr(die, DW_AT_data_member_location,
> + &expr, &expr_len))
> return false;
> +
> + // The DW_AT_data_member_location attribute is present. Let's
> + // evaluate it and get its constant sub-expression and return
> + // that one.
> + if (!eval_quickly(expr, expr_len, offset))
> + {
> + bool is_tls_address = false;
> + if (!eval_last_constant_dwarf_sub_expr(expr, expr_len,
> + offset, is_tls_address,
> +
> ctxt.dwarf_expr_eval_ctxt()))
> + return false;
> + }
> }
> offset *= 8;
>
> --
> 2.18.4
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] dwarf-reader: Workaround libdw dwarf_location_expression bug
2021-12-15 16:00 ` [PATCH] dwarf-reader: Workaround libdw dwarf_location_expression bug Mark J. Wielaard
2021-12-15 18:56 ` Giuliano Procida
@ 2021-12-15 19:47 ` Thomas Schwinge
2021-12-17 18:47 ` Dodji Seketeli
2 siblings, 0 replies; 7+ messages in thread
From: Thomas Schwinge @ 2021-12-15 19:47 UTC (permalink / raw)
To: Mark J. Wielaard, libabigail
Hi!
On 2021-12-15T17:00:21+0100, "Mark J. Wielaard" <mark@klomp.org> wrote:
> From: Mark Wielaard <mark@klomp.org>
>
> elfutils libdw before 0.184 would not correctly handle a
> DW_AT_data_member_location when encoded as a DW_FORM_implicit const
> in dwarf_location_expression.
>
> Work around this by first trying to read a data_member_location as a
> constant value and only try to get it as a DWARF expression if that
>
> * src/abg-dwarf-reader.cc (die_constant_data_member_location):
> New function.
> (die_member_offset): Use die_constant_data_member_location
> before calling die_location_expr and eval_quickly.
>
> Signed-off-by: Mark Wielaard <mark@klomp.org>
Tested-by: Thomas Schwinge <thomas@codesourcery.com>
Due to infamiliarity with the API, I cannot comment on the code changes,
but I'm confirming that this does resolve the 'runtestreaddwarf'
testsuite regression that I as well as Mark's buildbot have seen with
recent commit f76484cc9da0befa90e9c60867ce508202192282
"Add regression tests for ctf reading". In my case, with Ubuntu focal
0.176-1.1build1 elfutils, these were:
diff --git build-libabigail/tests/runtestreaddwarf.log build-libabigail/tests/runtestreaddwarf.log
index 243f92e..74ae893 100644
--- build-libabigail/tests/runtestreaddwarf.log
+++ build-libabigail/tests/runtestreaddwarf.log
@@ -1,5 +1,49 @@
Could not load elf symtab: Skipping symtab load.
-Symbol table of '[...]/source-libabigail/tests/data/test-read-dwarf/test25-bogus-binary.elf' could not be loaded
-Could not load elf symtab: Skipping symtab load.
Symbol table of '[...]/source-libabigail/tests/data/test-read-dwarf/test26-bogus-binary.elf' could not be loaded
-PASS runtestreaddwarf (exit status: 0)
+Could not load elf symtab: Skipping symtab load.
+Symbol table of '[...]/source-libabigail/tests/data/test-read-dwarf/test25-bogus-binary.elf' could not be loaded
+--- [...]/source-libabigail/tests/data/test-read-dwarf/test-PR26568-2.o.abi 2021-12-14 20:09:43.841418541 +0100
++++ [...]/build-libabigail/tests/output/test-read-dwarf/test-PR26568-2.o.abi 2021-12-14 20:28:52.053435017 +0100
+@@ -14,12 +14,12 @@
+ </data-member>
+ </union-decl>
+ <class-decl name='__anonymous_struct__' size-in-bits='32' is-struct='yes' is-anonymous='yes' visibility='default' filepath='tests/data/test-read-common/test-PR26568-2.c' line='5' column='1' id='type-id-4'>
+- <data-member access='public' layout-offset-in-bits='0'>
++ <data-member access='public' static='yes'>
+ <var-decl name='x' type-id='type-id-1' visibility='default' filepath='tests/data/test-read-common/test-PR26568-2.c' line='6' column='1'/>
+ </data-member>
+ </class-decl>
+ <class-decl name='__anonymous_struct__1' size-in-bits='64' is-struct='yes' is-anonymous='yes' visibility='default' filepath='tests/data/test-read-common/test-PR26568-2.c' line='8' column='1' id='type-id-5'>
+- <data-member access='public' layout-offset-in-bits='0'>
++ <data-member access='public' static='yes'>
+ <var-decl name='y' type-id='type-id-2' visibility='default' filepath='tests/data/test-read-common/test-PR26568-2.c' line='9' column='1'/>
+ </data-member>
+ </class-decl>
+--- [...]/source-libabigail/tests/data/test-read-dwarf/test-PR26568-1.o.abi 2021-12-14 20:09:43.841418541 +0100
++++ [...]/build-libabigail/tests/output/test-read-dwarf/test-PR26568-1.o.abi 2021-12-14 20:28:52.049435017 +0100
+@@ -19,12 +19,12 @@
+ </data-member>
+ </union-decl>
+ <class-decl name='__anonymous_struct__' size-in-bits='32' is-struct='yes' is-anonymous='yes' visibility='default' filepath='tests/data/test-read-common/test-PR26568-1.c' line='6' column='1' id='type-id-5'>
+- <data-member access='public' layout-offset-in-bits='0'>
++ <data-member access='public' static='yes'>
+ <var-decl name='x' type-id='type-id-1' visibility='default' filepath='tests/data/test-read-common/test-PR26568-1.c' line='7' column='1'/>
+ </data-member>
+ </class-decl>
+ <class-decl name='__anonymous_struct__1' size-in-bits='64' is-struct='yes' is-anonymous='yes' visibility='default' filepath='tests/data/test-read-common/test-PR26568-1.c' line='9' column='1' id='type-id-6'>
+- <data-member access='public' layout-offset-in-bits='0'>
++ <data-member access='public' static='yes'>
+ <var-decl name='y' type-id='type-id-2' visibility='default' filepath='tests/data/test-read-common/test-PR26568-1.c' line='10' column='1'/>
+ </data-member>
+ </class-decl>
+ABIs differ:
+[...]/source-libabigail/tests/data/test-read-dwarf/test-PR26568-2.o.abi
+and:
+[...]/build-libabigail/tests/output/test-read-dwarf/test-PR26568-2.o.abi
+
+ABIs differ:
+[...]/source-libabigail/tests/data/test-read-dwarf/test-PR26568-1.o.abi
+and:
+[...]/build-libabigail/tests/output/test-read-dwarf/test-PR26568-1.o.abi
+
+FAIL runtestreaddwarf (exit status: 1)
Grüße
Thomas
> src/abg-dwarf-reader.cc | 72 +++++++++++++++++++++++++++++++++--------
> 1 file changed, 58 insertions(+), 14 deletions(-)
>
> https://code.wildebeest.org/git/user/mjw/libabigail/commit/?h=data_member_location
>
> diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
> index ec92f3f8..3f716944 100644
> --- a/src/abg-dwarf-reader.cc
> +++ b/src/abg-dwarf-reader.cc
> @@ -8693,6 +8693,37 @@ read_and_convert_DW_at_bit_offset(const Dwarf_Die* die,
> return true;
> }
>
> +/// Get the value of the DW_AT_data_member_location of the given DIE
> +/// attribute as an constant.
> +///
> +/// @param die the DIE to read the attribute from.
> +///
> +/// @param offset the attribute as a constant value. This is set iff
> +/// the function returns true.
> +///
> +/// @return true if the attribute exists and has a constant value. In
> +/// that case the offset is set to the value.
> +static bool
> +die_constant_data_member_location(const Dwarf_Die *die,
> + int64_t& offset)
> +{
> + if (!die)
> + return false;
> +
> + Dwarf_Attribute attr;
> + if (!dwarf_attr(const_cast<Dwarf_Die*>(die),
> + DW_AT_data_member_location,
> + &attr))
> + return false;
> +
> + Dwarf_Word val;
> + if (dwarf_formudata(&attr, &val) != 0)
> + return false;
> +
> + offset = val;
> + return true;
> +}
> +
> /// Get the offset of a struct/class member as represented by the
> /// value of the DW_AT_data_member_location attribute.
> ///
> @@ -8758,21 +8789,34 @@ die_member_offset(const read_context& ctxt,
> return true;
> }
>
> - // Otherwise, let's see if the DW_AT_data_member_location attribute and,
> - // optionally, the DW_AT_bit_offset attributes are present.
> - if (!die_location_expr(die, DW_AT_data_member_location, &expr, &expr_len))
> - return false;
> -
> - // The DW_AT_data_member_location attribute is present.
> - // Let's evaluate it and get its constant
> - // sub-expression and return that one.
> - if (!eval_quickly(expr, expr_len, offset))
> - {
> - bool is_tls_address = false;
> - if (!eval_last_constant_dwarf_sub_expr(expr, expr_len,
> - offset, is_tls_address,
> - ctxt.dwarf_expr_eval_ctxt()))
> + // First try to read DW_AT_data_member_location as a plain constant.
> + // We do this because the generic method using die_location_expr
> + // might hit a bug in elfutils libdw dwarf_location_expression only
> + // fixed in elfutils 0.184+. The bug only triggers if the attribute
> + // is expressed as a (DWARF 5) DW_FORM_implicit_constant. But we
> + // handle all constants here because that is more consistent (and
> + // slightly faster in the general case where the attribute isn't a
> + // full DWARF expression).
> + if (!die_constant_data_member_location(die, offset))
> + {
> + // Otherwise, let's see if the DW_AT_data_member_location
> + // attribute and, optionally, the DW_AT_bit_offset attributes
> + // are present.
> + if (!die_location_expr(die, DW_AT_data_member_location,
> + &expr, &expr_len))
> return false;
> +
> + // The DW_AT_data_member_location attribute is present. Let's
> + // evaluate it and get its constant sub-expression and return
> + // that one.
> + if (!eval_quickly(expr, expr_len, offset))
> + {
> + bool is_tls_address = false;
> + if (!eval_last_constant_dwarf_sub_expr(expr, expr_len,
> + offset, is_tls_address,
> + ctxt.dwarf_expr_eval_ctxt()))
> + return false;
> + }
> }
> offset *= 8;
>
> --
> 2.18.4
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
^ permalink raw reply [flat|nested] 7+ messages in thread