From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 4CC4C385800C for ; Sat, 10 Jul 2021 09:18:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4CC4C385800C Received: from mail-ua1-f71.google.com (mail-ua1-f71.google.com [209.85.222.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-131-gASKPTCLObeOVNc3uBxUVw-1; Sat, 10 Jul 2021 05:18:48 -0400 X-MC-Unique: gASKPTCLObeOVNc3uBxUVw-1 Received: by mail-ua1-f71.google.com with SMTP id 76-20020ab003d20000b029029db3c53817so4999558uau.22 for ; Sat, 10 Jul 2021 02:18:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=4LN+sK/edAQTn0YxlZTekn8aDTYRtP4vBo9GFu5x/so=; b=muJ6NY8WeqkMjWBnfmmvZNpOVD95N0jwtDJLOh/n9zJIzmU2yB78YyCpI8FOM7nEQj 0P4L6YJ5Q5CnWupfxBNxCC89+F/b0VbiUtwMVpwlJOWo0scoOLfh0ZbKNokWcsjbiFGw trHxWtTAoBPPosmU9FzpCIzoT5EOh+O7hXoTpn9bKqMFikaSJ62TL+LCzncXIwMyajFG ylcwKJdbM3f3PebnIkRPZaUQnfFW5LWN+nJR9W/SHdV6o9fmVC2beXORILFe6Dc8d7FU AEdSPIiGY48yrDmOA1j6rMRecNyuBpDY12RKNVOTudwX39nMAE2rPwoLNidLcM4mDbih RzlA== X-Gm-Message-State: AOAM531tHvhkGQW/aCrVWm929+qwWTs16Ubr+D8B8RLNojThQvak3Iic 7wjd/3k4RpR/zQfhC4j1hSEd7QjPSI1dWnM9axJVQpxKb9ilDW8EZXQRkIbCT6Yzwrxl4dlbkta QeYowXrEOidnkNePHBXynZVKvrcZ9Mo132GU0 X-Received: by 2002:ac5:c888:: with SMTP id n8mr10834735vkl.22.1625908727396; Sat, 10 Jul 2021 02:18:47 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzKe4FI0Nz/2yDWQNhJ/IIPRJSLsvlBP0NFpYMhYvI9rwTBKI+JOrBl/LDrsAHH9pCS9Qhgt8usEUdPbpvTwhI= X-Received: by 2002:ac5:c888:: with SMTP id n8mr10834725vkl.22.1625908727063; Sat, 10 Jul 2021 02:18:47 -0700 (PDT) MIME-Version: 1.0 References: <20210709231649.2969609-1-gprocida@google.com> <20210709231649.2969609-2-gprocida@google.com> In-Reply-To: <20210709231649.2969609-2-gprocida@google.com> From: David Marchand Date: Sat, 10 Jul 2021 11:18:35 +0200 Message-ID: Subject: Re: [PATCH 1/1] DWARF reader: fix bitfield offset calculations To: Giuliano Procida Cc: libabigail@sourceware.org, Dodji Seketeli , kernel-team@android.com, maennich@google.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-13.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_FILL_THIS_FORM_SHORT autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 10 Jul 2021 09:18:54 -0000 Hello, On Sat, Jul 10, 2021 at 1:17 AM Giuliano Procida 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 > --- > 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