* [PATCH] Fix size calculations for multidimensional arrays.
@ 2020-03-26 16:24 Giuliano Procida
2020-03-26 16:49 ` [PATCH v2] " Giuliano Procida
2020-03-28 19:53 ` [PATCH v2] " Giuliano Procida
0 siblings, 2 replies; 7+ messages in thread
From: Giuliano Procida @ 2020-03-26 16:24 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich
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 (append_subrange): Remove this function.
* src/abg-ir.cc (append_subrange): Inline this function into
its only caller append_subranges and remove it.
(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.
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
include/abg-ir.h | 3 -
src/abg-ir.cc | 27 +++----
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, 74 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<subrange_sptr>& subs);
diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index a10b0bb7..c2379153 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -14565,29 +14565,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<subrange_sptr>& subs)
{
+ size_t s = get_element_type()->get_size_in_bits();
+
for (std::vector<shared_ptr<subrange_type> >::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"
+
// <headers defining libabigail's API go under here>
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<std::string> elems;
- std::stringstream aliases(alias_id);
- std::string item;
- while (std::getline(aliases, item, ','))
- elems.push_back(item);
- for (std::vector<string>::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<std::string> elems;
+ std::stringstream aliases(alias_id);
+ std::string item;
+ while (std::getline(aliases, item, ','))
+ elems.push_back(item);
+ for (std::vector<string>::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<array_type_def::subrange_sptr>::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 @@
</array-type-def>
<!-- REAL[40][4] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='1408' id='type-id-156'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='5120' id='type-id-156'>
<!-- <anonymous range>[40] -->
<subrange length='40' type-id='type-id-32' id='type-id-126'/>
@@ -9621,7 +9621,7 @@
</abi-instr>
<abi-instr version='1.0' address-size='64' path='libnurbs/internals/backend.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-T0fsxk/Mesa-8.0.5/src/glu/sgi' language='LANG_C_plus_plus'>
<!-- REAL[3][4] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-292'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='384' id='type-id-292'>
<!-- <anonymous range>[3] -->
<subrange length='3' type-id='type-id-32' id='type-id-35'/>
@@ -10572,7 +10572,7 @@
</array-type-def>
<!-- REAL[5][5] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-321'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='800' id='type-id-321'>
<!-- <anonymous range>[5] -->
<subrange length='5' type-id='type-id-32' id='type-id-320'/>
@@ -13394,7 +13394,7 @@
</array-type-def>
<!-- REAL[2][5] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-379'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-379'>
<!-- <anonymous range>[2] -->
<subrange length='2' type-id='type-id-32' id='type-id-191'/>
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 @@
</array-type-def>
<!-- hb_codepoint_t[2][5] -->
- <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='224' id='type-id-65'>
+ <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='320' id='type-id-65'>
<!-- <anonymous range>[2] -->
<subrange length='2' type-id='type-id-4' id='type-id-8'/>
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 @@
</array-type-def>
<!-- void*[20][32] -->
- <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='3328' id='type-id-434'>
+ <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='40960' id='type-id-434'>
<!-- <anonymous range>[20] -->
<subrange length='20' type-id='type-id-3' id='type-id-10'/>
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 @@
<!-- double -->
<type-decl name='double' size-in-bits='64' id='type-id-10'/>
<!-- double[5][3] -->
- <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='512' id='type-id-11'>
+ <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='960' id='type-id-11'>
<!-- <anonymous range>[5] -->
<subrange length='5' type-id='type-id-4' id='type-id-12'/>
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 @@
</data-member>
</class-decl>
<type-decl name='double' size-in-bits='64' alignment-in-bits='64' id='type-id-11'/>
- <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='512' alignment-in-bits='64' id='type-id-9'>
+ <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='960' alignment-in-bits='64' id='type-id-9'>
<subrange length='5' id='type-id-12'/>
<subrange length='3' id='type-id-13'/>
--
2.25.1.696.g5e7596f4ac-goog
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] Fix size calculations for multidimensional arrays.
2020-03-26 16:24 [PATCH] Fix size calculations for multidimensional arrays Giuliano Procida
@ 2020-03-26 16:49 ` Giuliano Procida
2020-03-26 16:59 ` [PATCH v3] " Giuliano Procida
2020-03-28 19:53 ` [PATCH v2] " Giuliano Procida
1 sibling, 1 reply; 7+ messages in thread
From: Giuliano Procida @ 2020-03-26 16:49 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida
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 <gprocida@google.com>
---
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<subrange_sptr>& 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<subrange_sptr>& subs)
{
+ size_t s = get_element_type()->get_size_in_bits();
+
for (std::vector<shared_ptr<subrange_type> >::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"
+
// <headers defining libabigail's API go under here>
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<std::string> elems;
- std::stringstream aliases(alias_id);
- std::string item;
- while (std::getline(aliases, item, ','))
- elems.push_back(item);
- for (std::vector<string>::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<std::string> elems;
+ std::stringstream aliases(alias_id);
+ std::string item;
+ while (std::getline(aliases, item, ','))
+ elems.push_back(item);
+ for (std::vector<string>::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<array_type_def::subrange_sptr>::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 @@
</array-type-def>
<!-- REAL[40][4] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='1408' id='type-id-156'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='5120' id='type-id-156'>
<!-- <anonymous range>[40] -->
<subrange length='40' type-id='type-id-32' id='type-id-126'/>
@@ -9621,7 +9621,7 @@
</abi-instr>
<abi-instr version='1.0' address-size='64' path='libnurbs/internals/backend.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-T0fsxk/Mesa-8.0.5/src/glu/sgi' language='LANG_C_plus_plus'>
<!-- REAL[3][4] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-292'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='384' id='type-id-292'>
<!-- <anonymous range>[3] -->
<subrange length='3' type-id='type-id-32' id='type-id-35'/>
@@ -10572,7 +10572,7 @@
</array-type-def>
<!-- REAL[5][5] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-321'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='800' id='type-id-321'>
<!-- <anonymous range>[5] -->
<subrange length='5' type-id='type-id-32' id='type-id-320'/>
@@ -13394,7 +13394,7 @@
</array-type-def>
<!-- REAL[2][5] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-379'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-379'>
<!-- <anonymous range>[2] -->
<subrange length='2' type-id='type-id-32' id='type-id-191'/>
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 @@
</array-type-def>
<!-- hb_codepoint_t[2][5] -->
- <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='224' id='type-id-65'>
+ <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='320' id='type-id-65'>
<!-- <anonymous range>[2] -->
<subrange length='2' type-id='type-id-4' id='type-id-8'/>
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 @@
</array-type-def>
<!-- void*[20][32] -->
- <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='3328' id='type-id-434'>
+ <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='40960' id='type-id-434'>
<!-- <anonymous range>[20] -->
<subrange length='20' type-id='type-id-3' id='type-id-10'/>
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 @@
<!-- double -->
<type-decl name='double' size-in-bits='64' id='type-id-10'/>
<!-- double[5][3] -->
- <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='512' id='type-id-11'>
+ <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='960' id='type-id-11'>
<!-- <anonymous range>[5] -->
<subrange length='5' type-id='type-id-4' id='type-id-12'/>
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 @@
</data-member>
</class-decl>
<type-decl name='double' size-in-bits='64' alignment-in-bits='64' id='type-id-11'/>
- <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='512' alignment-in-bits='64' id='type-id-9'>
+ <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='960' alignment-in-bits='64' id='type-id-9'>
<subrange length='5' id='type-id-12'/>
<subrange length='3' id='type-id-13'/>
--
2.25.1.696.g5e7596f4ac-goog
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3] Fix size calculations for multidimensional arrays.
2020-03-26 16:49 ` [PATCH v2] " Giuliano Procida
@ 2020-03-26 16:59 ` Giuliano Procida
2020-03-28 13:38 ` Matthias Maennich
0 siblings, 1 reply; 7+ messages in thread
From: Giuliano Procida @ 2020-03-26 16:59 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich
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.
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
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<subrange_sptr>& 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<subrange_sptr>& subs)
{
+ size_t s = get_element_type()->get_size_in_bits();
+
for (std::vector<shared_ptr<subrange_type> >::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"
+
// <headers defining libabigail's API go under here>
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<std::string> elems;
- std::stringstream aliases(alias_id);
- std::string item;
- while (std::getline(aliases, item, ','))
- elems.push_back(item);
- for (std::vector<string>::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<std::string> elems;
+ std::stringstream aliases(alias_id);
+ std::string item;
+ while (std::getline(aliases, item, ','))
+ elems.push_back(item);
+ for (std::vector<string>::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<array_type_def::subrange_sptr>::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 @@
</array-type-def>
<!-- REAL[40][4] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='1408' id='type-id-156'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='5120' id='type-id-156'>
<!-- <anonymous range>[40] -->
<subrange length='40' type-id='type-id-32' id='type-id-126'/>
@@ -9621,7 +9621,7 @@
</abi-instr>
<abi-instr version='1.0' address-size='64' path='libnurbs/internals/backend.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-T0fsxk/Mesa-8.0.5/src/glu/sgi' language='LANG_C_plus_plus'>
<!-- REAL[3][4] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-292'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='384' id='type-id-292'>
<!-- <anonymous range>[3] -->
<subrange length='3' type-id='type-id-32' id='type-id-35'/>
@@ -10572,7 +10572,7 @@
</array-type-def>
<!-- REAL[5][5] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-321'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='800' id='type-id-321'>
<!-- <anonymous range>[5] -->
<subrange length='5' type-id='type-id-32' id='type-id-320'/>
@@ -13394,7 +13394,7 @@
</array-type-def>
<!-- REAL[2][5] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-379'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-379'>
<!-- <anonymous range>[2] -->
<subrange length='2' type-id='type-id-32' id='type-id-191'/>
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 @@
</array-type-def>
<!-- hb_codepoint_t[2][5] -->
- <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='224' id='type-id-65'>
+ <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='320' id='type-id-65'>
<!-- <anonymous range>[2] -->
<subrange length='2' type-id='type-id-4' id='type-id-8'/>
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 @@
</array-type-def>
<!-- void*[20][32] -->
- <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='3328' id='type-id-434'>
+ <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='40960' id='type-id-434'>
<!-- <anonymous range>[20] -->
<subrange length='20' type-id='type-id-3' id='type-id-10'/>
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 @@
<!-- double -->
<type-decl name='double' size-in-bits='64' id='type-id-10'/>
<!-- double[5][3] -->
- <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='512' id='type-id-11'>
+ <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='960' id='type-id-11'>
<!-- <anonymous range>[5] -->
<subrange length='5' type-id='type-id-4' id='type-id-12'/>
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 @@
</data-member>
</class-decl>
<type-decl name='double' size-in-bits='64' alignment-in-bits='64' id='type-id-11'/>
- <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='512' alignment-in-bits='64' id='type-id-9'>
+ <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='960' alignment-in-bits='64' id='type-id-9'>
<subrange length='5' id='type-id-12'/>
<subrange length='3' id='type-id-13'/>
--
2.25.1.696.g5e7596f4ac-goog
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] Fix size calculations for multidimensional arrays.
2020-03-26 16:59 ` [PATCH v3] " Giuliano Procida
@ 2020-03-28 13:38 ` Matthias Maennich
2020-03-28 19:49 ` Giuliano Procida
0 siblings, 1 reply; 7+ messages in thread
From: Matthias Maennich @ 2020-03-28 13:38 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, dodji, kernel-team
On Thu, Mar 26, 2020 at 04:59:27PM +0000, Giuliano Procida wrote:
>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.
>
>Signed-off-by: Giuliano Procida <gprocida@google.com>
>---
> 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<subrange_sptr>& 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<subrange_sptr>& subs)
> {
>+ size_t s = get_element_type()->get_size_in_bits();
>+
> for (std::vector<shared_ptr<subrange_type> >::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_name(env->intern(get_pretty_representation()));
>+ 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"
>+
> // <headers defining libabigail's API go under here>
> 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<std::string> elems;
>- std::stringstream aliases(alias_id);
>- std::string item;
>- while (std::getline(aliases, item, ','))
>- elems.push_back(item);
>- for (std::vector<string>::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<std::string> elems;
>+ std::stringstream aliases(alias_id);
>+ std::string item;
>+ while (std::getline(aliases, item, ','))
>+ elems.push_back(item);
>+ for (std::vector<string>::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);
>+ }
This is a whitespace only commit and needs to probably go into a
separate change.
> }
> }
>
>@@ -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;
>+ }
As above
> 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<array_type_def::subrange_sptr>::const_iterator i = subranges.begin();
Line length.
>+ 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;
Should we provide more data for this finding? Like what was the
discrepancy?
>+ reported = true;
>+ }
>+ }
>+ else
Emit an error message before dying.
With the above addressed, please feel free to add
Reviewed-by: Matthias Maennich <maennich@google.com>
Cheers,
Matthias
>+ 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 @@
>
> </array-type-def>
> <!-- REAL[40][4] -->
>- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='1408' id='type-id-156'>
>+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='5120' id='type-id-156'>
> <!-- <anonymous range>[40] -->
> <subrange length='40' type-id='type-id-32' id='type-id-126'/>
>
>@@ -9621,7 +9621,7 @@
> </abi-instr>
> <abi-instr version='1.0' address-size='64' path='libnurbs/internals/backend.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-T0fsxk/Mesa-8.0.5/src/glu/sgi' language='LANG_C_plus_plus'>
> <!-- REAL[3][4] -->
>- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-292'>
>+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='384' id='type-id-292'>
> <!-- <anonymous range>[3] -->
> <subrange length='3' type-id='type-id-32' id='type-id-35'/>
>
>@@ -10572,7 +10572,7 @@
>
> </array-type-def>
> <!-- REAL[5][5] -->
>- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-321'>
>+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='800' id='type-id-321'>
> <!-- <anonymous range>[5] -->
> <subrange length='5' type-id='type-id-32' id='type-id-320'/>
>
>@@ -13394,7 +13394,7 @@
>
> </array-type-def>
> <!-- REAL[2][5] -->
>- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-379'>
>+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-379'>
> <!-- <anonymous range>[2] -->
> <subrange length='2' type-id='type-id-32' id='type-id-191'/>
>
>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 @@
>
> </array-type-def>
> <!-- hb_codepoint_t[2][5] -->
>- <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='224' id='type-id-65'>
>+ <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='320' id='type-id-65'>
> <!-- <anonymous range>[2] -->
> <subrange length='2' type-id='type-id-4' id='type-id-8'/>
>
>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 @@
>
> </array-type-def>
> <!-- void*[20][32] -->
>- <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='3328' id='type-id-434'>
>+ <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='40960' id='type-id-434'>
> <!-- <anonymous range>[20] -->
> <subrange length='20' type-id='type-id-3' id='type-id-10'/>
>
>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 @@
> <!-- double -->
> <type-decl name='double' size-in-bits='64' id='type-id-10'/>
> <!-- double[5][3] -->
>- <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='512' id='type-id-11'>
>+ <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='960' id='type-id-11'>
> <!-- <anonymous range>[5] -->
> <subrange length='5' type-id='type-id-4' id='type-id-12'/>
>
>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 @@
> </data-member>
> </class-decl>
> <type-decl name='double' size-in-bits='64' alignment-in-bits='64' id='type-id-11'/>
>- <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='512' alignment-in-bits='64' id='type-id-9'>
>+ <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='960' alignment-in-bits='64' id='type-id-9'>
> <subrange length='5' id='type-id-12'/>
>
> <subrange length='3' id='type-id-13'/>
>--
>2.25.1.696.g5e7596f4ac-goog
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] Fix size calculations for multidimensional arrays.
2020-03-28 13:38 ` Matthias Maennich
@ 2020-03-28 19:49 ` Giuliano Procida
0 siblings, 0 replies; 7+ messages in thread
From: Giuliano Procida @ 2020-03-28 19:49 UTC (permalink / raw)
To: Matthias Maennich; +Cc: libabigail, Dodji Seketeli, kernel-team
Hi.
On Sat, 28 Mar 2020 at 13:38, Matthias Maennich <maennich@google.com> wrote:
>
> On Thu, Mar 26, 2020 at 04:59:27PM +0000, Giuliano Procida wrote:
> >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.
> >
> >Signed-off-by: Giuliano Procida <gprocida@google.com>
> >---
> > 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<subrange_sptr>& 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<subrange_sptr>& subs)
> > {
> >+ size_t s = get_element_type()->get_size_in_bits();
> >+
> > for (std::vector<shared_ptr<subrange_type> >::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_name(env->intern(get_pretty_representation()));
Done.
> >+ 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"
> >+
> > // <headers defining libabigail's API go under here>
> > 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<std::string> elems;
> >- std::stringstream aliases(alias_id);
> >- std::string item;
> >- while (std::getline(aliases, item, ','))
> >- elems.push_back(item);
> >- for (std::vector<string>::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<std::string> elems;
> >+ std::stringstream aliases(alias_id);
> >+ std::string item;
> >+ while (std::getline(aliases, item, ','))
> >+ elems.push_back(item);
> >+ for (std::vector<string>::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);
> >+ }
>
> This is a whitespace only commit and needs to probably go into a
> separate change.
Agreed. I've split it out.
> > }
> > }
> >
> >@@ -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;
> >+ }
>
> As above
Same.
> > 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<array_type_def::subrange_sptr>::const_iterator i = subranges.begin();
>
> Line length.
Done.
> >+ 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;
>
> Should we provide more data for this finding? Like what was the
> discrepancy?
I don't think describing the details of the problem is helpful. I've
reworded this.
> >+ reported = true;
> >+ }
> >+ }
> >+ else
>
> Emit an error message before dying.
Done.
> With the above addressed, please feel free to add
> Reviewed-by: Matthias Maennich <maennich@google.com>
Thanks. Patch v2 on the way.
Giuliano.
> Cheers,
> Matthias
>
> >+ 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 @@
> >
> > </array-type-def>
> > <!-- REAL[40][4] -->
> >- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='1408' id='type-id-156'>
> >+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='5120' id='type-id-156'>
> > <!-- <anonymous range>[40] -->
> > <subrange length='40' type-id='type-id-32' id='type-id-126'/>
> >
> >@@ -9621,7 +9621,7 @@
> > </abi-instr>
> > <abi-instr version='1.0' address-size='64' path='libnurbs/internals/backend.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-T0fsxk/Mesa-8.0.5/src/glu/sgi' language='LANG_C_plus_plus'>
> > <!-- REAL[3][4] -->
> >- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-292'>
> >+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='384' id='type-id-292'>
> > <!-- <anonymous range>[3] -->
> > <subrange length='3' type-id='type-id-32' id='type-id-35'/>
> >
> >@@ -10572,7 +10572,7 @@
> >
> > </array-type-def>
> > <!-- REAL[5][5] -->
> >- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-321'>
> >+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='800' id='type-id-321'>
> > <!-- <anonymous range>[5] -->
> > <subrange length='5' type-id='type-id-32' id='type-id-320'/>
> >
> >@@ -13394,7 +13394,7 @@
> >
> > </array-type-def>
> > <!-- REAL[2][5] -->
> >- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-379'>
> >+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-379'>
> > <!-- <anonymous range>[2] -->
> > <subrange length='2' type-id='type-id-32' id='type-id-191'/>
> >
> >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 @@
> >
> > </array-type-def>
> > <!-- hb_codepoint_t[2][5] -->
> >- <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='224' id='type-id-65'>
> >+ <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='320' id='type-id-65'>
> > <!-- <anonymous range>[2] -->
> > <subrange length='2' type-id='type-id-4' id='type-id-8'/>
> >
> >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 @@
> >
> > </array-type-def>
> > <!-- void*[20][32] -->
> >- <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='3328' id='type-id-434'>
> >+ <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='40960' id='type-id-434'>
> > <!-- <anonymous range>[20] -->
> > <subrange length='20' type-id='type-id-3' id='type-id-10'/>
> >
> >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 @@
> > <!-- double -->
> > <type-decl name='double' size-in-bits='64' id='type-id-10'/>
> > <!-- double[5][3] -->
> >- <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='512' id='type-id-11'>
> >+ <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='960' id='type-id-11'>
> > <!-- <anonymous range>[5] -->
> > <subrange length='5' type-id='type-id-4' id='type-id-12'/>
> >
> >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 @@
> > </data-member>
> > </class-decl>
> > <type-decl name='double' size-in-bits='64' alignment-in-bits='64' id='type-id-11'/>
> >- <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='512' alignment-in-bits='64' id='type-id-9'>
> >+ <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='960' alignment-in-bits='64' id='type-id-9'>
> > <subrange length='5' id='type-id-12'/>
> >
> > <subrange length='3' id='type-id-13'/>
> >--
> >2.25.1.696.g5e7596f4ac-goog
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] Fix size calculations for multidimensional arrays.
2020-03-26 16:24 [PATCH] Fix size calculations for multidimensional arrays Giuliano Procida
2020-03-26 16:49 ` [PATCH v2] " Giuliano Procida
@ 2020-03-28 19:53 ` Giuliano Procida
2020-03-28 20:09 ` Giuliano Procida
1 sibling, 1 reply; 7+ messages in thread
From: Giuliano Procida @ 2020-03-28 19:53 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich
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_array_type_def): When checking
calculated against read array sizes, warn once if value
matches old behaviour rather than raising an assertion.
Otherwise, before raising an assertion, emit an informative
error message.
* 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.
Reviewed-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
include/abg-ir.h | 3 --
src/abg-ir.cc | 30 ++++++-------
src/abg-reader.cc | 43 ++++++++++++++++---
.../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, 62 insertions(+), 36 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<subrange_sptr>& subs);
diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index a10b0bb7..88a159c5 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,25 @@ 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<subrange_sptr>& subs)
{
+ size_t s = get_element_type()->get_size_in_bits();
+
for (std::vector<shared_ptr<subrange_type> >::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);
+ set_name(env->intern(get_pretty_representation()));
+ 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..019c0e99 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"
+
// <headers defining libabigail's API go under here>
ABG_BEGIN_EXPORT_DECLARATIONS
@@ -4075,13 +4077,40 @@ build_array_type_def(read_context& ctxt,
!= ar_type->get_element_type()->get_alignment_in_bits()))
return nil;
- if (has_size_in_bits)
- if (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);
- }
+ if (has_size_in_bits && size_in_bits != (size_t) -1
+ && size_in_bits != ar_type->get_size_in_bits())
+ {
+ // 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<array_type_def::subrange_sptr>::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 << "notice: Found incorrectly calculated array "
+ << "sizes in XML - this is benign.\nOlder versions "
+ << "of libabigail miscalculated multidimensional "
+ << "array sizes." << std::endl;
+ reported = true;
+ }
+ }
+ else
+ {
+ std::cerr << "error: Found incorrectly calculated array size in "
+ << "XML (id=\"" << id << "\")." << std::endl;
+ 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 @@
</array-type-def>
<!-- REAL[40][4] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='1408' id='type-id-156'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='5120' id='type-id-156'>
<!-- <anonymous range>[40] -->
<subrange length='40' type-id='type-id-32' id='type-id-126'/>
@@ -9621,7 +9621,7 @@
</abi-instr>
<abi-instr version='1.0' address-size='64' path='libnurbs/internals/backend.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-T0fsxk/Mesa-8.0.5/src/glu/sgi' language='LANG_C_plus_plus'>
<!-- REAL[3][4] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-292'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='384' id='type-id-292'>
<!-- <anonymous range>[3] -->
<subrange length='3' type-id='type-id-32' id='type-id-35'/>
@@ -10572,7 +10572,7 @@
</array-type-def>
<!-- REAL[5][5] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-321'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='800' id='type-id-321'>
<!-- <anonymous range>[5] -->
<subrange length='5' type-id='type-id-32' id='type-id-320'/>
@@ -13394,7 +13394,7 @@
</array-type-def>
<!-- REAL[2][5] -->
- <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-379'>
+ <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-379'>
<!-- <anonymous range>[2] -->
<subrange length='2' type-id='type-id-32' id='type-id-191'/>
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 @@
</array-type-def>
<!-- hb_codepoint_t[2][5] -->
- <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='224' id='type-id-65'>
+ <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='320' id='type-id-65'>
<!-- <anonymous range>[2] -->
<subrange length='2' type-id='type-id-4' id='type-id-8'/>
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 @@
</array-type-def>
<!-- void*[20][32] -->
- <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='3328' id='type-id-434'>
+ <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='40960' id='type-id-434'>
<!-- <anonymous range>[20] -->
<subrange length='20' type-id='type-id-3' id='type-id-10'/>
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 @@
<!-- double -->
<type-decl name='double' size-in-bits='64' id='type-id-10'/>
<!-- double[5][3] -->
- <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='512' id='type-id-11'>
+ <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='960' id='type-id-11'>
<!-- <anonymous range>[5] -->
<subrange length='5' type-id='type-id-4' id='type-id-12'/>
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 @@
</data-member>
</class-decl>
<type-decl name='double' size-in-bits='64' alignment-in-bits='64' id='type-id-11'/>
- <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='512' alignment-in-bits='64' id='type-id-9'>
+ <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='960' alignment-in-bits='64' id='type-id-9'>
<subrange length='5' id='type-id-12'/>
<subrange length='3' id='type-id-13'/>
--
2.26.0.rc2.310.g2932bb562d-goog
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Fix size calculations for multidimensional arrays.
2020-03-28 19:53 ` [PATCH v2] " Giuliano Procida
@ 2020-03-28 20:09 ` Giuliano Procida
0 siblings, 0 replies; 7+ messages in thread
From: Giuliano Procida @ 2020-03-28 20:09 UTC (permalink / raw)
To: libabigail; +Cc: Dodji Seketeli, kernel-team, Matthias Männich
Bad subject line. This is v4. Sorry!
On Sat, 28 Mar 2020 at 19:53, Giuliano Procida <gprocida@google.com> wrote:
>
> 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_array_type_def): When checking
> calculated against read array sizes, warn once if value
> matches old behaviour rather than raising an assertion.
> Otherwise, before raising an assertion, emit an informative
> error message.
> * 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.
>
> Reviewed-by: Matthias Maennich <maennich@google.com>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---
> include/abg-ir.h | 3 --
> src/abg-ir.cc | 30 ++++++-------
> src/abg-reader.cc | 43 ++++++++++++++++---
> .../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, 62 insertions(+), 36 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<subrange_sptr>& subs);
>
> diff --git a/src/abg-ir.cc b/src/abg-ir.cc
> index a10b0bb7..88a159c5 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,25 @@ 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<subrange_sptr>& subs)
> {
> + size_t s = get_element_type()->get_size_in_bits();
> +
> for (std::vector<shared_ptr<subrange_type> >::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);
> + set_name(env->intern(get_pretty_representation()));
> + 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..019c0e99 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"
> +
> // <headers defining libabigail's API go under here>
> ABG_BEGIN_EXPORT_DECLARATIONS
>
> @@ -4075,13 +4077,40 @@ build_array_type_def(read_context& ctxt,
> != ar_type->get_element_type()->get_alignment_in_bits()))
> return nil;
>
> - if (has_size_in_bits)
> - if (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);
> - }
> + if (has_size_in_bits && size_in_bits != (size_t) -1
> + && size_in_bits != ar_type->get_size_in_bits())
> + {
> + // 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<array_type_def::subrange_sptr>::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 << "notice: Found incorrectly calculated array "
> + << "sizes in XML - this is benign.\nOlder versions "
> + << "of libabigail miscalculated multidimensional "
> + << "array sizes." << std::endl;
> + reported = true;
> + }
> + }
> + else
> + {
> + std::cerr << "error: Found incorrectly calculated array size in "
> + << "XML (id=\"" << id << "\")." << std::endl;
> + 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 @@
>
> </array-type-def>
> <!-- REAL[40][4] -->
> - <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='1408' id='type-id-156'>
> + <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='5120' id='type-id-156'>
> <!-- <anonymous range>[40] -->
> <subrange length='40' type-id='type-id-32' id='type-id-126'/>
>
> @@ -9621,7 +9621,7 @@
> </abi-instr>
> <abi-instr version='1.0' address-size='64' path='libnurbs/internals/backend.cc' comp-dir-path='/tmp/legendre/spack-stage/spack-stage-T0fsxk/Mesa-8.0.5/src/glu/sgi' language='LANG_C_plus_plus'>
> <!-- REAL[3][4] -->
> - <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-292'>
> + <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='384' id='type-id-292'>
> <!-- <anonymous range>[3] -->
> <subrange length='3' type-id='type-id-32' id='type-id-35'/>
>
> @@ -10572,7 +10572,7 @@
>
> </array-type-def>
> <!-- REAL[5][5] -->
> - <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-321'>
> + <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='800' id='type-id-321'>
> <!-- <anonymous range>[5] -->
> <subrange length='5' type-id='type-id-32' id='type-id-320'/>
>
> @@ -13394,7 +13394,7 @@
>
> </array-type-def>
> <!-- REAL[2][5] -->
> - <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='224' id='type-id-379'>
> + <array-type-def dimensions='2' type-id='type-id-122' size-in-bits='320' id='type-id-379'>
> <!-- <anonymous range>[2] -->
> <subrange length='2' type-id='type-id-32' id='type-id-191'/>
>
> 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 @@
>
> </array-type-def>
> <!-- hb_codepoint_t[2][5] -->
> - <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='224' id='type-id-65'>
> + <array-type-def dimensions='2' type-id='type-id-64' size-in-bits='320' id='type-id-65'>
> <!-- <anonymous range>[2] -->
> <subrange length='2' type-id='type-id-4' id='type-id-8'/>
>
> 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 @@
>
> </array-type-def>
> <!-- void*[20][32] -->
> - <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='3328' id='type-id-434'>
> + <array-type-def dimensions='2' type-id='type-id-17' size-in-bits='40960' id='type-id-434'>
> <!-- <anonymous range>[20] -->
> <subrange length='20' type-id='type-id-3' id='type-id-10'/>
>
> 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 @@
> <!-- double -->
> <type-decl name='double' size-in-bits='64' id='type-id-10'/>
> <!-- double[5][3] -->
> - <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='512' id='type-id-11'>
> + <array-type-def dimensions='2' type-id='type-id-10' size-in-bits='960' id='type-id-11'>
> <!-- <anonymous range>[5] -->
> <subrange length='5' type-id='type-id-4' id='type-id-12'/>
>
> 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 @@
> </data-member>
> </class-decl>
> <type-decl name='double' size-in-bits='64' alignment-in-bits='64' id='type-id-11'/>
> - <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='512' alignment-in-bits='64' id='type-id-9'>
> + <array-type-def dimensions='2' type-id='type-id-11' size-in-bits='960' alignment-in-bits='64' id='type-id-9'>
> <subrange length='5' id='type-id-12'/>
>
> <subrange length='3' id='type-id-13'/>
> --
> 2.26.0.rc2.310.g2932bb562d-goog
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-28 20:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26 16:24 [PATCH] Fix size calculations for multidimensional arrays Giuliano Procida
2020-03-26 16:49 ` [PATCH v2] " Giuliano Procida
2020-03-26 16:59 ` [PATCH v3] " Giuliano Procida
2020-03-28 13:38 ` Matthias Maennich
2020-03-28 19:49 ` Giuliano Procida
2020-03-28 19:53 ` [PATCH v2] " Giuliano Procida
2020-03-28 20:09 ` Giuliano Procida
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).