From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <3Lt18XggKCtU7GIF39417FF7C5.3FDC92129719CJFLI35N1I5.FI7@flex--gprocida.bounces.google.com> Received: from mail-ua1-x949.google.com (mail-ua1-x949.google.com [IPv6:2607:f8b0:4864:20::949]) by sourceware.org (Postfix) with ESMTPS id 8B92C385E006 for ; Thu, 26 Mar 2020 16:49:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8B92C385E006 Received: by mail-ua1-x949.google.com with SMTP id e17so2544299uar.2 for ; Thu, 26 Mar 2020 09:49:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc; bh=hUUUfDAb8B3Q79hLQqd63XqIgrRcTA/nnnYIyT6/iRs=; b=okAMiE0FMPBkSYcontehh02elpTNjOKiJ0s/HTrV2On52gInmgVcj8uC0O4IAHIhoR +HhOi7pf5f6calBiXzf5iuYbH1puifpiRrfHKFvb+AKQ/TTRo/pI2MUcGVYPH4uvC6Mb wsseqgtF7Y7hDvYXOrhHgaRnyGhANiKtjxYeEI+f5Qb0yCvkgTGBD69At1oJF/2kr/eq aNNlk4lkQhVYhrEqLM9Q5Q/SCqoJwxs5wXOOB5ar4ztgaR9vlDgIareJ8VE3hGuVd1l2 djR93OTivAa1N+fR/eWDGQJ6h2Ci2kA4UKXl5HI05COVpaAPNJ8jwpR4OdKSOvkndbPV AxNA== X-Gm-Message-State: ANhLgQ2fXxPB3mCbXZTzXYfEfx8wCJ7nIPX+NkKdGvZWqLuGCehcM5zo 3Uw9VkWTFNpFm2ENSyJaA0N+LDved5F6YaIiJk4+8WdOtYzD3qoSzxxpX8ccc1WJVZ+kPYqF0a0 MuKoiObalMAI/YGia9sZwiN8ypLyRVhlXvSsPehSI5J3C/5DlIbCHcL3RudTa8UhFvizoMD8= X-Google-Smtp-Source: ADFU+vt2NN716BT/33gVpXpbz9PNns8uBBeJ+hlIcS2z7QJS7vyS53TLWBhuxGu7BNCCQzdleSChyBlrupwjBw== X-Received: by 2002:a67:db83:: with SMTP id f3mr6530980vsk.221.1585241390442; Thu, 26 Mar 2020 09:49:50 -0700 (PDT) Date: Thu, 26 Mar 2020 16:49:39 +0000 In-Reply-To: <20200326162411.69662-1-gprocida@google.com> Message-Id: <20200326164939.81553-1-gprocida@google.com> Mime-Version: 1.0 References: <20200326162411.69662-1-gprocida@google.com> X-Mailer: git-send-email 2.25.1.696.g5e7596f4ac-goog Subject: [PATCH v2] Fix size calculations for multidimensional arrays. From: Giuliano Procida To: libabigail@sourceware.org Cc: dodji@seketeli.org, kernel-team@android.com, gprocida@google.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-33.8 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libabigail mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 26 Mar 2020 16:49:53 -0000 The code in abg-ir.cc that calculated the memory size of an array summed, rather than multiplied, the dimensions. It also did duplicate work for each dimension after the first. Existing code in abg-reader.cc asserted that array size information read from XML match freshly calculated values. This patch corrects the calculation, eliminates the duplicate work and updates the XML reader validation to just emit a warning if old bad array size information is found. * include/abg-ir.h (array_type_def::append_subrange): Remove this function. * src/abg-ir.cc (array_type_def::set_element_type): Add a note about safe usage. (array_type_def::append_subrange): Inline this function into its only caller append_subranges and remove it. (array_type_def::append_subranges): Do correct multiplicative calculation of multidimensional array sizes. * src/abg-reader.cc: (build_elf_symbol_db): Fix code indentation. (build_array_type_def): Tabify. When checking calculated against read array sizes, warn once if value matches old behaviour rather than raising an assertion. * tests/data/test-annotate/test14-pr18893.so.abi: Correct array sizes. * tests/data/test-annotate/test17-pr19027.so.abi: Ditto. * tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi: Ditto. * tests/data/test-annotate/test7.so.abi: Ditto. * tests/data/test-diff-dwarf/test10-report.txt: Ditto. * tests/data/test-diff-dwarf/test11-report.txt: Ditto. * tests/data/test-read-write/test25.xml: Ditto. Fix size calculations for multidimensional arrays. Signed-off-by: Giuliano Procida --- include/abg-ir.h | 3 - src/abg-ir.cc | 31 +++---- src/abg-reader.cc | 80 ++++++++++++------- .../data/test-annotate/test14-pr18893.so.abi | 8 +- .../data/test-annotate/test17-pr19027.so.abi | 2 +- ...19-pr19023-libtcmalloc_and_profiler.so.abi | 2 +- tests/data/test-annotate/test7.so.abi | 2 +- tests/data/test-diff-dwarf/test10-report.txt | 2 +- tests/data/test-diff-dwarf/test11-report.txt | 4 +- tests/data/test-read-write/test25.xml | 2 +- 10 files changed, 78 insertions(+), 58 deletions(-) diff --git a/include/abg-ir.h b/include/abg-ir.h index 1278da94..fda10de5 100644 --- a/include/abg-ir.h +++ b/include/abg-ir.h @@ -2380,9 +2380,6 @@ public: void set_element_type(const type_base_sptr& element_type); - virtual void - append_subrange(subrange_sptr sub); - virtual void append_subranges(const std::vector& subs); diff --git a/src/abg-ir.cc b/src/abg-ir.cc index a10b0bb7..5576c137 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -14558,6 +14558,10 @@ array_type_def::get_element_type() const /// re-compute the canonical type of the array, if one has already /// been computed. /// +/// The intended use of this method is to permit in-place adjustment +/// of the element type's qualifiers. In particular, the size of the +/// element type should not be changed. +/// /// @param element_type the new element type to set. void array_type_def::set_element_type(const type_base_sptr& element_type) @@ -14565,29 +14569,26 @@ array_type_def::set_element_type(const type_base_sptr& element_type) priv_->element_type_ = element_type; } -// Append a single subrange @param sub. -void -array_type_def::append_subrange(subrange_sptr sub) -{ - priv_->subranges_.push_back(sub); - size_t s = get_size_in_bits(); - s += sub->get_length() * get_element_type()->get_size_in_bits(); - set_size_in_bits(s); - string r = get_pretty_representation(); - const environment* env = get_environment(); - ABG_ASSERT(env); - set_name(env->intern(r)); -} - /// Append subranges from the vector @param subs to the current /// vector of subranges. void array_type_def::append_subranges(const std::vector& subs) { + size_t s = get_element_type()->get_size_in_bits(); + for (std::vector >::const_iterator i = subs.begin(); i != subs.end(); ++i) - append_subrange(*i); + { + priv_->subranges_.push_back(*i); + s *= (*i)->get_length(); + } + + const environment* env = get_environment(); + ABG_ASSERT(env); + string r = get_pretty_representation(); + set_name(env->intern(r)); + set_size_in_bits(s); } /// @return true if one of the sub-ranges of the array is infinite, or diff --git a/src/abg-reader.cc b/src/abg-reader.cc index 0dcb2e92..31f83ca2 100644 --- a/src/abg-reader.cc +++ b/src/abg-reader.cc @@ -39,6 +39,8 @@ #include "abg-suppression-priv.h" #include "abg-internal.h" +#include "abg-tools-utils.h" + // ABG_BEGIN_EXPORT_DECLARATIONS @@ -3003,24 +3005,24 @@ build_elf_symbol_db(read_context& ctxt, { if (xml_char_sptr s = XML_NODE_GET_ATTRIBUTE(x->first, "alias")) { - string alias_id = CHAR_STR(s); - - // Symbol aliases can be multiple separated by comma(,), split them - std::vector elems; - std::stringstream aliases(alias_id); - std::string item; - while (std::getline(aliases, item, ',')) - elems.push_back(item); - for (std::vector::iterator alias = elems.begin(); - alias != elems.end(); ++alias) - { - string_elf_symbol_sptr_map_type::const_iterator i = - id_sym_map.find(*alias); - ABG_ASSERT(i != id_sym_map.end()); - ABG_ASSERT(i->second->is_main_symbol()); - - x->second->get_main_symbol()->add_alias(i->second); - } + string alias_id = CHAR_STR(s); + + // Symbol aliases can be multiple separated by comma(,), split them + std::vector elems; + std::stringstream aliases(alias_id); + std::string item; + while (std::getline(aliases, item, ',')) + elems.push_back(item); + for (std::vector::iterator alias = elems.begin(); + alias != elems.end(); ++alias) + { + string_elf_symbol_sptr_map_type::const_iterator i = + id_sym_map.find(*alias); + ABG_ASSERT(i != id_sym_map.end()); + ABG_ASSERT(i->second->is_main_symbol()); + + x->second->get_main_symbol()->add_alias(i->second); + } } } @@ -4019,12 +4021,12 @@ build_array_type_def(read_context& ctxt, { size_in_bits = strtoull(CHAR_STR(s), &endptr, 0); if (*endptr != '\0') - { - if (!strcmp(CHAR_STR(s), "infinite")) - size_in_bits = (size_t) -1; - else - return nil; - } + { + if (!strcmp(CHAR_STR(s), "infinite")) + size_in_bits = (size_t) -1; + else + return nil; + } has_size_in_bits = true; } @@ -4032,7 +4034,7 @@ build_array_type_def(read_context& ctxt, { alignment_in_bits = strtoull(CHAR_STR(s), &endptr, 0); if (*endptr != '\0') - return nil; + return nil; } string id; @@ -4076,11 +4078,31 @@ build_array_type_def(read_context& ctxt, return nil; if (has_size_in_bits) - if (size_in_bits != ar_type->get_size_in_bits()) + if (size_in_bits != (size_t) -1 + && size_in_bits != ar_type->get_size_in_bits()) { - ABG_ASSERT(size_in_bits == (size_t) -1 - || ar_type->get_element_type()->get_size_in_bits() == (size_t)-1 - || ar_type->get_element_type()->get_size_in_bits() == 0); + // We have a potential discrepancy between calculated and recorded sizes. + size_t element_size = ar_type->get_element_type()->get_size_in_bits(); + if (element_size && element_size != (size_t)-1) + { + // Older versions miscalculated multidimensional array sizes. + size_t bad_count = 0; + for (vector::const_iterator i = subranges.begin(); + i != subranges.end(); ++i) + bad_count += (*i)->get_length(); + if (size_in_bits == bad_count * element_size) + { + static bool reported = false; + if (!reported) + { + std::cerr << "warning: ignoring bad array sizes in XML" + << std::endl; + reported = true; + } + } + else + ABG_ASSERT_NOT_REACHED; + } } if (ctxt.push_and_key_type_decl(ar_type, id, add_to_current_scope)) diff --git a/tests/data/test-annotate/test14-pr18893.so.abi b/tests/data/test-annotate/test14-pr18893.so.abi index d357bfbd..c7baf6ad 100644 --- a/tests/data/test-annotate/test14-pr18893.so.abi +++ b/tests/data/test-annotate/test14-pr18893.so.abi @@ -5047,7 +5047,7 @@ - + @@ -9621,7 +9621,7 @@ - + @@ -10572,7 +10572,7 @@ - + @@ -13394,7 +13394,7 @@ - + diff --git a/tests/data/test-annotate/test17-pr19027.so.abi b/tests/data/test-annotate/test17-pr19027.so.abi index fae390c6..9214a0f4 100644 --- a/tests/data/test-annotate/test17-pr19027.so.abi +++ b/tests/data/test-annotate/test17-pr19027.so.abi @@ -1296,7 +1296,7 @@ - + diff --git a/tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi b/tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi index 64c8c395..b62486d9 100644 --- a/tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi +++ b/tests/data/test-annotate/test19-pr19023-libtcmalloc_and_profiler.so.abi @@ -38460,7 +38460,7 @@ - + diff --git a/tests/data/test-annotate/test7.so.abi b/tests/data/test-annotate/test7.so.abi index 294bac3c..11bd6a3d 100644 --- a/tests/data/test-annotate/test7.so.abi +++ b/tests/data/test-annotate/test7.so.abi @@ -34,7 +34,7 @@ - + diff --git a/tests/data/test-diff-dwarf/test10-report.txt b/tests/data/test-diff-dwarf/test10-report.txt index 96e0d46c..0f616713 100644 --- a/tests/data/test-diff-dwarf/test10-report.txt +++ b/tests/data/test-diff-dwarf/test10-report.txt @@ -10,7 +10,7 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 2 data member changes: type of 'int S::m0[5][3]' changed: type name changed from 'int[5][3]' to 'int[5][4]' - array type size changed from 256 to 288 + array type size changed from 480 to 640 array type subrange 2 changed length from 3 to 4 type of 'int* S::m1[10]' changed: array element type 'int*' changed: diff --git a/tests/data/test-diff-dwarf/test11-report.txt b/tests/data/test-diff-dwarf/test11-report.txt index 0979602f..655802f2 100644 --- a/tests/data/test-diff-dwarf/test11-report.txt +++ b/tests/data/test-diff-dwarf/test11-report.txt @@ -10,11 +10,11 @@ Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 2 data member changes: type of 'int S::m0[5][3]' changed: type name changed from 'int[5][3]' to 'int[6][3]' - array type size changed from 256 to 288 + array type size changed from 480 to 576 array type subrange 1 changed length from 5 to 6 type of 'int S::m1[6][4]' changed: type name changed from 'int[6][4]' to 'int[6][5]' - array type size changed from 320 to 352 + array type size changed from 768 to 960 array type subrange 2 changed length from 4 to 5 and offset changed from 480 to 576 (in bits) (by +96 bits) diff --git a/tests/data/test-read-write/test25.xml b/tests/data/test-read-write/test25.xml index 9be61cce..5be51f85 100644 --- a/tests/data/test-read-write/test25.xml +++ b/tests/data/test-read-write/test25.xml @@ -26,7 +26,7 @@ - + -- 2.25.1.696.g5e7596f4ac-goog