Patchwork remove illegal if_marked usage in type_hash_marked_p

login
register
mail settings
Submitter Tom de Vries
Date July 9, 2010, 11:13 a.m.
Message ID <B361D33B-5783-4E62-9D6B-DB01DBE47C29@xs4all.nl>
Download mbox | patch
Permalink /patch/58387/
State New
Headers show

Comments

Tom de Vries - July 9, 2010, 11:13 a.m.
The hash table type_hash_table has as if_marked function  
type_hash_marked_p.
...
static GTY ((if_marked ("type_hash_marked_p"), param_is (struct  
type_hash)))
      htab_t type_hash_table;
...

The 'TYPE_SYMTAB_POINTER (type)' clause in type_hash_marked_p has 2  
problems:
1. it is incorrect usage of the if_marked construct. Only testing of  
ggc_marked_p is allowed in a if_marked function, as stated in http:// 
gcc.gnu.org/ml/gcc/2010-06/msg00693.html.
2. it has no positive effect from 4.4 onwards. From 4.4 onwards, - 
funit-at-a-time is more or less hardcoded, with as effect that if a  
type is used in any function in the unit, the type will be marked.  
This way, it is already ensured that different functions will share  
types and therefore debug info. The only effect the clause has from  
4.4 onwards, is to keep types not used in the unit, from being  
garbage collected.

The problematic clause was found via a debug bootstrap comparison  
failure for 4.3, as described in bug 31230.

The patch is for trunk, and removes the problematic clause.

The testcase is not very good, since it also passes without the patch  
for trunk, but I don't know how to make a better one.

Bootstrapped and regression tested (gcc, objc, gfortran, g++,  
libgomp, libstdc++, libjava, libmudflap, libffi) on x86_64-unknown- 
linux-gnu with and without patch, with similar results.
Same thing for a debug bootstrap.

Ok for trunk? FYI I don't have copyright assignment or write access.

	* tree.c (type_hash_if_marked_p): illegal if_marked usage,
	removed non-ggc_marked_p clause.

+  char buffer2[100];
+}
Richard Guenther - July 9, 2010, 11:35 a.m.
On Fri, Jul 9, 2010 at 1:13 PM, Tom de Vries <tjvries@xs4all.nl> wrote:
> The hash table type_hash_table has as if_marked function type_hash_marked_p.
> ...
> static GTY ((if_marked ("type_hash_marked_p"), param_is (struct type_hash)))
>     htab_t type_hash_table;
> ...
>
> The 'TYPE_SYMTAB_POINTER (type)' clause in type_hash_marked_p has 2
> problems:
> 1. it is incorrect usage of the if_marked construct. Only testing of
> ggc_marked_p is allowed in a if_marked function, as stated in
> http://gcc.gnu.org/ml/gcc/2010-06/msg00693.html.
> 2. it has no positive effect from 4.4 onwards. From 4.4 onwards,
> -funit-at-a-time is more or less hardcoded, with as effect that if a type is
> used in any function in the unit, the type will be marked. This way, it is
> already ensured that different functions will share types and therefore
> debug info. The only effect the clause has from 4.4 onwards, is to keep
> types not used in the unit, from being garbage collected.
>
> The problematic clause was found via a debug bootstrap comparison failure
> for 4.3, as described in bug 31230.
>
> The patch is for trunk, and removes the problematic clause.
>
> The testcase is not very good, since it also passes without the patch for
> trunk, but I don't know how to make a better one.
>
> Bootstrapped and regression tested (gcc, objc, gfortran, g++, libgomp,
> libstdc++, libjava, libmudflap, libffi) on x86_64-unknown-linux-gnu with and
> without patch, with similar results.
> Same thing for a debug bootstrap.
>
> Ok for trunk? FYI I don't have copyright assignment or write access.

Ok.  I will take care of applyign it.

Thanks,
Richard.

>        * tree.c (type_hash_if_marked_p): illegal if_marked usage,
>        removed non-ggc_marked_p clause.
>
> Index: src/gcc/tree.c
> ===================================================================
> --- src/gcc/tree.c      (revision 161295)
> +++ src/gcc/tree.c      (working copy)
> @@ -5976,16 +5976,14 @@ type_hash_canon (unsigned int hashcode,
>
>  /* See if the data pointed to by the type hash table is marked.  We
> consider
>    it marked if the type is marked or if a debug type number or symbol
> -   table entry has been made for the type.  This reduces the amount of
> -   debugging output and eliminates that dependency of the debug output on
> -   the number of garbage collections.  */
> +   table entry has been made for the type.  */
>
>  static int
>  type_hash_marked_p (const void *p)
>  {
>   const_tree const type = ((const struct type_hash *) p)->type;
>
> -  return ggc_marked_p (type) || TYPE_SYMTAB_POINTER (type);
> +  return ggc_marked_p (type);
>  }
>
>  static void
> Index: src/gcc/testsuite/gcc.dg/debug/dwarf2/pr31230.c
> ===================================================================
> --- src/gcc/testsuite/gcc.dg/debug/dwarf2/pr31230.c     (revision 0)
> +++ src/gcc/testsuite/gcc.dg/debug/dwarf2/pr31230.c     (revision 0)
> @@ -0,0 +1,19 @@
> +/* { dg-do compile } */
> +/* { dg-options "-gdwarf-2 -dA --param ggc-min-expand=0 --param
> ggc-min-heapsize=0" } */
> +/* { dg-final { scan-assembler-times "DIE.*DW_TAG_array_type" 1  } } */
> +/* { dg-final { scan-assembler-times "DIE.*DW_TAG_subrange_type" 1  } } */
> +
> +void f1 (void)
> +{
> +  char buffer1[100];
> +}
> +
> +int f2 (void)
> +{
> +  return 0;
> +}
> +
> +void f3 (void)
> +{
> +  char buffer2[100];
> +}
>
>

Patch

Index: src/gcc/tree.c
===================================================================
--- src/gcc/tree.c	(revision 161295)
+++ src/gcc/tree.c	(working copy)
@@ -5976,16 +5976,14 @@  type_hash_canon (unsigned int hashcode,

  /* See if the data pointed to by the type hash table is marked.  We  
consider
     it marked if the type is marked or if a debug type number or symbol
-   table entry has been made for the type.  This reduces the amount of
-   debugging output and eliminates that dependency of the debug  
output on
-   the number of garbage collections.  */
+   table entry has been made for the type.  */

  static int
  type_hash_marked_p (const void *p)
  {
    const_tree const type = ((const struct type_hash *) p)->type;

-  return ggc_marked_p (type) || TYPE_SYMTAB_POINTER (type);
+  return ggc_marked_p (type);
  }

  static void
Index: src/gcc/testsuite/gcc.dg/debug/dwarf2/pr31230.c
===================================================================
--- src/gcc/testsuite/gcc.dg/debug/dwarf2/pr31230.c	(revision 0)
+++ src/gcc/testsuite/gcc.dg/debug/dwarf2/pr31230.c	(revision 0)
@@ -0,0 +1,19 @@ 
+/* { dg-do compile } */
+/* { dg-options "-gdwarf-2 -dA --param ggc-min-expand=0 --param ggc- 
min-heapsize=0" } */
+/* { dg-final { scan-assembler-times "DIE.*DW_TAG_array_type" 1  } } */
+/* { dg-final { scan-assembler-times "DIE.*DW_TAG_subrange_type"  
1  } } */
+
+void f1 (void)
+{
+  char buffer1[100];
+}
+
+int f2 (void)
+{
+  return 0;
+}
+
+void f3 (void)
+{