diff mbox series

GGC, GTY: No pointer walking for 'atomic' in PCH 'gt_pch_note_object' (was: Patch: New GTY ((atomic)) option)

Message ID 87a5watjg1.fsf@euler.schwinge.homeip.net
State New
Headers show
Series GGC, GTY: No pointer walking for 'atomic' in PCH 'gt_pch_note_object' (was: Patch: New GTY ((atomic)) option) | expand

Commit Message

Thomas Schwinge July 5, 2023, 4:24 p.m. UTC 
Hi!

My original motivation for the following exercise what that, for example,
for: 'const unsigned char * GTY((atomic)) mode_table', we currently run
into 'const' mismatches, 'error: invalid conversion':

    [...]
    gtype-desc.cc: In function 'void gt_pch_nx_lto_file_decl_data(void*)':
    gtype-desc.cc:6531:34: error: invalid conversion from 'const void*' to 'void*' [-fpermissive]
             gt_pch_note_object ((*x).mode_table, x, gt_pch_p_18lto_file_decl_data);
                                      ^
    In file included from [...]/source-gcc/gcc/hash-table.h:247:0,
                     from [...]/source-gcc/gcc/coretypes.h:486,
                     from gtype-desc.cc:23:
    [...]/source-gcc/gcc/ggc.h:47:12: note:   initializing argument 1 of 'int gt_pch_note_object(void*, void*, gt_note_pointers, size_t)'
     extern int gt_pch_note_object (void *, void *, gt_note_pointers,
                ^
    make[2]: *** [Makefile:1180: gtype-desc.o] Error 1
    [...]

..., as I had reported as "'GTY' issues: (1) 'const' build error" in
<https://inbox.sourceware.org/87o7kxuq9s.fsf@euler.schwinge.homeip.net>
'Adjust LTO mode tables for "Machine_Mode: Extend machine_mode from 8 to 16 bits"'.

That said:

On 2011-05-16T02:13:56+0200, "Nicola Pero" <nicola.pero@meta-innovation.com> wrote:
> This patch adds a new GTY option, "atomic", which is similar to the identical option you have with Boehm GC
> and which can be used with pointers to inform the GC/PCH machinery that they point to an area of memory that
> contains no pointers (and hence needs no scanning).
>
> [...]

On top of that, OK to push the attached
"GGC, GTY: No pointer walking for 'atomic' in PCH 'gt_pch_note_object'"?
Appreciate review from a GGC, GTY-savvy person.

This depends on
<https://inbox.sourceware.org/87edlmtjwh.fsf@euler.schwinge.homeip.net>
"GGC, GTY: Tighten up a few things re 'reorder' option and strings".


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Comments

Richard Biener July 6, 2023, 6:07 a.m. UTC | #1 
On Wed, Jul 5, 2023 at 6:25 PM Thomas Schwinge <thomas@codesourcery.com> wrote:
>
> Hi!
>
> My original motivation for the following exercise what that, for example,
> for: 'const unsigned char * GTY((atomic)) mode_table', we currently run
> into 'const' mismatches, 'error: invalid conversion':
>
>     [...]
>     gtype-desc.cc: In function 'void gt_pch_nx_lto_file_decl_data(void*)':
>     gtype-desc.cc:6531:34: error: invalid conversion from 'const void*' to 'void*' [-fpermissive]
>              gt_pch_note_object ((*x).mode_table, x, gt_pch_p_18lto_file_decl_data);
>                                       ^
>     In file included from [...]/source-gcc/gcc/hash-table.h:247:0,
>                      from [...]/source-gcc/gcc/coretypes.h:486,
>                      from gtype-desc.cc:23:
>     [...]/source-gcc/gcc/ggc.h:47:12: note:   initializing argument 1 of 'int gt_pch_note_object(void*, void*, gt_note_pointers, size_t)'
>      extern int gt_pch_note_object (void *, void *, gt_note_pointers,
>                 ^
>     make[2]: *** [Makefile:1180: gtype-desc.o] Error 1
>     [...]
>
> ..., as I had reported as "'GTY' issues: (1) 'const' build error" in
> <https://inbox.sourceware.org/87o7kxuq9s.fsf@euler.schwinge.homeip.net>
> 'Adjust LTO mode tables for "Machine_Mode: Extend machine_mode from 8 to 16 bits"'.
>
> That said:
>
> On 2011-05-16T02:13:56+0200, "Nicola Pero" <nicola.pero@meta-innovation.com> wrote:
> > This patch adds a new GTY option, "atomic", which is similar to the identical option you have with Boehm GC
> > and which can be used with pointers to inform the GC/PCH machinery that they point to an area of memory that
> > contains no pointers (and hence needs no scanning).
> >
> > [...]
>
> On top of that, OK to push the attached
> "GGC, GTY: No pointer walking for 'atomic' in PCH 'gt_pch_note_object'"?
> Appreciate review from a GGC, GTY-savvy person.

OK.  Thanks for the detailed explanations, that helps even a not
GGC/GTY savy person to
review this ;)

Thanks,
Richard.

> This depends on
> <https://inbox.sourceware.org/87edlmtjwh.fsf@euler.schwinge.homeip.net>
> "GGC, GTY: Tighten up a few things re 'reorder' option and strings".
>
>
> Grüße
>  Thomas
>
>
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
diff mbox series

Patch

From 2f12ce94166f411e4b9084b1c89738bb480343cc Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Tue, 4 Jul 2023 11:46:00 +0200
Subject: [PATCH] GGC, GTY: No pointer walking for 'atomic' in PCH
 'gt_pch_note_object'

Since it's inception in 2011 commit 555c3771903cc461949f06acf28f92fc067b6a1c
(Subversion r173996) "New GTY ((atomic)) option", the following rationale has
been given in (nowadays) 'gcc/gengtype.cc':

    If a pointer type is marked as "atomic", we process the
    field itself, but we don't walk the data that they point to.

    There are two main cases where we walk types: to mark
    pointers that are reachable, and to relocate pointers when
    writing a PCH file.  In both cases, an atomic pointer is
    itself marked or relocated, but the memory that it points
    to is left untouched.  In the case of PCH, that memory will
    be read/written unchanged to the PCH file.

Therefore, we may completely skip the boilerplate pointer walking, which we
didn't for PCH 'gt_pch_note_object'.

    --- build-gcc/gcc/gt-c-c-decl.h	2023-06-26 08:59:55.120395571 +0200
    +++ build-gcc/gcc/gt-c-c-decl.h	2023-07-05 15:58:36.286165439 +0200
    @@ -1138,7 +1138,7 @@
                 case TS_OPTIMIZATION:
                   gt_pch_n_15cl_optimization ((*x).generic.optimization.opts);
                   if ((*x).generic.optimization.optabs != NULL) {
    -                gt_pch_note_object ((*x).generic.optimization.optabs, x, gt_pch_p_14lang_tree_node);
    +                gt_pch_note_object ((*x).generic.optimization.optabs, x, NULL);
                   }
                   break;
                 case TS_TARGET_OPTION:
    --- build-gcc/gcc/gt-cp-tree.h	2023-06-26 08:59:55.120395571 +0200
    +++ build-gcc/gcc/gt-cp-tree.h	2023-07-05 15:58:36.286165439 +0200
    @@ -1452,7 +1452,7 @@
                 case TS_OPTIMIZATION:
                   gt_pch_n_15cl_optimization ((*x).generic.optimization.opts);
                   if ((*x).generic.optimization.optabs != NULL) {
    -                gt_pch_note_object ((*x).generic.optimization.optabs, x, gt_pch_p_14lang_tree_node);
    +                gt_pch_note_object ((*x).generic.optimization.optabs, x, NULL);
                   }
                   break;
                 case TS_TARGET_OPTION:
    [...]

..., which is for 'gcc/tree-core.h':

    struct GTY(()) tree_optimization_option {
      struct tree_base base;
      [...]
      struct cl_optimization *opts;
      [...]
      void *GTY ((atomic)) optabs;
      [...]
      struct target_optabs *GTY ((skip)) base_optabs;
    };

..., which means we'll still 'gt_pch_note_object' 'optabs' itself.  (That's
important; if we skip those completely, we'll later fail the
'gcc_assert (result)' in 'gcc/ggc-common.cc:relocate_ptrs'.)  However, we no
longer attempt to walk 'optabs' via 'gt_pch_save', which per the original
'build-gcc/gcc/gt-c-c-decl.h:gt_pch_p_14lang_tree_node' call here meant:

    void
    gt_pch_p_14lang_tree_node (ATTRIBUTE_UNUSED void *this_obj,
    	void *x_p,
    	ATTRIBUTE_UNUSED gt_pointer_operator op,
    	ATTRIBUTE_UNUSED void *cookie)
    {
      union lang_tree_node * x ATTRIBUTE_UNUSED = (union lang_tree_node *)x_p;
      switch ((int) (TREE_CODE (&((*x)).generic) == IDENTIFIER_NODE))
        {
        case 0:
          switch ((int) (tree_node_structure (&((*x).generic))))
            {
    [...]
            case TS_OPTIMIZATION:
              if ((void *)(x) == this_obj)
                op (&((*x).generic.optimization.opts), NULL, cookie);
              if ((*x).generic.optimization.optabs != NULL) {
                if ((void *)(x) == this_obj)
                  op (&((*x).generic.optimization.optabs), NULL, cookie);
              }
              break;
    [...]
          break;
    [...]
        }
    }

Given that we're changing the
'gt_pch_note_object ((*x).generic.optimization.optabs, x, [...])' call, in
'gt_pch_p_14lang_tree_node' it was the case that (the second) '(x) == this_obj'
did *not* hold, and we therefore didn't call
'op (&((*x).generic.optimization.optabs), NULL, cookie)', and therefore not
invoking 'gt_pch_p_14lang_tree_node' at all does achieve the same thing (no-op)
-- per my understanding of all this machinery.

The other (few) changes/instances of 'atomic' behave similarly.

As a corollary, we may then tag 'const' data as 'atomic' (for example:
'const unsigned char * GTY((atomic)) mode_table'), without running into 'const'
mismatches, 'error: invalid conversion':

    [...]
    gtype-desc.cc: In function 'void gt_pch_nx_lto_file_decl_data(void*)':
    gtype-desc.cc:6531:34: error: invalid conversion from 'const void*' to 'void*' [-fpermissive]
             gt_pch_note_object ((*x).mode_table, x, gt_pch_p_18lto_file_decl_data);
                                      ^
    In file included from [...]/source-gcc/gcc/hash-table.h:247:0,
                     from [...]/source-gcc/gcc/coretypes.h:486,
                     from gtype-desc.cc:23:
    [...]/source-gcc/gcc/ggc.h:47:12: note:   initializing argument 1 of 'int gt_pch_note_object(void*, void*, gt_note_pointers, size_t)'
     extern int gt_pch_note_object (void *, void *, gt_note_pointers,
                ^
    make[2]: *** [Makefile:1180: gtype-desc.o] Error 1
    [...]

..., which was the original motivation of this exercise.

	gcc/
	* gengtype.cc (struct walk_type_data): Add 'bool atomic_p'.
	(walk_type, write_types_process_field): Handle it.
	* ggc-common.cc (gt_pch_note_reorder, gt_pch_save): Likewise.
---
 gcc/gengtype.cc   | 10 +++++++++-
 gcc/ggc-common.cc |  4 ++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/gcc/gengtype.cc b/gcc/gengtype.cc
index 49ddba684af..07c7c4e91c8 100644
--- a/gcc/gengtype.cc
+++ b/gcc/gengtype.cc
@@ -2451,6 +2451,7 @@  struct walk_type_data
   bool in_ptr_field;
   bool have_this_obj;
   bool in_nested_ptr;
+  bool atomic_p;
 };
 
 
@@ -2747,11 +2748,16 @@  walk_type (type_p t, struct walk_type_data *d)
 	   be read/written unchanged to the PCH file.  */
 	if (atomic_p)
 	  {
+	    bool old_atomic_p = d->atomic_p;
+	    d->atomic_p = true;
+
 	    oprintf (d->of, "%*sif (%s != NULL) {\n", d->indent, "", d->val);
 	    d->indent += 2;
 	    d->process_field (t, d);
 	    d->indent -= 2;
 	    oprintf (d->of, "%*s}\n", d->indent, "");
+
+	    d->atomic_p = old_atomic_p;
 	    break;
 	  }
 
@@ -3214,7 +3220,9 @@  write_types_process_field (type_p f, const struct walk_type_data *d)
 	    oprintf (d->of, ", x");
 	  else
 	    oprintf (d->of, ", %s", d->prev_val[3]);
-	  if (d->orig_s)
+	  if (d->atomic_p)
+	    oprintf (d->of, ", NULL");
+	  else if (d->orig_s)
 	    {
 	      oprintf (d->of, ", gt_%s_", wtd->param_prefix);
 	      output_mangled_typename (d->of, d->orig_s);
diff --git a/gcc/ggc-common.cc b/gcc/ggc-common.cc
index bed7a9d4d02..f30ac748688 100644
--- a/gcc/ggc-common.cc
+++ b/gcc/ggc-common.cc
@@ -316,7 +316,8 @@  gt_pch_note_reorder (void *obj, void *note_ptr_cookie,
   gcc_assert (data && data->note_ptr_cookie == note_ptr_cookie);
   /* The GTY 'reorder' option doesn't make sense if we don't walk pointers,
      such as for strings.  */
-  gcc_checking_assert (data->note_ptr_fn != gt_pch_p_S);
+  gcc_checking_assert (data->note_ptr_fn != NULL
+		       && data->note_ptr_fn != gt_pch_p_S);
 
   data->reorder_fn = reorder_fn;
 }
@@ -640,7 +641,6 @@  gt_pch_save (FILE *f)
 				   state.ptrs[i]->note_ptr_cookie,
 				   relocate_ptrs, &state);
       gt_note_pointers note_ptr_fn = state.ptrs[i]->note_ptr_fn;
-      gcc_checking_assert (note_ptr_fn != NULL);
       /* 'gt_pch_p_S' enables certain special handling, but otherwise
      corresponds to no 'note_ptr_fn'.  */
       if (note_ptr_fn == gt_pch_p_S)
-- 
2.34.1