Patch: New GTY ((atomic)) option

Patch: New GTY ((atomic)) option

[Nicola Pero]

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).

The reason for adding this option is that, without it, it seems to be (surprisingly) impossible
to write code that keeps a GC pointer to a plain array of C stuff such as integers.  In my case,
I was experimenting with hash tables that can automatically cache hash values.  So I needed a plain
C array to store the cached hash values, but found that it is currently unsupported by GC/PCH! :-(

That is, at the moment you can't have a struct such as the following one --

struct GTY(()) my_struct {
  ...
  unsigned int * some_ints;
  size_t count;
  ...
};

because gengtype rejects it with the error "field `(*x).some_ints' is pointer to unimplemented type".

This patch basically implements it, but at this stage requires you to explicitly tell gengtype that the
pointer is atomic (and that is safe for gengtype to ignore the memory it points to).  So, the following
now works as expected --

struct GTY(()) my_struct {
  ...
  unsigned int * GTY((atomic)) some_ints;
  size_t count;
  ...
};

A next, nice step would be to have gengtype automatically mark as "atomic" any pointers that gengtype can safely determine
point to an area of memory that never contains any pointers.  But that's slightly more complicated (eg, currently
gengtype makes no difference between "unsigned int" and "void", hence "unsigned int *" and "void *" would be treated
the same, while you'd want the first one to be automatically marked as atomic, and the second one to generate an error
as gengtype has no way to determine if it's atomic or not - unless it's explicitly marked as atomic of course), so for now
I haven't implemented it; it could be a follow-up patch (even after implementing it, the explicit "atomic" option
would remain useful for "void *" pointers and such like, so it's a good starting point).

Btw, there are a few existing pointers in GCC that could be marked as atomic, for example the field "su" of struct
function in function.h.  The advantage of marking them as atomic would be a slight speedup of the GC marking by saving
a function call each time one of these structs is being walked; I suspect that alone wouldn't make any visibile difference
in practice, but I haven't done any profiling or benchmarking to know for sure.

I have done some testing of this patch, and I want to do some more before I commit.  If anyone has good ideas on how
to perform throughout testing, they are welcome. :-)

Ok to commit ?

Thanks

PS: This patch does not include support for marking root/global variables with "atomic" (neither manually nor automatically);
only fields in a struct.  That would be useful too, but I'm leaving it for yet another patch.

2011-05-16  Nicola Pero  <nicola.pero@meta-innovation.com>

        * gengtype.c (walk_type): Implemented "atomic" GTY option.
        * doc/gty.texi (GTY Options): Document "atomic" GTY option.

Index: doc/gty.texi
===================================================================
--- doc/gty.texi        (revision 173768)
+++ doc/gty.texi        (working copy)
@@ -383,6 +383,42 @@ could be calculated as follows:
   size_t size = sizeof (struct sorted_fields_type) + n * sizeof (tree);
 @end smallexample
 
+@findex atomic
+@item atomic
+
+The @code{atomic} option can only be used with pointers.  It informs
+the GC machinery that the memory that the pointer points to does not
+contain any pointers, and hence it should be treated by the GC and PCH
+machinery as an ``atomic'' block of memory that does not need to be
+examined.  In particular, the machinery will not scan that memory for
+pointers to mark them as reachable (when marking pointers for GC) or
+to relocate them (when writing a PCH file).
+
+The @code{atomic} option must be used with great care, because all
+sorts of problem can occur if used incorrectly, that is, if the memory
+the pointer points to does actually contain a pointer.
+
+Here is an example of how to use it:
+@smallexample
+struct GTY(()) my_struct @{
+  int number_of_elements;
+  unsigned int GTY ((atomic)) * elements;
+@};
+@end smallexample
+In this case, @code{elements} is a pointer under GC, and the memory it
+points to needs to be allocated using the Garbage Collector, and will
+be freed automatically by the Garbage Collector when it is no longer
+referenced.  But the memory that the pointer points to is an array of
+@code{unsigned int} elements, and the GC does not need, and indeed
+must not, try to scan it to find pointers to mark or relocate, which
+is why it is marked with the @code{atomic} option.
+
+Note that, currently, global variables can not be marked with
+@code{atomic}; only fields of a struct can.  This is a known
+limitation.  It would be useful to be able to mark global pointers
+with @code{atomic} to make the PCH machinery aware of them so that
+they are saved and restored correctly to PCH files.
+
 @findex special
 @item special ("@var{name}")
 

Index: gengtype.c
===================================================================
--- gengtype.c  (revision 173768)
+++ gengtype.c  (working copy)
@@ -2386,6 +2386,7 @@ walk_type (type_p t, struct walk_type_data *d)
   int maybe_undef_p = 0;
   int use_param_num = -1;
   int use_params_p = 0;
+  int atomic_p = 0;
   options_p oo;
   const struct nested_ptr_data *nested_ptr_d = NULL;
 
@@ -2415,6 +2416,8 @@ walk_type (type_p t, struct walk_type_data *d)
       ;
     else if (strcmp (oo->name, "skip") == 0)
       ;
+    else if (strcmp (oo->name, "atomic") == 0)
+      atomic_p = 1;
     else if (strcmp (oo->name, "default") == 0)
       ;
     else if (strcmp (oo->name, "param_is") == 0)
@@ -2480,6 +2483,12 @@ walk_type (type_p t, struct walk_type_data *d)
       return;
     }
 
+  if (atomic_p && (t->kind != TYPE_POINTER))
+    {
+      error_at_line (d->line, "field `%s' has invalid option `atomic'\n", d->val);
+      return;
+    }
+
   switch (t->kind)
     {
     case TYPE_SCALAR:
@@ -2495,6 +2504,25 @@ walk_type (type_p t, struct walk_type_data *d)
            break;
          }
 
+       /* 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.  */
+       if (atomic_p)
+         {
+           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, "");
+           break;
+         }
+
        if (!length)
          {
            if (!UNION_OR_STRUCT_P (t->u.p)

Re: Patch: New GTY ((atomic)) option

[Gabriel Dos Reis]

On Sun, May 15, 2011 at 7:13 PM, 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
[...]
> This patch basically implements it, but at this stage requires you to explicitly tell gengtype that the
> pointer is atomic (and that is safe for gengtype to ignore the memory it points to).

then should you not name the attribute "ignore"?

-- Gaby

Re: Patch: New GTY ((atomic)) option

[Nathan Froyd]

On 05/15/2011 08:49 PM, Gabriel Dos Reis wrote:
> On Sun, May 15, 2011 at 7:13 PM, 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
> [...]
>> This patch basically implements it, but at this stage requires you to explicitly tell gengtype that the
>> pointer is atomic (and that is safe for gengtype to ignore the memory it points to).
> 
> then should you not name the attribute "ignore"?

Or even the existing attribute "skip"?

-Nathan

Re: Patch: New GTY ((atomic)) option

[Gabriel Dos Reis]

On Sun, May 15, 2011 at 7:52 PM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On 05/15/2011 08:49 PM, Gabriel Dos Reis wrote:
>> On Sun, May 15, 2011 at 7:13 PM, 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
>> [...]
>>> This patch basically implements it, but at this stage requires you to explicitly tell gengtype that the
>>> pointer is atomic (and that is safe for gengtype to ignore the memory it points to).
>>
>> then should you not name the attribute "ignore"?
>
> Or even the existing attribute "skip"?

better, indeed. :-)

-- Gaby

Re: Patch: New GTY ((atomic)) option

[Nicola Pero]

>>> 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
>> [...]
>>> This patch basically implements it, but at this stage requires you to explicitly tell gengtype that the
>>> pointer is atomic (and that is safe for gengtype to ignore the memory it points to).
>>
>> then should you not name the attribute "ignore"?
>
> Or even the existing attribute "skip"?

"skip" is different.  With "skip", the pointer is completely ignored by GC/PCH.  In this case, we don't want
to skip the pointer; we want the pointer itself to be managed by GC/PCH, but the memory *it points to* to not
be scanned for pointers. ;-)

In the example I gave,

struct GTY(()) my_struct {
 ...
 unsigned int * GTY((atomic)) some_ints;
 size_t count;
 ...
};

you'd allocate "some_ints" using, for example, ggc_alloc_atomic_stat(), which already exists in the GC (even
if at the moment there seems to be no particular support for atomic allocation other than the name of that
function, which allocates memory in the same way as for non-atomic allocation), and which would put the pointer
under control of the GC; so it is freed when the GC decides that it is no longer referenced; you don't free
it manually.  That is different from "skip", which would make then pointer simply invisible to GC (you'd allocate
it using malloc()), and you'd have to free it manually (or to never free it).

In practice, when the GC is doing its marking pass, and is marking a structure of type "my_struct", if the
"some_ints" pointer has the option "skip", the GC would not mark it at all; it's ignored.  The option "atomic"
would cause the GC to mark the pointer but ignore what it points to.  The default behaviour is yet different;
it is to examine the memory it points to, mark any pointers in there, and then mark the pointer itself
too.  But because gengtype does not know, at the moment, how to examine unsigned ints (you don't examine them,
because the pointer is atomic!), it will print a error saying that the pointer type is uninmplemented, and abort
(a further step, after introducing the "atomic" option, would be to have the GC automatically mark such pointers
as atomic, as explained in the original post).

To clarify, I didn't invent the word "atomic" - AFAIK it is the standard GC naming convention for memory that contains
no pointers.  It's the name used for this in Boehm GC (the most popular C/C++ GC), where the function is called
GC_MALLOC_ATOMIC(), and it is also the name for it in the GCC GC, presumably, since there already is a function
"ggc_alloc_atomic_stat()" which presumably is meant to allocate atomic memory (hard to say in the absence of
documentation and given that the implementation is identical to the other memory allocation at the moment, but it's
a safe guess).

What is the problem with "atomic" ?  I guess you find it confusing because it makes you think of "atomic access"
to memory ?  You are right that there is that potential for confusion. :-(

We could rename it, but then we'd want to rename the GCC ggc_alloc_atomic_stat() function too, and I'm not entirely
sure it would make anything clearer ... as "atomic" is the standard word for that.  I think the best we can do is
provide good documentation.

So, I guess what I take from your comments is that I should update the documentation in my patch to include
a short discussion of how "atomic" differs from "skip", since it doesn't seem to be that obvious for people. :-)

But please let me know if I'm missing something.

Thanks

Re: Patch: New GTY ((atomic)) option

[Laurynas Biveinis]

2011/5/16 Nicola Pero <nicola.pero@meta-innovation.com>:
> 2011-05-16  Nicola Pero  <nicola.pero@meta-innovation.com>
>
>        * gengtype.c (walk_type): Implemented "atomic" GTY option.
>        * doc/gty.texi (GTY Options): Document "atomic" GTY option.

The patch is OK, with difference between "skip" and "atomic" options
documented. (Can be done as a follow-up patch).

Thanks,
-- 
Laurynas

Re: Patch: New GTY ((atomic)) option

[Nicola Pero]

>>        * gengtype.c (walk_type): Implemented "atomic" GTY option.
>>        * doc/gty.texi (GTY Options): Document "atomic" GTY option.
>
> The patch is OK, with difference between "skip" and "atomic" options
> documented. (Can be done as a follow-up patch).

Thanks for the quick review.  Here's an updated patch with revised
documentation.

Ok to go ?

Thanks

Index: gcc/doc/gty.texi
===================================================================
--- gcc/doc/gty.texi    (revision 173917)
+++ gcc/doc/gty.texi    (working copy)
@@ -383,6 +383,51 @@
   size_t size = sizeof (struct sorted_fields_type) + n * sizeof (tree);
 @end smallexample
 
+@findex atomic
+@item atomic
+
+The @code{atomic} option can only be used with pointers.  It informs
+the GC machinery that the memory that the pointer points to does not
+contain any pointers, and hence it should be treated by the GC and PCH
+machinery as an ``atomic'' block of memory that does not need to be
+examined when scanning memory for pointers.  In particular, the
+machinery will not scan that memory for pointers to mark them as
+reachable (when marking pointers for GC) or to relocate them (when
+writing a PCH file).
+
+The @code{atomic} option differs from the @code{skip} option.
+@code{atomic} keeps the memory under Garbage Collection, but makes the
+GC ignore the contents of the memory.  @code{skip} is more drastic in
+that it causes the pointer and the memory to be completely ignored by
+the Garbage Collector.  So, memory marked as @code{atomic} is
+automatically freed when no longer reachable, while memory marked as
+@code{skip} is not.
+
+The @code{atomic} option must be used with great care, because all
+sorts of problem can occur if used incorrectly, that is, if the memory
+the pointer points to does actually contain a pointer.
+
+Here is an example of how to use it:
+@smallexample
+struct GTY(()) my_struct @{
+  int number_of_elements;
+  unsigned int GTY ((atomic)) * elements;
+@};
+@end smallexample
+In this case, @code{elements} is a pointer under GC, and the memory it
+points to needs to be allocated using the Garbage Collector, and will
+be freed automatically by the Garbage Collector when it is no longer
+referenced.  But the memory that the pointer points to is an array of
+@code{unsigned int} elements, and the GC must not try to scan it to
+find pointers to mark or relocate, which is why it is marked with the
+@code{atomic} option.
+
+Note that, currently, global variables can not be marked with
+@code{atomic}; only fields of a struct can.  This is a known
+limitation.  It would be useful to be able to mark global pointers
+with @code{atomic} to make the PCH machinery aware of them so that
+they are saved and restored correctly to PCH files.
+
 @findex special
 @item special ("@var{name}")
 
Index: gcc/gengtype.c
===================================================================
--- gcc/gengtype.c      (revision 173917)
+++ gcc/gengtype.c      (working copy)
@@ -2386,6 +2386,7 @@
   int maybe_undef_p = 0;
   int use_param_num = -1;
   int use_params_p = 0;
+  int atomic_p = 0;
   options_p oo;
   const struct nested_ptr_data *nested_ptr_d = NULL;
 
@@ -2415,6 +2416,8 @@
       ;
     else if (strcmp (oo->name, "skip") == 0)
       ;
+    else if (strcmp (oo->name, "atomic") == 0)
+      atomic_p = 1;
     else if (strcmp (oo->name, "default") == 0)
       ;
     else if (strcmp (oo->name, "param_is") == 0)
@@ -2480,6 +2483,12 @@
       return;
     }
 
+  if (atomic_p && (t->kind != TYPE_POINTER))
+    {
+      error_at_line (d->line, "field `%s' has invalid option `atomic'\n", d->val);
+      return;
+    }
+
   switch (t->kind)
     {
     case TYPE_SCALAR:
@@ -2495,6 +2504,25 @@
            break;
          }
 
+       /* 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.  */
+       if (atomic_p)
+         {
+           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, "");
+           break;
+         }
+
        if (!length)
          {
            if (!UNION_OR_STRUCT_P (t->u.p)
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog       (revision 173917)
+++ gcc/ChangeLog       (working copy)
@@ -1,3 +1,8 @@
+2011-05-20  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * gengtype.c (walk_type): Implemented "atomic" GTY option.
+       * doc/gty.texi (GTY Options): Document "atomic" GTY option.
+
 2011-05-19  Joseph Myers  <joseph@codesourcery.com>
 
        * config/arm/arm-fpus.def: New.

Re: Patch: New GTY ((atomic)) option

[Basile Starynkevitch]

On Thu, 19 May 2011 20:10:35 +0200 (CEST)
"Nicola Pero" <nicola.pero@meta-innovation.com> wrote:

> >>        * gengtype.c (walk_type): Implemented "atomic" GTY option.
> >>        * doc/gty.texi (GTY Options): Document "atomic" GTY option.
> >
> > The patch is OK, with difference between "skip" and "atomic" options
> > documented. (Can be done as a follow-up patch).
> 
> Thanks for the quick review.  Here's an updated patch with revised
> documentation.
> 
> Ok to go ?

I have no power to approve that, but I find the patch quite nice.

However, did you check that the atomic qualifier is correctly written &
re-read in the state (I believe you did, otherwise it probably won't
work). This is needed for plugins using it, or using atomic qualified
fields of existing (or future) structures.

Regards
-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mine, sont seulement les miennes} ***

Re: Patch: New GTY ((atomic)) option

[Basile Starynkevitch]

On Thu, 19 May 2011 21:45:49 +0200
Basile Starynkevitch <basile@starynkevitch.net> wrote:
> However, did you check that the atomic qualifier is correctly written &
> re-read in the state (I believe you did, otherwise it probably won't
> work). This is needed for plugins using it, or using atomic qualified
> fields of existing (or future) structures.

This also brings a wish. Now that gengtype is plugin friendly, I would
find great if some testcases appeared for gengtype on plugins. That
would be the good way to test new GTY or gengtype features.
Unfortunately, this probably requires fluency with dejagnu & expect,
which I don't have. The dream I have is to be able to test with dejagnu
that gengtype -P is able to generate some gt*.h file with such and such
regexp patterns.

To say it otherwise, testing new GTY features is best done on
plugin-like code, because these new featurres cannot be used before
gengtype has been properly extended. That "plugin" code don't need to
do something useful, or even to work as a GCC plugin; it simply have to
be passed to gengtype with -P.

Regards.
-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mine, sont seulement les miennes} ***

Re: Patch: New GTY ((atomic)) option

[Laurynas Biveinis]

2011/5/19 Nicola Pero <nicola.pero@meta-innovation.com>:
> Ok to go ?

> +2011-05-20  Nicola Pero  <nicola.pero@meta-innovation.com>
> +
> +       * gengtype.c (walk_type): Implemented "atomic" GTY option.
> +       * doc/gty.texi (GTY Options): Document "atomic" GTY option.

I assume it received the usual testing? If yes, then OK. Thanks!

-- 
Laurynas

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

[Thomas Schwinge]

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

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-GGC-GTY-No-pointer-walking-for-atomic-in-PCH-gt_pch_.patch --]
[-- Type: text/x-diff, Size: 7744 bytes --]

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

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

[Richard Biener]

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

Re: Patch: New GTY ((atomic)) option

[Nicola Pero]

> Yes why can't you use vec(int,gc)?

Can you use that in libiberty ?  I thought it was a GCC-only thing ?

Thanks

Re: Patch: New GTY ((atomic)) option

[Nicola Pero]

> However, did you check that the atomic qualifier is correctly written &
> re-read in the state (I believe you did, otherwise it probably won't
> work). This is needed for plugins using it, or using atomic qualified
> fields of existing (or future) structures.

Yes.  String options are written as they are, no particular changes are
needed. ;-)

I'd like to show you a testcase, but, as you discuss in your other email, we can't
really write GTY testcases at this stage.  So here is a walked tour of how to test
the new atomic option --

 * apply the GTY ((atomic)) patch (obviously)

 * add the "GTY ((atomic))" option to field 'su' of struct 'function' in function.h
(that field is atomic, so you can mark it "atomic" and it should work)

 * rebuild everything from scratch (bootstrap the compiler)

 * gtype.state now contains, some time after (!pair "su", the lines

(!srcfileloc  "function.h" 516)

(!options
(!option atomic string  "")
)

btw, we should probably improve gtype.state; indentation would be nice :-) ...

 * gtype-desc.c now contains

void
gt_ggc_mx_function (void *x_p)
{
  struct function * const x = (struct function *)x_p;
  if (ggc_test_and_set_mark (x))
    {
      gt_ggc_m_9eh_status ((*x).eh);
      gt_ggc_m_18control_flow_graph ((*x).cfg);
      gt_ggc_m_12gimple_seq_d ((*x).gimple_body);
      gt_ggc_m_9gimple_df ((*x).gimple_df);
      gt_ggc_m_5loops ((*x).x_current_loops);
      if ((*x).su != NULL) {
        ggc_mark ((*x).su);
      }
      gt_ggc_m_9tree_node ((*x).decl);
      gt_ggc_m_9tree_node ((*x).static_chain_decl);
      gt_ggc_m_9tree_node ((*x).nonlocal_goto_save_area);
      gt_ggc_m_11VEC_tree_gc ((*x).local_decls);
      gt_ggc_m_16machine_function ((*x).machine);
      gt_ggc_m_17language_function ((*x).language);
      gt_ggc_m_P9tree_node4htab ((*x).used_types_hash);
    }
}

as you notice, the "su" field is only marked, but not followed.  Normally that same marking
function would be

void
gt_ggc_mx_function (void *x_p)
{
  struct function * const x = (struct function *)x_p;
  if (ggc_test_and_set_mark (x))
    {
      gt_ggc_m_9eh_status ((*x).eh);
      gt_ggc_m_18control_flow_graph ((*x).cfg);
      gt_ggc_m_12gimple_seq_d ((*x).gimple_body);
      gt_ggc_m_9gimple_df ((*x).gimple_df);
      gt_ggc_m_5loops ((*x).x_current_loops);
      gt_ggc_m_11stack_usage ((*x).su);
      gt_ggc_m_9tree_node ((*x).decl);
      gt_ggc_m_9tree_node ((*x).static_chain_decl);
      gt_ggc_m_9tree_node ((*x).nonlocal_goto_save_area);
      gt_ggc_m_11VEC_tree_gc ((*x).local_decls);
      gt_ggc_m_16machine_function ((*x).machine);
      gt_ggc_m_17language_function ((*x).language);
      gt_ggc_m_P9tree_node4htab ((*x).used_types_hash);
    }
}

which would call the gt_ggc_mx_stack_usage() function, which does nothing but mark the pointer in this case
(since the field actually is atomic), but would in other cases mark the children as well.  So, in this case
marking the field as "atomic" results in a small optimization when doing the GC marking stage.  In other cases,
when gengtype doesn't know how to inspect the memory pointed to (eg, "unsigned int *"), then it would make it
possible to use GC for the pointer.

PCH traversing is similar.

To test that the GC marking actually works, I mostly bootstrap the compiler first, then test compiling a few
testcases using --param ggc-min-expand=0 --param ggc-min-heapsize=0.  This is by no means a full test; if you
or Laurynas have any suggestions on how to test better, they'd be welcome. :-)

Thanks

Re: Patch: New GTY ((atomic)) option

[Laurynas Biveinis]

2011/5/20 Nicola Pero <nicola.pero@meta-innovation.com>:
> I'd like to show you a testcase, but, as you discuss in your other email, we can't
> really write GTY testcases at this stage.  So here is a walked tour of how to test
> the new atomic option --
[...]
> To test that the GC marking actually works, I mostly bootstrap the compiler first, then test compiling a few
> testcases using --param ggc-min-expand=0 --param ggc-min-heapsize=0.  This is by no means a full test; if you
> or Laurynas have any suggestions on how to test better, they'd be welcome. :-)

I think this is about the best GTY testing one can do right now. We
ought to have some support in testsuite (not only for plugins, so more
like unit tests), but well, currently we don't.

-- 
Laurynas