dwarf-reader: Workaround libdw dwarf_location_expression bug

  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
  

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
>
>
  

Hui Giuliano,

On Wed, Dec 15, 2021 at 10:56:35PM +0400, Giuliano Procida via Libabigail wrote:
> What's the relationship, if any, between this and the patch I posted on 24
> Nov to address Clang DWARF 5 addresses?

They are separate issues. Your patch deals with die_location_address,
this patch deals with die_member_offset.

Cheers,

Mark
  

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
  

Hello Mark,

"Mark J. Wielaard" <mark@klomp.org> a écrit:

> 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>

Applied to master, thanks!

[...]

Cheers,