Re: [PATCH] Fix CK_BAD propagation for --odr

[Tom de Vries <tdevries@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

On 2/16/21 10:11 AM, Tom de Vries wrote:
> The problem is caused by the odr code in checksum_ref_die, which skips
> checksum calculation for the children of odr types, with as unintended
> side-effect that it break the CK_BAD propagation to toplevel DIEs.
> 
> Fix this by making the skipping of the checksum calculation less intrusive.
> 
> Also, add the assert in partition_found_dups to catch this earlier and
> easier.
> 
> Unfortunately, the fix also means that the earlier reported performance
> improvement of odr on cc1, of 42.30%:
> ...
> $ ./dwz.master cc1 -o 1 -lnone
> $ ./diff.sh cc1 1
> .debug_info      red: 44.84%    111527248  61527733
> .debug_abbrev    red: 40.28%    1722726  1028968
> .debug_str       red: 0%        6609355  6609355
> total            red: 42.30%    119859329 69166056
> ...
> to 54.55%:
> ...
> $ ./dwz.master cc1 -o 2 -lnone --odr
> $ ./diff.sh cc1 2
> .debug_info      red: 57.46%    111527248  47449258
> .debug_abbrev    red: 75.08%    1722726  429434
> .debug_str       red: 0%        6609355  6609355
> total            red: 54.55%    119859329 54488047
> ...
> pretty much disappears:
> ...
> $ ./dwz.fix cc1 -o 3 -lnone --odr
> $ ./diff.sh cc1 3
> .debug_info      red: 45.44%    111527248  60858640
> .debug_abbrev    red: 42.88%    1722726  984084
> .debug_str       red: 0%        6609355  6609355
> total            red: 42.89%    119859329 68452079
> ...
> 

I managed to come up with a fix that doesn't suffer from this
performance regression.

Any comments?

Thanks,
- Tom


[-- Attachment #2: 0001-Fix-CK_BAD-propagation-for-odr.patch --]
[-- Type: text/x-patch, Size: 6141 bytes --]

Fix CK_BAD propagation for --odr

With the reproducer from PR26252 we get:
...
$ dwz 2 -o 2.z --odr -lnone
dwz: dwz.c:7396: partition_found_dups: \
  Assertion `die->die_ck_state == CK_KNOWN' failed.
Aborted (core dumped)
...

The problem is caused by the odr code in checksum_ref_die, which skips
checksum calculation for the children of odr types, with as unintended
side-effect that it break the CK_BAD propagation to toplevel DIEs.

Fix this by making the skipping of the checksum calculation less intrusive.
Specially:
- undo all modifications related to odr in checksum_ref_die
- After calling checksum_ref_die in read_debug_info:
  - override die_ref_hash for odr DIEs, and
  - recalculate die_ref_hash for all other DIEs.

We still have the same amount of compression with cc1, that is: without odr
we have 42.30% reduction:
...
$ dwz cc1 -o cc1.z -lnone --no-odr
$ diff.sh cc1 cc1.z
.debug_info      red: 44.84%    111527248  61527733
.debug_abbrev    red: 40.28%    1722726  1028968
.debug_str       red: 0%        6609355  6609355
total            red: 42.30%    119859329 69166056
...
and with odr but without the bug fix we have 54.55%:
...
$ dwz cc1 -o cc1.z -lnone --odr
$ diff.sh cc1 cc1.z
.debug_info      red: 57.46%    111527248  47449258
.debug_abbrev    red: 75.08%    1722726  429434
.debug_str       red: 0%        6609355  6609355
total            red: 54.55%    119859329 54488047
...
and with odr and the bug fix still 54.55%:
...
$ dwz cc1 -o cc1.z -lnone --odr
$ ../measure/diff.sh cc1 cc1.z
.debug_info      red: 57.46%    111527248  47446501
.debug_abbrev    red: 75.51%    1722726  422027
.debug_str       red: 0%        6609355  6609355
total            red: 54.55%    119859329 54477883
...

2021-02-18  Tom de Vries  <tdevries@suse.de>

	PR dwz/26252
	* dwz.c (checksum_ref_die): Undo modifications related to odr.
	(read_debug_info): After calling checksum_ref_die, override
	die_ref_hash for odr DIEs, and recalculate die_ref_hash for all
	other DIEs.

---
 dwz.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 20 deletions(-)

diff --git a/dwz.c b/dwz.c
index 0b751fc..62ebe97 100644
--- a/dwz.c
+++ b/dwz.c
@@ -3994,7 +3994,6 @@ checksum_ref_die (dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die,
   unsigned int i, ret = 0;
   unsigned char *ptr;
   dw_die_ref child;
-  bool only_hash_name_p;
 
   if (top_die == die)
     {
@@ -4025,7 +4024,6 @@ checksum_ref_die (dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die,
   else
     assert (top_die == NULL || die->die_ck_state == CK_KNOWN);
   t = die->die_abbrev;
-  only_hash_name_p = odr && die_odr_state (die_cu (die), die) != ODR_NONE;
   for (i = 0; i < t->nattr; ++i)
     if (t->attr[i].attr != DW_AT_sibling)
       switch (t->attr[i].form)
@@ -4209,24 +4207,25 @@ checksum_ref_die (dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die,
 	}
     }
 
-  if (!only_hash_name_p
-      && (top_die == NULL || top_die->die_ck_state != CK_BAD))
-    for (child = die->die_child; child; child = child->die_sib)
-      {
-	unsigned int r
-	  = checksum_ref_die (cu,
-			      top_die ? top_die
-			      : child->die_named_namespace
-			      ? NULL : child, child,
-			      second_idx, second_hash);
-	if (top_die == NULL)
-	  assert (r == 0 && obstack_object_size (&ob) == 0);
-
-	if (ret == 0 || (r && r < ret))
-	  ret = r;
-	if (top_die && top_die->die_ck_state == CK_BAD)
-	  break;
-      }
+  if (top_die == NULL || top_die->die_ck_state != CK_BAD)
+    {
+      for (child = die->die_child; child; child = child->die_sib)
+	{
+	  unsigned int r
+	    = checksum_ref_die (cu,
+				top_die ? top_die
+				: child->die_named_namespace
+				? NULL : child, child,
+				second_idx, second_hash);
+	  if (top_die == NULL)
+	    assert (r == 0 && obstack_object_size (&ob) == 0);
+
+	  if (ret == 0 || (r && r < ret))
+	    ret = r;
+	  if (top_die && top_die->die_ck_state == CK_BAD)
+	    break;
+	}
+     }
 
   if (top_die == die)
     {
@@ -6742,6 +6741,24 @@ read_debug_info (DSO *dso, int kind, unsigned int *die_count)
 
 	      for (cu = cuf; cu; cu = cu->cu_next)
 		checksum_ref_die (cu, NULL, cu->cu_die, NULL, NULL);
+	      if (odr)
+		{
+		  for (cu = cuf; cu; cu = cu->cu_next)
+		    {
+		      dw_die_ref die;
+		      FOREACH_LOW_TOPLEVEL_DIE_IN_CU (die, cu)
+			{
+			  if (die->die_ck_state != CK_KNOWN)
+			    continue;
+			  if (die_odr_state (die_cu (die), die) != ODR_NONE)
+			    die->u.p1.die_ref_hash = die->u.p1.die_hash;
+			  else
+			    die->die_ref_hash_computed = 0;
+			}
+		    }
+		  for (cu = cuf; cu; cu = cu->cu_next)
+		    checksum_ref_die (cu, NULL, cu->cu_die, NULL, NULL);
+		}
 
 	      if (dump_dies_p)
 		for (cu = cuf; cu; cu = cu->cu_next)
@@ -7220,6 +7237,20 @@ read_debug_info (DSO *dso, int kind, unsigned int *die_count)
 	  if (checksum_die (dso, cu, NULL, cu->cu_die))
 	    goto fail;
 	  checksum_ref_die (cu, NULL, cu->cu_die, NULL, NULL);
+	  if (odr)
+	    {
+	      dw_die_ref die;
+	      FOREACH_LOW_TOPLEVEL_DIE_IN_CU (die, cu)
+		{
+		  if (die->die_ck_state != CK_KNOWN)
+		    continue;
+		  if (die_odr_state (die_cu (die), die) != ODR_NONE)
+		    die->u.p1.die_ref_hash = die->u.p1.die_hash;
+		  else
+		    die->die_ref_hash_computed = 0;
+		}
+	      checksum_ref_die (cu, NULL, cu->cu_die, NULL, NULL);
+	    }
 
 	  if (dump_dies_p)
 	    dump_dies (0, cu->cu_die);
@@ -7292,6 +7323,24 @@ read_debug_info (DSO *dso, int kind, unsigned int *die_count)
 
       for (cu = first_cu; cu; cu = cu->cu_next)
 	checksum_ref_die (cu, NULL, cu->cu_die, NULL, NULL);
+      if (odr)
+	{
+	  for (cu = first_cu; cu; cu = cu->cu_next)
+	    {
+	      dw_die_ref die;
+	      FOREACH_LOW_TOPLEVEL_DIE_IN_CU (die, cu)
+		{
+		  if (die->die_ck_state != CK_KNOWN)
+		    continue;
+		  if (die_odr_state (die_cu (die), die) != ODR_NONE)
+		    die->u.p1.die_ref_hash = die->u.p1.die_hash;
+		  else
+		    die->die_ref_hash_computed = 0;
+		}
+	    }
+	  for (cu = first_cu; cu; cu = cu->cu_next)
+	    checksum_ref_die (cu, NULL, cu->cu_die, NULL, NULL);
+	}
 
       if (dump_dies_p)
 	for (cu = first_cu; cu; cu = cu->cu_next)