Re: [PATCH 1/1] DWARF reader: fix bitfield offset calculations

[David Marchand <david.marchand@redhat.com>]

Hello,

On Sat, Jul 10, 2021 at 1:17 AM Giuliano Procida <gprocida@google.com> wrote:
>
> PR28060 - Invalid offset for bitfields
>
> Bitfield and other member offsets can be specified in DWARF using:
>
> - DW_AT_bit_offset, or
> - DW_AT_data_member_location and optionally DW_AT_data_bit_offset.

I understood the standard (and the code below) differently:

- DW_AT_*data*_bit_offset,
- DW_AT_data_member_location associated with an optional
DW_AT_bit_offset (DW_AT_bit_offset maintained for compat with Dwarf <=
4),

Maybe this is a ENOTENOUGHCOFFEE from me.


>
> The code would only use the value DW_AT_data_member_location if there
> was no DW_AT_data_bit_offset. This commit fixes this and adjusts
> documentation and affected tests.
>
>         * src/abg-dwarf-reader.cc (read_and_convert_DW_at_bit_offset):
>         Update documentation.
>         (die_member_offset): Treat DW_AT_data_bit_offset as an
>         optional adjustment to DW_AT_data_member_location.
>         * tests/data/test-annotate/test13-pr18894.so.abi: Update.
>         * tests/data/test-annotate/test15-pr18892.so.abi: Update.
>         * tests/data/test-annotate/test17-pr19027.so.abi: Update.
>         * tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi:
>         Update.
>         * tests/data/test-annotate/test21-pr19092.so.abi: Update.
>         * tests/data/test-diff-dwarf-abixml/PR25409-librte_bus_dpaa.so.20.0.abi:
>         Regenerate.
>         * tests/data/test-diff-pkg/libcdio-0.94-1.fc26.x86_64--libcdio-0.94-2.fc26.x86_64-report.1.txt:
>         Report now empty.
>         * tests/data/test-read-dwarf/PR25007-sdhci.ko.abi: Update.
>         * tests/data/test-read-dwarf/PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi:
>         Update.
>         * tests/data/test-read-dwarf/test13-pr18894.so.abi: Update.
>         * tests/data/test-read-dwarf/test15-pr18892.so.abi: Update.
>         * tests/data/test-read-dwarf/test17-pr19027.so.abi: Update.
>         * tests/data/test-read-dwarf/test19-pr19023-libtcmalloc_and_profiler.so.abi:
>         Update.
>         * tests/data/test-read-dwarf/test21-pr19092.so.abi: Update.
>         * tests/data/test-read-dwarf/test22-pr19097-libstdc++.so.6.0.17.so.abi:
>         Update.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---
>  src/abg-dwarf-reader.cc                       |   60 +-
>  .../data/test-annotate/test13-pr18894.so.abi  |   54 +-
>  .../data/test-annotate/test15-pr18892.so.abi  |    4 +-
>  .../data/test-annotate/test17-pr19027.so.abi  |   12 +-
>  ...19-pr19023-libtcmalloc_and_profiler.so.abi |    8 +-
>  .../data/test-annotate/test21-pr19092.so.abi  |   44 +-
>  .../PR25409-librte_bus_dpaa.so.20.0.abi       | 5258 +++++++++--------
>  ...4--libcdio-0.94-2.fc26.x86_64-report.1.txt |   42 -
>  .../data/test-read-dwarf/PR25007-sdhci.ko.abi |  168 +-
>  .../PR25042-libgdbm-clang-dwarf5.so.6.0.0.abi |   24 +-
>  .../test-read-dwarf/test13-pr18894.so.abi     |   54 +-
>  .../test-read-dwarf/test15-pr18892.so.abi     |    4 +-
>  .../test-read-dwarf/test17-pr19027.so.abi     |   12 +-
>  ...19-pr19023-libtcmalloc_and_profiler.so.abi |    8 +-
>  .../test-read-dwarf/test21-pr19092.so.abi     |   44 +-
>  .../test22-pr19097-libstdc++.so.6.0.17.so.abi |   14 +-
>  16 files changed, 2886 insertions(+), 2924 deletions(-)
>
> diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
> index c8cd5170..0f016b66 100644
> --- a/src/abg-dwarf-reader.cc
> +++ b/src/abg-dwarf-reader.cc
> @@ -8434,7 +8434,8 @@ eval_last_constant_dwarf_sub_expr(Dwarf_Op*       expr,
>  // -----------------------------------
>
>  /// Convert the value of the DW_AT_bit_offset attribute into the value
> -/// of the DW_AT_data_bit_offset attribute.
> +/// of the DW_AT_data_bit_offset attribute, ignoring the byte offset
> +/// contribution of the DW_AT_data_member_location attribute.
>  ///
>  /// On big endian machines, the value of the DW_AT_bit_offset
>  /// attribute is the same as the value of the DW_AT_data_bit_offset
> @@ -8605,8 +8606,9 @@ eval_last_constant_dwarf_sub_expr(Dwarf_Op*       expr,
>  ///
>  /// @param offset this is the output parameter into which the value of
>  /// the DW_AT_bit_offset is put, converted as if it was the value of
> -/// the DW_AT_data_bit_offset parameter.  This parameter is set iff
> -/// the function returns true.
> +/// the DW_AT_data_bit_offset parameter, less the contribution of
> +/// DW_AT_data_member_location.  This parameter is set iff the

Nit: *if

> +/// function returns true.
>  ///
>  /// @return true if DW_AT_bit_offset was found on @p die.
>  static bool
> @@ -8657,10 +8659,11 @@ read_and_convert_DW_at_bit_offset(const Dwarf_Die* die,
>  /// DW_AT_data_member_location is not necessarily a constant that one
>  /// would just read and be done with it.  Rather, it can be a DWARF
>  /// expression that one has to interpret.  In general, the offset can
> -/// be given by the DW_AT_bit_offset or DW_AT_data_bit_offset
> -/// attribute.  In that case the offset is a constant.  But it can
> -/// also be given by the DW_AT_data_member_location attribute.  In
> -/// that case it's a DWARF location expression.
> +/// be given by the DW_AT_bit_offset or by the
> +/// DW_AT_data_member_location attribute and optionally the
> +/// DW_AT_data_bit_offset attribute.  The bit offset attributes are

The "x or y and optional z" is not that clear without paren.
To enhance readability, I would rephrase as:
"""
In general, the offset can be given by the DW_AT_data_bit_offset attribute
or by the DW_AT_data_member_location attribute associated with an optional
DW_AT_bit_offset attribute.
"""


> +/// always simple constants, but the DW_AT_data_member_location
> +/// attribute is a DWARF location expression.
>  ///
>  /// When the it's the DW_AT_data_member_location that is present,
>  /// there are three cases to possibly take into account:
> @@ -8704,39 +8707,24 @@ die_member_offset(const read_context& ctxt,
>  {
>    Dwarf_Op* expr = NULL;
>    uint64_t expr_len = 0;
> -  uint64_t off = 0;
> +  uint64_t bit_offset = 0;
>
>    // First let's see if the DW_AT_data_bit_offset attribute is
>    // present.
> -  if (die_unsigned_constant_attribute(die, DW_AT_data_bit_offset, off))
> -    {
> -      offset = off;
> -      return true;
> -    }
> -
> -  // Otherwise, let's see if the DW_AT_bit_offset attribute is
> -  // present.  On little endian machines, we need to convert this
> -  // attribute into what it would have been if the
> -  // DW_AT_data_bit_offset was used instead.  In other words,
> -  // DW_AT_bit_offset needs to be converted into a
> -  // human-understandable form that represents the offset of the
> -  // bitfield data member it describes.  For details about the
> -  // conversion, please read the extensive comments of
> -  // read_and_convert_DW_at_bit_offset.
> -  bool is_big_endian = architecture_is_big_endian(ctxt.elf_handle());
> -  if (read_and_convert_DW_at_bit_offset(die, is_big_endian, off))
> +  if (die_unsigned_constant_attribute(die, DW_AT_data_bit_offset, bit_offset))
>      {
> -      offset = off;
> +      offset = bit_offset;
>        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;
>
> -  // Otherwise, the DW_AT_data_member_location attribute is present.
> -  // In that case, let's evaluate it and get its constant
> +  // 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;
> @@ -8745,8 +8733,20 @@ die_member_offset(const read_context& ctxt,
>                                              ctxt.dwarf_expr_eval_ctxt()))
>         return false;
>      }
> -
>    offset *= 8;
> +
> +  // On little endian machines, we need to convert the DW_AT_bit_offset
> +  // attribute into what it would have been if the DW_AT_data_bit_offset were
> +  // used instead.

Again, maybe my lack of coffee, correct me if I am wrong.

I understood DW_AT_data_bit_offset specify an "absolute" offset while
DW_AT_bit_offset specify an offset "relative" to
DW_AT_data_member_location.
If this is the case, I find this comment misleading and I would remove it.
Else, well, disregard this comment :-).


> +  //
> +  // In other words, DW_AT_bit_offset needs to be converted into a
> +  // human-understandable form that represents the offset of the bitfield data
> +  // member it describes.  For details about the conversion, please read the
> +  // extensive comments of read_and_convert_DW_at_bit_offset.
> +  bool is_big_endian = architecture_is_big_endian(ctxt.elf_handle());
> +  if (read_and_convert_DW_at_bit_offset(die, is_big_endian, bit_offset))
> +    offset += bit_offset;
> +
>    return true;
>  }
>


- I have tested this patch against DPDK.
I have dwarf4 blobs.
Before the patch:
          type of 'rte_eth_dev_data* data' changed:
            in pointed to type 'struct rte_eth_dev_data' at
rte_ethdev_core.h:131:1:
              type size hasn't changed
              1 data member insertion:
                'uint8_t dev_configured', at offset 5 (in bits) at
rte_ethdev_core.h:171:1
After the patch:
          type of 'rte_eth_dev_data* data' changed:
            in pointed to type 'struct rte_eth_dev_data' at
rte_ethdev_core.h:131:1:
              type size hasn't changed
              1 data member insertion:
                'uint8_t dev_configured', at offset 34005 (in bits) at
rte_ethdev_core.h:171:1

Which seems all good on this side.


But now, I face an issue trying to waive this.
The addition of the "dev_configured" field in this bitfield is right
after a 1 bit wide "lro" field and before a "rx_queue_state" field.
https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev_core.h?id=f0b97fc4fe4cd16dd492cf30ece4296451e10e10#n165

The rule below does not work:
[suppress_type]
        name = rte_eth_dev_data
        has_data_member_inserted_between = {offset_after(lro),
offset_of(rx_queue_state)}

While on the other hand, this one works:
[suppress_type]
        name = rte_eth_dev_data
        has_data_member_inserted_between = {34005, offset_of(rx_queue_state)}

Not sure what the issue is, but supposing the offsets are now correct,
then my bet is on the "lro" field size (here, 1 bit).


Back to my reproducer, that I updated:

struct bigstruct {
    char name[128];
    uint8_t bitfield0:1
#ifndef BEFORE
            ,bitfield1:1
#endif
    ;
    uint8_t other;
};

I wrote a suppression rule:
[suppress_type]
    name = bigstruct
    has_data_member_inserted_between = {offset_after(bitfield0),
offset_of(other)}

And then I tried to debug, but I am lost in libabigail internals.
I attached a gdb to catch all get_size_in_bits() calls, I see either
1024 (sizeof name), 8 (sizeof other?), 0 (this is suspicious) or 64
(?).


-- 
David Marchand