[PATCH 0/3] Improve resolution of declaration-only enums

[PATCH 0/3] Improve resolution of declaration-only enums

[Giuliano Procida]

Hi Dodji.

We noticed this sort of output in Android kernel ABI XML:

    <enum-decl name='dax_access_mode' is-declaration-only='yes' id='241913bb'>
      <underlying-type type-id='52ba91e1'/>
    </enum-decl>
    ...
    <enum-decl name='dax_access_mode' filepath='include/linux/dax.h' line='17' column='1' id='241913ba'>
      <underlying-type type-id='9cac1fee'/>
      <enumerator name='DAX_ACCESS' value='0'/>
      <enumerator name='DAX_RECOVERY_WRITE' value='1'/>
    </enum-decl>

Note that they have distinct type ids and are both referenced by the
rest of the ABI.

This short series fixes the issue and another minor bug at the same
time.

Regards,
Giuliano.

Giuliano Procida (3):
  abidw: fix --stats output for resolved classes and enums
  abidw: remove always true test in resolve_declaration_only_classes
  abidw: resolve declaration-only enums the same as classes

 src/abg-dwarf-reader.cc | 84 ++++++++++++++++++++++++++++-------------
 1 file changed, 57 insertions(+), 27 deletions(-)

-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 1/3] abidw: fix --stats output for resolved classes and enums

[Giuliano Procida]

The code to print out the remaining declaration-only types
unintentionally omitted the first such type. This change fixes the
logic and uses a single test to decide whether or not to print the
stats header line.

	* src/abg-dwarf-reader.cc
	(read_context::resolve_declaration_only_classes): Fix
	conditional logic so that showing stats includes the first
	unresolved type.
	(read_context::resolve_declaration_only_enums): Likewise.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-dwarf-reader.cc | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index e5159c89..a954de6d 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -4325,19 +4325,15 @@ public:
 	 ++i)
       declaration_only_classes().erase(*i);
 
-    for (string_classes_map::iterator i = declaration_only_classes().begin();
-	 i != declaration_only_classes().end();
-	 ++i)
+    if (show_stats() && !declaration_only_classes().empty())
       {
-	if (show_stats())
-	  {
-	    if (i == declaration_only_classes().begin())
-	      cerr << "Here are the "
-		   << num_decl_only_classes - num_resolved
-		   << " unresolved class declarations:\n";
-	    else
-	      cerr << "    " << i->first << "\n";
-	  }
+	cerr << "Here are the "
+	     << num_decl_only_classes - num_resolved
+	     << " unresolved class declarations:\n";
+	for (string_classes_map::iterator i = declaration_only_classes().begin();
+	     i != declaration_only_classes().end();
+	     ++i)
+	  cerr << "    " << i->first << "\n";
       }
   }
 
@@ -4527,19 +4523,15 @@ public:
 	 ++i)
       declaration_only_enums().erase(*i);
 
-    for (string_enums_map::iterator i = declaration_only_enums().begin();
-	 i != declaration_only_enums().end();
-	 ++i)
+    if (show_stats() && !declaration_only_enums().empty())
       {
-	if (show_stats())
-	  {
-	    if (i == declaration_only_enums().begin())
-	      cerr << "Here are the "
-		   << num_decl_only_enums - num_resolved
-		   << " unresolved enum declarations:\n";
-	    else
-	      cerr << "    " << i->first << "\n";
-	  }
+	cerr << "Here are the "
+	     << num_decl_only_enums - num_resolved
+	     << " unresolved enum declarations:\n";
+	for (string_enums_map::iterator i = declaration_only_enums().begin();
+	     i != declaration_only_enums().end();
+	     ++i)
+	  cerr << "    " << i->first << "\n";
       }
   }
 
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 2/3] abidw: remove always true test in resolve_declaration_only_classes

[Giuliano Procida]

The code that makes the last attempt to resolve declaration-only types
was protected by a conditional checking that the number of TUs for a
given type was more than 1. The previous branch checked for exactly 1.
However, the entire block is inside a conditional where the number of
TUs is guaranteed to be greater than 0.

Removing the conditional makes it clear that this branch handles all
remaining cases.

	* src/abg-dwarf-reader.cc
	(read_context::resolve_declaration_only_classes): Remove
	tautological conditional.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-dwarf-reader.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index a954de6d..b5e60e35 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -4274,7 +4274,7 @@ public:
 		    else if (per_tu_class_map.size() == 1)
 		      (*j)->set_definition_of_declaration
 			(per_tu_class_map.begin()->second);
-		    else if (per_tu_class_map.size() > 1)
+		    else
 		      {
 			// We are in case where there are more than
 			// one definition for the declaration.  Let's
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 3/3] abidw: resolve declaration-only enums the same as classes

[Giuliano Procida]

The logic for resolving declaration-only enums and classes was almost
the same. However, the class code had a couple of extra improvements
that were missing from the enum code. One of these caused resolution
failures with Linux kernel ABIs, resulting in duplicate (declared /
defined) enums in ABI XML.

This change adds the improvements to the enum resolution code.

	* src/abg-dwarf-reader.cc
	(read_context::resolve_declaration_only_enums): Use an ordered
	map to ensure TUs are always considered in the same order and
	so improve ABI XML stability. Given multiple possible
	definitions for a enum declaration, check to see if they are
	equal and resolve the declaration to the first definition if
	so.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-dwarf-reader.cc | 42 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index b5e60e35..df50ef7d 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -4460,7 +4460,13 @@ public:
 	if (!enums)
 	  continue;
 
-	unordered_map<string, enum_type_decl_sptr> per_tu_enum_map;
+	// This is a map that associates the translation unit path to
+	// the enum (that potentially defines the declarations that
+	// we consider) that are defined in that translation unit.  It
+	// should stay ordered by using the TU path as key to ensure
+	// stability of the order of enum definitions in ABIXML
+	// output.
+	map<string, enum_type_decl_sptr> per_tu_enum_map;
 	for (type_base_wptrs_type::const_iterator c = enums->begin();
 	     c != enums->end();
 	     ++c)
@@ -4497,13 +4503,45 @@ public:
 		  {
 		    string tu_path =
 		      (*j)->get_translation_unit()->get_absolute_path();
-		    unordered_map<string, enum_type_decl_sptr>::const_iterator e =
+		    map<string, enum_type_decl_sptr>::const_iterator e =
 		      per_tu_enum_map.find(tu_path);
 		    if (e != per_tu_enum_map.end())
 		      (*j)->set_definition_of_declaration(e->second);
 		    else if (per_tu_enum_map.size() == 1)
 		      (*j)->set_definition_of_declaration
 			(per_tu_enum_map.begin()->second);
+		    else
+		      {
+			// We are in case where there are more than
+			// one definition for the declaration.  Let's
+			// see if they are all equal.  If they are,
+			// then the declaration resolves to the
+			// definition.  Otherwise, we are in the case
+			// 3/ described above.
+			map<string,
+			    enum_type_decl_sptr>::const_iterator it;
+			enum_type_decl_sptr first_enum =
+			  per_tu_enum_map.begin()->second;
+			bool all_enum_definitions_are_equal = true;
+			for (it = per_tu_enum_map.begin();
+			     it != per_tu_enum_map.end();
+			     ++it)
+			  {
+			    if (it == per_tu_enum_map.begin())
+			      continue;
+			    else
+			      {
+				if (!compare_before_canonicalisation(it->second,
+								     first_enum))
+				  {
+				    all_enum_definitions_are_equal = false;
+				    break;
+				  }
+			      }
+			  }
+			if (all_enum_definitions_are_equal)
+			  (*j)->set_definition_of_declaration(first_enum);
+		      }
 		  }
 	      }
 	    resolved_enums.push_back(i->first);
-- 
2.37.1.595.g718a3a8f04-goog

Re: [PATCH 1/3] abidw: fix --stats output for resolved classes and enums

[Dodji Seketeli]

Giuliano Procida <gprocida@google.com> a écrit:

> The code to print out the remaining declaration-only types
> unintentionally omitted the first such type. This change fixes the
> logic and uses a single test to decide whether or not to print the
> stats header line.
>
> 	* src/abg-dwarf-reader.cc
> 	(read_context::resolve_declaration_only_classes): Fix
> 	conditional logic so that showing stats includes the first
> 	unresolved type.
> 	(read_context::resolve_declaration_only_enums): Likewise.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>

Applied to master, thanks!

Cheers,

[...]

-- 
		Dodji

Re: [PATCH 2/3] abidw: remove always true test in resolve_declaration_only_classes

[Dodji Seketeli]

Giuliano Procida <gprocida@google.com> a écrit:

> The code that makes the last attempt to resolve declaration-only types
> was protected by a conditional checking that the number of TUs for a
> given type was more than 1. The previous branch checked for exactly 1.
> However, the entire block is inside a conditional where the number of
> TUs is guaranteed to be greater than 0.
>
> Removing the conditional makes it clear that this branch handles all
> remaining cases.
>
> 	* src/abg-dwarf-reader.cc
> 	(read_context::resolve_declaration_only_classes): Remove
> 	tautological conditional.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>

Applied to master, thanks !

[...]

Cheers,

-- 
		Dodji

Re: [PATCH 3/3] abidw: resolve declaration-only enums the same as classes

[Dodji Seketeli]

Giuliano Procida <gprocida@google.com> a écrit:

> The logic for resolving declaration-only enums and classes was almost
> the same. However, the class code had a couple of extra improvements
> that were missing from the enum code. One of these caused resolution
> failures with Linux kernel ABIs, resulting in duplicate (declared /
> defined) enums in ABI XML.
>
> This change adds the improvements to the enum resolution code.
>
> 	* src/abg-dwarf-reader.cc
> 	(read_context::resolve_declaration_only_enums): Use an ordered
> 	map to ensure TUs are always considered in the same order and
> 	so improve ABI XML stability. Given multiple possible
> 	definitions for a enum declaration, check to see if they are
> 	equal and resolve the declaration to the first definition if
> 	so.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>

Applied to master, thanks!

[...]

Cheers,

-- 
		Dodji