stabilize .gcc_except_table with or without -g

stabilize .gcc_except_table with or without -g

[Alexandre Oliva]

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

In Java, on x86_64-linux-gnu (both -m32 and -m64 IIRC)
.gcc_except_table action record tables are sometimes output using
dw2_asm_output_encoded_addr_rtx(), which, with indirect addressing,
forces expressions into memory using a splay tree that uses string
*pointers* as keys.  These pointers are not stable, so the order in
which the splay tree entries are output by
dw2_output_indirect_constants() varies depending on where the symbol
names are located in memory.  Their locations often change when -g is
given, so we end up generating code that wouldn't pass compare-debug.

I haven't completed bootstrap-testing of this patch yet, but it has
shaved off more than half of the remaining compare-debug differences
between libjava compiled with -fvar-tracking-assignments enabled or
disabled on x86_64-linux-gnu.

Ok to install if it passes bootstrap?  Should new the compare function
be moved into libiberty proper?  Does the assertion check seem safe in
the context of this use, that when pointers differ then the strings
they point to must differ as well?  It's not necessary, but I thought
it would be nice to make sure we weren't goofing with identical symbol
names at different locations before.  I could be easily convinced to
take it out, though.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-stabilize-dw2-force-const-mem.patch --]
[-- Type: text/x-patch, Size: 1548 bytes --]

for  gcc/ChangeLog .vta?
from  Alexandre Oliva  <aoliva@redhat.com>

	* dwarf2asm.c (splay_tree_compare_strings): New function.
	(dw2_force_const_mem): Use it.

Index: gcc/dwarf2asm.c
===================================================================
--- gcc/dwarf2asm.c.orig	2007-11-05 04:51:08.000000000 -0200
+++ gcc/dwarf2asm.c	2007-11-05 02:20:11.000000000 -0200
@@ -701,6 +701,28 @@ static GTY(()) int dw2_const_labelno;
 # define USE_LINKONCE_INDIRECT 0
 #endif
 
+/* Compare C strings used as keys in a splay tree, optimizing the case
+   in which the pointers are identical.  We need to use string rather
+   than pointer comparison in order to make Java exception action
+   records stable in the presence of different debug info options.  */
+
+static int
+splay_tree_compare_strings (splay_tree_key k1, splay_tree_key k2)
+{
+  const char *s1 = (const char *)k1;
+  const char *s2 = (const char *)k2;
+  int ret;
+
+  if (s1 == s2)
+    return 0;
+
+  ret = strcmp (s1, s2);
+
+  gcc_assert (ret);
+
+  return ret;
+}
+
 /* Put X, a SYMBOL_REF, in memory.  Return a SYMBOL_REF to the allocated
    memory.  Differs from force_const_mem in that a single pool is used for
    the entire unit of translation, and the memory is not guaranteed to be
@@ -715,7 +737,7 @@ dw2_force_const_mem (rtx x, bool public)
   tree decl;
 
   if (! indirect_pool)
-    indirect_pool = splay_tree_new_ggc (splay_tree_compare_pointers);
+    indirect_pool = splay_tree_new_ggc (splay_tree_compare_strings);
 
   gcc_assert (GET_CODE (x) == SYMBOL_REF);
 

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: stabilize .gcc_except_table with or without -g

[Mark Mitchell]

Alexandre Oliva wrote:

> I haven't completed bootstrap-testing of this patch yet, but it has
> shaved off more than half of the remaining compare-debug differences
> between libjava compiled with -fvar-tracking-assignments enabled or
> disabled on x86_64-linux-gnu.
> 
> Ok to install if it passes bootstrap?  Should new the compare function
> be moved into libiberty proper?  Does the assertion check seem safe in
> the context of this use, that when pointers differ then the strings
> they point to must differ as well?  It's not necessary, but I thought
> it would be nice to make sure we weren't goofing with identical symbol
> names at different locations before.  I could be easily convinced to
> take it out, though.

Thanks for working on this issue.

+/* Compare C strings used as keys in a splay tree, optimizing the case
+   in which the pointers are identical.  We need to use string rather
+   than pointer comparison in order to make Java exception action
+   records stable in the presence of different debug info options.  */

I think a lot of this comment is unhelpful.  I would say:

  /* Comparison function for a splay tree in which the keys are strings.
     K1 and K2 have the dynamic type "const char *".  Returns <0, 0,
     or >0 to indicate whether K1 is less than, equal to, or greater
     than K2, respectively.  */

The optimization isn't important to a reader trying to understand the
function is for.  The Java comment is confusing, in that it might lead
you to think that without Java we don't need to do this -- but, quite
probably, we do.  In the body of the function, you could write:

  /* We use strcmp, rather than just comparing pointers, so that the
     sort order will not depend on the host system.  */

If you leave the assertion in, you definitely should comment:

  /* The strings are always those from IDENTIFIER_NODEs, and, therefore,
     we should never have two copies of the same string.  */

The patch is OK with those changes.  If DJ prefers to put this function
in libiberty, that's fine; if you check it into GCC first, consider a
patch to adjust GCC to call into libiberty pre-approved.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: stabilize .gcc_except_table with or without -g

[DJ Delorie]

> Should new the compare function be moved into libiberty proper?

If it were to be moved into libiberty, I think (1) it would need to
guard against NULL arguments, and (2) it must assume that two strings
may compare equal.  Libiberty functions shouldn't abort if they can
return a meaningful error value.

As for should it be in libiberty... I think it would be reasonable for
each hash type to have a "stock" implementation of hashing a simple
string.  Sometimes it would be useful, but at least it would serve as
an example upon which to build application-specific hashes.

Re: stabilize .gcc_except_table with or without -g

[Alexandre Oliva]

On Nov  5, 2007, DJ Delorie <dj@redhat.com> wrote:

>> Should new the compare function be moved into libiberty proper?

> If it were to be moved into libiberty, I think (1) it would need to
> guard against NULL arguments, and (2) it must assume that two strings
> may compare equal.  Libiberty functions shouldn't abort if they can
> return a meaningful error value.

Works for me.

> As for should it be in libiberty... I think it would be reasonable for
> each hash type to have a "stock" implementation of hashing a simple
> string.

But this is not about hashing at all.  We're talking about no more
than a wrapper for strcmp with different static types for the
arguments.  So I'm not sure it's all that useful for libiberty, but I
can see that it could be convenient.  Your call, really.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: stabilize .gcc_except_table with or without -g

[Alexandre Oliva]

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

On Nov  5, 2007, Mark Mitchell <mark@codesourcery.com> wrote:

>   /* Comparison function for a splay tree in which the keys are strings.
>      K1 and K2 have the dynamic type "const char *".  Returns <0, 0,
>      or >0 to indicate whether K1 is less than, equal to, or greater
>      than K2, respectively.  */

> probably, we do.  In the body of the function, you could write:

>   /* We use strcmp, rather than just comparing pointers, so that the
>      sort order will not depend on the host system.  */

> If you leave the assertion in, you definitely should comment:

>   /* The strings are always those from IDENTIFIER_NODEs, and, therefore,
>      we should never have two copies of the same string.  */

> The patch is OK with those changes.

Thanks, here's what I'm checking in:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-stabilize-dw2-force-const-mem.patch --]
[-- Type: text/x-patch, Size: 1772 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* dwarf2asm.c (splay_tree_compare_strings): New function.
	(dw2_force_const_mem): Use it.

Index: gcc/dwarf2asm.c
===================================================================
--- gcc/dwarf2asm.c.orig	2007-11-25 03:04:19.000000000 -0200
+++ gcc/dwarf2asm.c	2007-11-25 03:31:43.000000000 -0200
@@ -701,6 +701,31 @@ static GTY(()) int dw2_const_labelno;
 # define USE_LINKONCE_INDIRECT 0
 #endif
 
+/* Comparison function for a splay tree in which the keys are strings.
+   K1 and K2 have the dynamic type "const char *".  Returns <0, 0, or
+   >0 to indicate whether K1 is less than, equal to, or greater than
+   K2, respectively.  */
+
+static int
+splay_tree_compare_strings (splay_tree_key k1, splay_tree_key k2)
+{
+  const char *s1 = (const char *)k1;
+  const char *s2 = (const char *)k2;
+  int ret;
+
+  if (s1 == s2)
+    return 0;
+
+  ret = strcmp (s1, s2);
+
+  /* The strings are always those from IDENTIFIER_NODEs, and,
+     therefore, we should never have two copies of the same
+     string.  */
+  gcc_assert (ret);
+
+  return ret;
+}
+
 /* Put X, a SYMBOL_REF, in memory.  Return a SYMBOL_REF to the allocated
    memory.  Differs from force_const_mem in that a single pool is used for
    the entire unit of translation, and the memory is not guaranteed to be
@@ -715,7 +740,9 @@ dw2_force_const_mem (rtx x, bool public)
   tree decl;
 
   if (! indirect_pool)
-    indirect_pool = splay_tree_new_ggc (splay_tree_compare_pointers);
+    /* We use strcmp, rather than just comparing pointers, so that the
+       sort order will not depend on the host system.  */
+    indirect_pool = splay_tree_new_ggc (splay_tree_compare_strings);
 
   gcc_assert (GET_CODE (x) == SYMBOL_REF);
 

[-- Attachment #3: Type: text/plain, Size: 249 bytes --]


-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
FSF Latin America Board Member         http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}