libcpp: Improve the diagnostic for poisoned identifiers [PR36887]

Hello-

This patch implements the PR's request to add more information to the
diagnostic issued for using a poisoned identifier. Bootstrapped + regtested
all languages on x86-64 Linux. Does it look OK please? Thanks!

-Lewis

-- >8 --

The PR requests an enhancement to the diagnostic issued for the use of a
poisoned identifier. Currently, we show the location of the usage, but not
the location which requested the poisoning, which would be helpful for the
user if the decision to poison an identifier was made externally, such as
in a library header.

In order to output this information, we need to remember a location_t for
each identifier that has been poisoned, and that data needs to be preserved
as well in a PCH. One option would be to add a field to struct cpp_hashnode,
but there is no convenient place to add it without increasing the size of
the struct for all identifiers. Given this facility will be needed rarely,
it seemed better to add a second hash map, which is handled PCH-wise the
same as the current one in gcc/stringpool.cc. This hash map associates a new
struct cpp_hashnode_extra with each identifier that needs one. Currently
that struct only contains the new location_t, but it could be extended in
the future if there is other ancillary data that may be convenient to put
there for other purposes.

libcpp/ChangeLog:

	PR preprocessor/36887
	* directives.cc (do_pragma_poison): Store in the extra hash map the
	location from which an identifier has been poisoned.
	* lex.cc (identifier_diagnostics_on_lex): When issuing a diagnostic
	for the use of a poisoned identifier, also add a note indicating the
	location from which it was poisoned.
	* identifiers.cc (alloc_node): Convert to template function.
	(_cpp_init_hashtable): Handle the new extra hash map.
	(_cpp_destroy_hashtable): Likewise.
	* include/cpplib.h (struct cpp_hashnode_extra): New struct.
	(cpp_create_reader): Update prototype to...
	* init.cc (cpp_create_reader): ...accept an argument for the extra
	hash table and pass it to _cpp_init_hashtable.
	* include/symtab.h (ht_lookup): New overload for convenience.
	* internal.h (struct cpp_reader): Add EXTRA_HASH_TABLE member.
	(_cpp_init_hashtable): Adjust prototype.

gcc/c-family/ChangeLog:

	PR preprocessor/36887
	* c-opts.cc (c_common_init_options): Pass new extra hash map
	argument to cpp_create_reader().

gcc/ChangeLog:

	PR preprocessor/36887
	* toplev.h (ident_hash_extra): Declare...
	* stringpool.cc (ident_hash_extra): ...this new global variable.
	(init_stringpool): Handle ident_hash_extra as well as ident_hash.
	(ggc_mark_stringpool): Likewise.
	(ggc_purge_stringpool): Likewise.
	(struct string_pool_data_extra): New struct.
	(spd2): New GC root variable.
	(gt_pch_save_stringpool): Use spd2 to handle ident_hash_extra,
	analogous to how spd is used to handle ident_hash.
	(gt_pch_restore_stringpool): Likewise.

gcc/testsuite/ChangeLog:

	PR preprocessor/36887
	* c-c++-common/cpp/diagnostic-poison.c: New test.
	* g++.dg/pch/pr36887.C: New test.
	* g++.dg/pch/pr36887.Hs: New test.
---
 libcpp/directives.cc                          |  3 ++
 libcpp/identifiers.cc                         | 42 +++++++++++------
 libcpp/include/cpplib.h                       | 21 ++++++---
 libcpp/include/symtab.h                       |  6 +++
 libcpp/init.cc                                |  4 +-
 libcpp/internal.h                             |  8 +++-
 libcpp/lex.cc                                 | 10 ++++-
 gcc/c-family/c-opts.cc                        |  2 +-
 gcc/stringpool.cc                             | 45 +++++++++++++++++++
 gcc/toplev.h                                  |  3 +-
 .../c-c++-common/cpp/diagnostic-poison.c      | 13 ++++++
 gcc/testsuite/g++.dg/pch/pr36887.C            |  3 ++
 gcc/testsuite/g++.dg/pch/pr36887.Hs           |  1 +
 13 files changed, 134 insertions(+), 27 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/cpp/diagnostic-poison.c
 create mode 100644 gcc/testsuite/g++.dg/pch/pr36887.C
 create mode 100644 gcc/testsuite/g++.dg/pch/pr36887.Hs

On Wed, 2023-09-20 at 00:12 -0400, Lewis Hyatt wrote:
> Hello-
> 
> This patch implements the PR's request to add more information to the
> diagnostic issued for using a poisoned identifier. Bootstrapped +
> regtested
> all languages on x86-64 Linux. Does it look OK please? Thanks!

Thanks!

Patch looks good to me; please go ahead and push it.

Dave

Hi!

On Wed, 20 Sept 2023 at 06:12, Lewis Hyatt <lhyatt@gmail.com> wrote:
>
> Hello-
>
> This patch implements the PR's request to add more information to the
> diagnostic issued for using a poisoned identifier. Bootstrapped + regtested
> all languages on x86-64 Linux. Does it look OK please? Thanks!
>
> -Lewis
>
> -- >8 --
>
> The PR requests an enhancement to the diagnostic issued for the use of a
> poisoned identifier. Currently, we show the location of the usage, but not
> the location which requested the poisoning, which would be helpful for the
> user if the decision to poison an identifier was made externally, such as
> in a library header.
>
> In order to output this information, we need to remember a location_t for
> each identifier that has been poisoned, and that data needs to be preserved
> as well in a PCH. One option would be to add a field to struct cpp_hashnode,
> but there is no convenient place to add it without increasing the size of
> the struct for all identifiers. Given this facility will be needed rarely,
> it seemed better to add a second hash map, which is handled PCH-wise the
> same as the current one in gcc/stringpool.cc. This hash map associates a new
> struct cpp_hashnode_extra with each identifier that needs one. Currently
> that struct only contains the new location_t, but it could be extended in
> the future if there is other ancillary data that may be convenient to put
> there for other purposes.
>
> libcpp/ChangeLog:
>
>         PR preprocessor/36887
>         * directives.cc (do_pragma_poison): Store in the extra hash map the
>         location from which an identifier has been poisoned.
>         * lex.cc (identifier_diagnostics_on_lex): When issuing a diagnostic
>         for the use of a poisoned identifier, also add a note indicating the
>         location from which it was poisoned.
>         * identifiers.cc (alloc_node): Convert to template function.
>         (_cpp_init_hashtable): Handle the new extra hash map.
>         (_cpp_destroy_hashtable): Likewise.
>         * include/cpplib.h (struct cpp_hashnode_extra): New struct.
>         (cpp_create_reader): Update prototype to...
>         * init.cc (cpp_create_reader): ...accept an argument for the extra
>         hash table and pass it to _cpp_init_hashtable.
>         * include/symtab.h (ht_lookup): New overload for convenience.
>         * internal.h (struct cpp_reader): Add EXTRA_HASH_TABLE member.
>         (_cpp_init_hashtable): Adjust prototype.
>
> gcc/c-family/ChangeLog:
>
>         PR preprocessor/36887
>         * c-opts.cc (c_common_init_options): Pass new extra hash map
>         argument to cpp_create_reader().
>
> gcc/ChangeLog:
>
>         PR preprocessor/36887
>         * toplev.h (ident_hash_extra): Declare...
>         * stringpool.cc (ident_hash_extra): ...this new global variable.
>         (init_stringpool): Handle ident_hash_extra as well as ident_hash.
>         (ggc_mark_stringpool): Likewise.
>         (ggc_purge_stringpool): Likewise.
>         (struct string_pool_data_extra): New struct.
>         (spd2): New GC root variable.
>         (gt_pch_save_stringpool): Use spd2 to handle ident_hash_extra,
>         analogous to how spd is used to handle ident_hash.
>         (gt_pch_restore_stringpool): Likewise.
>
> gcc/testsuite/ChangeLog:
>
>         PR preprocessor/36887
>         * c-c++-common/cpp/diagnostic-poison.c: New test.
>         * g++.dg/pch/pr36887.C: New test.
>         * g++.dg/pch/pr36887.Hs: New test.

We have noticed that the new tests fail on aarch64 with:
.../aarch64-unknown-linux-gnu/libc/usr/lib/crt1.o: in function `_start':
.../sysdeps/aarch64/start.S:110:(.text+0x38): undefined reference to `main'

Looking at the test, I'd say it lacks a dg-do compile (to avoid
linking), but how does it work on other targets?

Thanks,

Christophe

> ---
>  libcpp/directives.cc                          |  3 ++
>  libcpp/identifiers.cc                         | 42 +++++++++++------
>  libcpp/include/cpplib.h                       | 21 ++++++---
>  libcpp/include/symtab.h                       |  6 +++
>  libcpp/init.cc                                |  4 +-
>  libcpp/internal.h                             |  8 +++-
>  libcpp/lex.cc                                 | 10 ++++-
>  gcc/c-family/c-opts.cc                        |  2 +-
>  gcc/stringpool.cc                             | 45 +++++++++++++++++++
>  gcc/toplev.h                                  |  3 +-
>  .../c-c++-common/cpp/diagnostic-poison.c      | 13 ++++++
>  gcc/testsuite/g++.dg/pch/pr36887.C            |  3 ++
>  gcc/testsuite/g++.dg/pch/pr36887.Hs           |  1 +
>  13 files changed, 134 insertions(+), 27 deletions(-)
>  create mode 100644 gcc/testsuite/c-c++-common/cpp/diagnostic-poison.c
>  create mode 100644 gcc/testsuite/g++.dg/pch/pr36887.C
>  create mode 100644 gcc/testsuite/g++.dg/pch/pr36887.Hs
>
> diff --git a/libcpp/directives.cc b/libcpp/directives.cc
> index ee5419d1f40..c5c938fda1d 100644
> --- a/libcpp/directives.cc
> +++ b/libcpp/directives.cc
> @@ -1737,6 +1737,9 @@ do_pragma_poison (cpp_reader *pfile)
>                    NODE_NAME (hp));
>        _cpp_free_definition (hp);
>        hp->flags |= NODE_POISONED | NODE_DIAGNOSTIC;
> +      const auto data = (cpp_hashnode_extra *)
> +       ht_lookup (pfile->extra_hash_table, hp->ident, HT_ALLOC);
> +      data->poisoned_loc = tok->src_loc;
>      }
>    pfile->state.poisoned_ok = 0;
>  }
> diff --git a/libcpp/identifiers.cc b/libcpp/identifiers.cc
> index 7eccaa9bfd3..10cbbdf703d 100644
> --- a/libcpp/identifiers.cc
> +++ b/libcpp/identifiers.cc
> @@ -27,24 +27,22 @@ along with this program; see the file COPYING3.  If not see
>  #include "cpplib.h"
>  #include "internal.h"
>
> -static hashnode alloc_node (cpp_hash_table *);
> -
>  /* Return an identifier node for hashtable.c.  Used by cpplib except
>     when integrated with the C front ends.  */
> +template<typename Node>
>  static hashnode
>  alloc_node (cpp_hash_table *table)
>  {
> -  cpp_hashnode *node;
> -
> -  node = XOBNEW (&table->pfile->hash_ob, cpp_hashnode);
> -  memset (node, 0, sizeof (cpp_hashnode));
> +  const auto node = XOBNEW (&table->pfile->hash_ob, Node);
> +  memset (node, 0, sizeof (Node));
>    return HT_NODE (node);
>  }
>
>  /* Set up the identifier hash table.  Use TABLE if non-null, otherwise
>     create our own.  */
>  void
> -_cpp_init_hashtable (cpp_reader *pfile, cpp_hash_table *table)
> +_cpp_init_hashtable (cpp_reader *pfile, cpp_hash_table *table,
> +                    cpp_hash_table *extra_table)
>  {
>    struct spec_nodes *s;
>
> @@ -52,13 +50,23 @@ _cpp_init_hashtable (cpp_reader *pfile, cpp_hash_table *table)
>      {
>        pfile->our_hashtable = 1;
>        table = ht_create (13);  /* 8K (=2^13) entries.  */
> -      table->alloc_node = alloc_node;
> +      table->alloc_node = alloc_node<cpp_hashnode>;
> +    }
>
> -      obstack_specify_allocation (&pfile->hash_ob, 0, 0, xmalloc, free);
> +  if (extra_table == NULL)
> +    {
> +      pfile->our_extra_hashtable = true;
> +      extra_table = ht_create (6); /* 64 entries.  */
> +      extra_table->alloc_node = alloc_node<cpp_hashnode_extra>;
>      }
>
> +  if (pfile->our_hashtable || pfile->our_extra_hashtable)
> +    obstack_specify_allocation (&pfile->hash_ob, 0, 0, xmalloc, free);
> +
>    table->pfile = pfile;
> +  extra_table->pfile = pfile;
>    pfile->hash_table = table;
> +  pfile->extra_hash_table = extra_table;
>
>    /* Now we can initialize things that use the hash table.  */
>    _cpp_init_directives (pfile);
> @@ -80,10 +88,11 @@ void
>  _cpp_destroy_hashtable (cpp_reader *pfile)
>  {
>    if (pfile->our_hashtable)
> -    {
> -      ht_destroy (pfile->hash_table);
> -      obstack_free (&pfile->hash_ob, 0);
> -    }
> +    ht_destroy (pfile->hash_table);
> +  if (pfile->our_extra_hashtable)
> +    ht_destroy (pfile->extra_hash_table);
> +  if (pfile->our_hashtable || pfile->our_extra_hashtable)
> +    obstack_free (&pfile->hash_ob, 0);
>  }
>
>  /* Returns the hash entry for the STR of length LEN, creating one
> @@ -110,7 +119,12 @@ cpp_defined (cpp_reader *pfile, const unsigned char *str, int len)
>  /* We don't need a proxy since the hash table's identifier comes first
>     in cpp_hashnode.  However, in case this is ever changed, we have a
>     static assertion for it.  */
> -extern char proxy_assertion_broken[offsetof (struct cpp_hashnode, ident) == 0 ? 1 : -1];
> +static_assert (offsetof (cpp_hashnode, ident) == 0,
> +              "struct cpp_hashnode must have a struct ht_identifier as"
> +              " its first member");
> +static_assert (offsetof (cpp_hashnode_extra, ident) == 0,
> +              "struct cpp_hashnode_extra must have a struct ht_identifier as"
> +              " its first member");
>
>  /* For all nodes in the hashtable, callback CB with parameters PFILE,
>     the node, and V.  */
> diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
> index fcdaf082b09..175d408182a 100644
> --- a/libcpp/include/cpplib.h
> +++ b/libcpp/include/cpplib.h
> @@ -1003,6 +1003,14 @@ struct GTY(()) cpp_hashnode {
>    union _cpp_hashnode_value GTY ((desc ("%1.type"))) value;
>  };
>
> +/* Extra information we may need to store per identifier, which is needed rarely
> +   enough that it's not worth adding directly into the main identifier hash.  */
> +struct GTY(()) cpp_hashnode_extra
> +{
> +  struct ht_identifier ident;
> +  location_t poisoned_loc;
> +};
> +
>  /* A class for iterating through the source locations within a
>     string token (before escapes are interpreted, and before
>     concatenation).  */
> @@ -1049,12 +1057,15 @@ class cpp_substring_ranges
>
>  /* Call this first to get a handle to pass to other functions.
>
> -   If you want cpplib to manage its own hashtable, pass in a NULL
> -   pointer.  Otherwise you should pass in an initialized hash table
> -   that cpplib will share; this technique is used by the C front
> -   ends.  */
> +   The first hash table argument is for associating a struct cpp_hashnode
> +   with each identifier.  The second hash table argument is for associating
> +   a struct cpp_hashnode_extra with each identifier that needs one.  For
> +   either, pass in a NULL pointer if you want cpplib to create and manage
> +   the hash table itself, or else pass a suitably initialized hash table to
> +   be managed external to libcpp, as is done by the C-family frontends.  */
>  extern cpp_reader *cpp_create_reader (enum c_lang, struct ht *,
> -                                     class line_maps *);
> +                                     class line_maps *,
> +                                     struct ht * = nullptr);
>
>  /* Reset the cpp_reader's line_map.  This is only used after reading a
>     PCH file.  */
> diff --git a/libcpp/include/symtab.h b/libcpp/include/symtab.h
> index 0c713f2ad30..4a2370e02a6 100644
> --- a/libcpp/include/symtab.h
> +++ b/libcpp/include/symtab.h
> @@ -81,6 +81,12 @@ extern hashnode ht_lookup (cpp_hash_table *, const unsigned char *,
>  extern hashnode ht_lookup_with_hash (cpp_hash_table *, const unsigned char *,
>                                       size_t, unsigned int,
>                                       enum ht_lookup_option);
> +inline hashnode ht_lookup (cpp_hash_table *ht, const ht_identifier &id,
> +                          ht_lookup_option opt)
> +{
> +  return ht_lookup_with_hash (ht, id.str, id.len, id.hash_value, opt);
> +}
> +
>  #define HT_HASHSTEP(r, c) ((r) * 67 + ((c) - 113));
>  #define HT_HASHFINISH(r, len) ((r) + (len))
>
> diff --git a/libcpp/init.cc b/libcpp/init.cc
> index 693feaa31ed..f8d0a8edc8c 100644
> --- a/libcpp/init.cc
> +++ b/libcpp/init.cc
> @@ -191,7 +191,7 @@ init_library (void)
>  /* Initialize a cpp_reader structure.  */
>  cpp_reader *
>  cpp_create_reader (enum c_lang lang, cpp_hash_table *table,
> -                  class line_maps *line_table)
> +                  class line_maps *line_table, cpp_hash_table *extra_table)
>  {
>    cpp_reader *pfile;
>
> @@ -307,7 +307,7 @@ cpp_create_reader (enum c_lang lang, cpp_hash_table *table,
>
>    _cpp_init_files (pfile);
>
> -  _cpp_init_hashtable (pfile, table);
> +  _cpp_init_hashtable (pfile, table, extra_table);
>
>    return pfile;
>  }
> diff --git a/libcpp/internal.h b/libcpp/internal.h
> index 8b74d10c1a3..9b034b76a3b 100644
> --- a/libcpp/internal.h
> +++ b/libcpp/internal.h
> @@ -555,6 +555,9 @@ struct cpp_reader
>    /* Identifier hash table.  */
>    struct ht *hash_table;
>
> +  /* Identifier ancillary data hash table.  */
> +  struct ht *extra_hash_table;
> +
>    /* Expression parser stack.  */
>    struct op *op_stack, *op_limit;
>
> @@ -566,7 +569,7 @@ struct cpp_reader
>    struct spec_nodes spec_nodes;
>
>    /* Whether cpplib owns the hashtable.  */
> -  bool our_hashtable;
> +  bool our_hashtable, our_extra_hashtable;
>
>    /* Traditional preprocessing output buffer (a logical line).  */
>    struct
> @@ -704,7 +707,8 @@ extern void _cpp_push_token_context (cpp_reader *, cpp_hashnode *,
>  extern void _cpp_backup_tokens_direct (cpp_reader *, unsigned int);
>
>  /* In identifiers.cc */
> -extern void _cpp_init_hashtable (cpp_reader *, cpp_hash_table *);
> +extern void
> +_cpp_init_hashtable (cpp_reader *, cpp_hash_table *, cpp_hash_table *);
>  extern void _cpp_destroy_hashtable (cpp_reader *);
>
>  /* In files.cc */
> diff --git a/libcpp/lex.cc b/libcpp/lex.cc
> index 8dea4d3d4eb..f1fd7e74336 100644
> --- a/libcpp/lex.cc
> +++ b/libcpp/lex.cc
> @@ -2168,8 +2168,14 @@ identifier_diagnostics_on_lex (cpp_reader *pfile, cpp_hashnode *node)
>
>    /* It is allowed to poison the same identifier twice.  */
>    if ((node->flags & NODE_POISONED) && !pfile->state.poisoned_ok)
> -    cpp_error (pfile, CPP_DL_ERROR, "attempt to use poisoned \"%s\"",
> -              NODE_NAME (node));
> +    {
> +      cpp_error (pfile, CPP_DL_ERROR, "attempt to use poisoned \"%s\"",
> +                NODE_NAME (node));
> +      const auto data = (cpp_hashnode_extra *)
> +       ht_lookup (pfile->extra_hash_table, node->ident, HT_NO_INSERT);
> +      if (data && data->poisoned_loc)
> +       cpp_error_at (pfile, CPP_DL_NOTE, data->poisoned_loc, "poisoned here");
> +    }
>
>    /* Constraint 6.10.3.5: __VA_ARGS__ should only appear in the
>       replacement list of a variadic macro.  */
> diff --git a/gcc/c-family/c-opts.cc b/gcc/c-family/c-opts.cc
> index d9f55f45e03..a00cdcc6d5c 100644
> --- a/gcc/c-family/c-opts.cc
> +++ b/gcc/c-family/c-opts.cc
> @@ -235,7 +235,7 @@ c_common_init_options (unsigned int decoded_options_count,
>      = new (ggc_alloc <string_concat_db> ()) string_concat_db ();
>
>    parse_in = cpp_create_reader (c_dialect_cxx () ? CLK_GNUCXX: CLK_GNUC89,
> -                               ident_hash, line_table);
> +                               ident_hash, line_table, ident_hash_extra);
>    cb = cpp_get_callbacks (parse_in);
>    cb->diagnostic = c_cpp_diagnostic;
>
> diff --git a/gcc/stringpool.cc b/gcc/stringpool.cc
> index 8658e6ab52a..38489677bf3 100644
> --- a/gcc/stringpool.cc
> +++ b/gcc/stringpool.cc
> @@ -29,8 +29,10 @@ along with GCC; see the file COPYING3.  If not see
>  #include "system.h"
>  #include "coretypes.h"
>  #include "tree.h"
> +#include "cpplib.h"
>
>  struct ht *ident_hash;
> +struct ht *ident_hash_extra;
>
>  static hashnode alloc_node (cpp_hash_table *);
>  static int mark_ident (struct cpp_reader *, hashnode, const void *);
> @@ -49,11 +51,21 @@ init_stringpool (void)
>       (We can't make this idempotent since identifiers contain state) */
>    if (ident_hash)
>      ht_destroy (ident_hash);
> +  if (ident_hash_extra)
> +    ht_destroy (ident_hash_extra);
>
>    /* Create with 16K (2^14) entries.  */
>    ident_hash = ht_create (14);
>    ident_hash->alloc_node = alloc_node;
>    ident_hash->alloc_subobject = stringpool_ggc_alloc;
> +
> +  /* Create with 64 (2^6) entries.  */
> +  ident_hash_extra = ht_create (6);
> +  ident_hash_extra->alloc_node = [] (cpp_hash_table *)
> +  {
> +    return HT_NODE (ggc_cleared_alloc<cpp_hashnode_extra> ());
> +  };
> +  ident_hash_extra->alloc_subobject = stringpool_ggc_alloc;
>  }
>
>  /* Allocate a hash node.  */
> @@ -166,6 +178,12 @@ void
>  ggc_mark_stringpool (void)
>  {
>    ht_forall (ident_hash, mark_ident, NULL);
> +  ht_forall (ident_hash_extra,
> +            [] (cpp_reader *, hashnode h, const void *)
> +            {
> +              gt_ggc_m_18cpp_hashnode_extra (h);
> +              return 1;
> +            }, nullptr);
>  }
>
>  /* Purge the identifier hash of identifiers which are no longer
> @@ -175,6 +193,11 @@ void
>  ggc_purge_stringpool (void)
>  {
>    ht_purge (ident_hash, maybe_delete_ident, NULL);
> +  ht_purge (ident_hash_extra,
> +           [] (cpp_reader *, hashnode h, const void *) -> int
> +           {
> +             return !ggc_marked_p (h);
> +           }, nullptr);
>  }
>
>  /* Pointer-walking routine for strings (not very interesting, since
> @@ -251,7 +274,19 @@ struct GTY(()) string_pool_data {
>    unsigned int nelements;
>  };
>
> +struct GTY (()) string_pool_data_extra
> +{
> +  ht_identifier_ptr *
> +    GTY((length ("%h.nslots"),
> +        nested_ptr (cpp_hashnode_extra, "%h ? HT_NODE (%h) : nullptr",
> +                    "(cpp_hashnode_extra *)%h")))
> +    entries;
> +  unsigned int nslots;
> +  unsigned int nelements;
> +};
> +
>  static GTY(()) struct string_pool_data * spd;
> +static GTY(()) struct string_pool_data_extra *spd2;
>
>  /* Save the stringpool data in SPD.  */
>
> @@ -264,6 +299,13 @@ gt_pch_save_stringpool (void)
>    spd->entries = ggc_vec_alloc<ht_identifier_ptr> (spd->nslots);
>    memcpy (spd->entries, ident_hash->entries,
>           spd->nslots * sizeof (spd->entries[0]));
> +
> +  spd2 = ggc_alloc<string_pool_data_extra> ();
> +  spd2->nslots = ident_hash_extra->nslots;
> +  spd2->nelements = ident_hash_extra->nelements;
> +  spd2->entries = ggc_vec_alloc<ht_identifier_ptr> (spd2->nslots);
> +  memcpy (spd2->entries, ident_hash_extra->entries,
> +         spd2->nslots * sizeof (spd2->entries[0]));
>  }
>
>  /* Return the stringpool to its state before gt_pch_save_stringpool
> @@ -281,7 +323,10 @@ void
>  gt_pch_restore_stringpool (void)
>  {
>    ht_load (ident_hash, spd->entries, spd->nslots, spd->nelements, false);
> +  ht_load (ident_hash_extra, spd2->entries, spd2->nslots, spd2->nelements,
> +          false);
>    spd = NULL;
> +  spd2 = NULL;
>  }
>
>  #include "gt-stringpool.h"
> diff --git a/gcc/toplev.h b/gcc/toplev.h
> index 981112df113..7150665aa24 100644
> --- a/gcc/toplev.h
> +++ b/gcc/toplev.h
> @@ -81,8 +81,9 @@ extern int flag_rerun_cse_after_global_opts;
>
>  extern void print_version (FILE *, const char *, bool);
>
> -/* The hashtable, so that the C front ends can pass it to cpplib.  */
> +/* The hashtables, so that the C front ends can pass them to cpplib.  */
>  extern struct ht *ident_hash;
> +extern struct ht *ident_hash_extra;
>
>  /* Functions used to get and set GCC's notion of in what directory
>     compilation was started.  */
> diff --git a/gcc/testsuite/c-c++-common/cpp/diagnostic-poison.c b/gcc/testsuite/c-c++-common/cpp/diagnostic-poison.c
> new file mode 100644
> index 00000000000..294f77e615f
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/cpp/diagnostic-poison.c
> @@ -0,0 +1,13 @@
> +/* PR preprocessor/36887 */
> +/* { dg-do preprocess } */
> +
> +#ifdef LEVEL2
> +/* Test that we get the include traced location as well.  */
> +#pragma GCC poison p1 /* { dg-note "poisoned here" } */
> +#else
> +#define LEVEL2
> +#include "diagnostic-poison.c"
> +int p1; /* { dg-error "attempt to use poisoned" } */
> +_Pragma("GCC poison p2") /* { dg-note "poisoned here" } */
> +int p2; /* { dg-error "attempt to use poisoned" } */
> +#endif
> diff --git a/gcc/testsuite/g++.dg/pch/pr36887.C b/gcc/testsuite/g++.dg/pch/pr36887.C
> new file mode 100644
> index 00000000000..620ccc17f45
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pch/pr36887.C
> @@ -0,0 +1,3 @@
> +#include "pr36887.H"
> +int p1; /* { dg-error "attempt to use poisoned" } */
> +/* { dg-note "poisoned here" "" { target *-*-* } 1 } */
> diff --git a/gcc/testsuite/g++.dg/pch/pr36887.Hs b/gcc/testsuite/g++.dg/pch/pr36887.Hs
> new file mode 100644
> index 00000000000..e5f4caf150b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pch/pr36887.Hs
> @@ -0,0 +1 @@
> +#pragma GCC poison p1

On Thu, Oct 26, 2023 at 4:49 AM Christophe Lyon
<christophe.lyon@linaro.org> wrote:
> We have noticed that the new tests fail on aarch64 with:
> .../aarch64-unknown-linux-gnu/libc/usr/lib/crt1.o: in function `_start':
> .../sysdeps/aarch64/start.S:110:(.text+0x38): undefined reference to `main'
>
> Looking at the test, I'd say it lacks a dg-do compile (to avoid
> linking), but how does it work on other targets?

Thanks for pointing it out. I am definitely under the impression that
{ dg-do compile } is the default and doesn't need to be specified, I
have never seen it not be the case before... Is that just not correct?
I tried it out on the cfarm (gcc185) for aarch64-redhat-linux and it
works for me there too, I tried the test individually and also as part
of the whole check-gcc-c++ target.

I do see that there are target-dependent functions in
testsuite/lib/*.exp that will change dg-do-what-default under some
circumstances... but I also see in dg-pch.exp (which is the one
relevant for this test g++.dg/pch/pr36887.C) that dg-do-what-default
is set to compile explicitly.

Note sure what the best next step is, should I just add { dg-do
compile } since it's harmless in any case, or is there something else
worth looking into here? I'm not sure why I couldn't reproduce the
issue on the compile farm machine either, maybe you wouldn't mind
please check if adding this line fixes it for you anyway? Thanks...

-Lewis

On Thu, 26 Oct 2023 at 18:18, Lewis Hyatt <lhyatt@gmail.com> wrote:
>
> On Thu, Oct 26, 2023 at 4:49 AM Christophe Lyon
> <christophe.lyon@linaro.org> wrote:
> > We have noticed that the new tests fail on aarch64 with:
> > .../aarch64-unknown-linux-gnu/libc/usr/lib/crt1.o: in function `_start':
> > .../sysdeps/aarch64/start.S:110:(.text+0x38): undefined reference to `main'
> >
> > Looking at the test, I'd say it lacks a dg-do compile (to avoid
> > linking), but how does it work on other targets?
>
> Thanks for pointing it out. I am definitely under the impression that
> { dg-do compile } is the default and doesn't need to be specified, I
> have never seen it not be the case before... Is that just not correct?
> I tried it out on the cfarm (gcc185) for aarch64-redhat-linux and it
> works for me there too, I tried the test individually and also as part
> of the whole check-gcc-c++ target.
>
> I do see that there are target-dependent functions in
> testsuite/lib/*.exp that will change dg-do-what-default under some
> circumstances... but I also see in dg-pch.exp (which is the one
> relevant for this test g++.dg/pch/pr36887.C) that dg-do-what-default
> is set to compile explicitly.

Indeed, thanks for checking.

> Note sure what the best next step is, should I just add { dg-do
> compile } since it's harmless in any case, or is there something else
> worth looking into here? I'm not sure why I couldn't reproduce the
> issue on the compile farm machine either, maybe you wouldn't mind
> please check if adding this line fixes it for you anyway? Thanks...

Can you share the compile line for this test in g++.log?

Actually I'm seeing several similar errors in our g++.log, not
reported before because they were "pre-existing" failures.
So something is confusing the testsuite and puts it into link mode.

I am currently building from scratch, without our CI scripts to get
some additional logs in a setup that probably matches yours. Then I
should be able to add more traces a dejagnu level to understand what's
happening.

Thanks,

Christophe

On Thu, Oct 26, 2023 at 12:48 PM Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>
> On Thu, 26 Oct 2023 at 18:18, Lewis Hyatt <lhyatt@gmail.com> wrote:
> >
> > On Thu, Oct 26, 2023 at 4:49 AM Christophe Lyon
> > <christophe.lyon@linaro.org> wrote:
> > > We have noticed that the new tests fail on aarch64 with:
> > > .../aarch64-unknown-linux-gnu/libc/usr/lib/crt1.o: in function `_start':
> > > .../sysdeps/aarch64/start.S:110:(.text+0x38): undefined reference to `main'
> > >
> > > Looking at the test, I'd say it lacks a dg-do compile (to avoid
> > > linking), but how does it work on other targets?
> >
> > Thanks for pointing it out. I am definitely under the impression that
> > { dg-do compile } is the default and doesn't need to be specified, I
> > have never seen it not be the case before... Is that just not correct?
> > I tried it out on the cfarm (gcc185) for aarch64-redhat-linux and it
> > works for me there too, I tried the test individually and also as part
> > of the whole check-gcc-c++ target.
> >
> > I do see that there are target-dependent functions in
> > testsuite/lib/*.exp that will change dg-do-what-default under some
> > circumstances... but I also see in dg-pch.exp (which is the one
> > relevant for this test g++.dg/pch/pr36887.C) that dg-do-what-default
> > is set to compile explicitly.
>
> Indeed, thanks for checking.
>
> > Note sure what the best next step is, should I just add { dg-do
> > compile } since it's harmless in any case, or is there something else
> > worth looking into here? I'm not sure why I couldn't reproduce the
> > issue on the compile farm machine either, maybe you wouldn't mind
> > please check if adding this line fixes it for you anyway? Thanks...
>
> Can you share the compile line for this test in g++.log?
>

Sure, here is what I got on aarch64 for

make RUNTESTFLAGS=pch.exp=pr36887.C check-gcc-c++

For making the PCH:

xg++ -B/dev/shm/lhyatt/build/gcc/testsuite/g++/../../ ./pr36887.H
-fdiagnostics-plain-output -nostdinc++
-I/dev/shm/lhyatt/build/aarch64-unknown-linux-gnu/libstdc++-v3/include/aarch64-unknown-linux-gnu
-I/dev/shm/lhyatt/build/aarch64-unknown-linux-gnu/libstdc++-v3/include
-I/dev/shm/lhyatt/src/libstdc++-v3/libsupc++
-I/dev/shm/lhyatt/src/libstdc++-v3/include/backward
-I/dev/shm/lhyatt/src/libstdc++-v3/testsuite/util -fmessage-length=0
-g -o pr36887.H.gch

For compiling the test:

xg++ -B/dev/shm/lhyatt/build/gcc/testsuite/g++/../../
/dev/shm/lhyatt/src/gcc/testsuite/g++.dg/pch/pr36887.C
-fdiagnostics-plain-output -nostdinc++
-I/dev/shm/lhyatt/build/aarch64-unknown-linux-gnu/libstdc++-v3/include/aarch64-unknown-linux-gnu
-I/dev/shm/lhyatt/build/aarch64-unknown-linux-gnu/libstdc++-v3/include
-I/dev/shm/lhyatt/src/libstdc++-v3/libsupc++
-I/dev/shm/lhyatt/src/libstdc++-v3/include/backward
-I/dev/shm/lhyatt/src/libstdc++-v3/testsuite/util -fmessage-length=0
-g -I. -Dwith_PCH -S -o pr36887.s

(and then it repeats with -O2 added, or with -g removed as well)

> Actually I'm seeing several similar errors in our g++.log, not
> reported before because they were "pre-existing" failures.
> So something is confusing the testsuite and puts it into link mode.
>
> I am currently building from scratch, without our CI scripts to get
> some additional logs in a setup that probably matches yours. Then I
> should be able to add more traces a dejagnu level to understand what's
> happening.
>
> Thanks,
>
> Christophe

On Thu, 26 Oct 2023 at 19:44, Lewis Hyatt <lhyatt@gmail.com> wrote:
>
> On Thu, Oct 26, 2023 at 12:48 PM Christophe Lyon
> <christophe.lyon@linaro.org> wrote:
> >
> > On Thu, 26 Oct 2023 at 18:18, Lewis Hyatt <lhyatt@gmail.com> wrote:
> > >
> > > On Thu, Oct 26, 2023 at 4:49 AM Christophe Lyon
> > > <christophe.lyon@linaro.org> wrote:
> > > > We have noticed that the new tests fail on aarch64 with:
> > > > .../aarch64-unknown-linux-gnu/libc/usr/lib/crt1.o: in function `_start':
> > > > .../sysdeps/aarch64/start.S:110:(.text+0x38): undefined reference to `main'
> > > >
> > > > Looking at the test, I'd say it lacks a dg-do compile (to avoid
> > > > linking), but how does it work on other targets?
> > >
> > > Thanks for pointing it out. I am definitely under the impression that
> > > { dg-do compile } is the default and doesn't need to be specified, I
> > > have never seen it not be the case before... Is that just not correct?
> > > I tried it out on the cfarm (gcc185) for aarch64-redhat-linux and it
> > > works for me there too, I tried the test individually and also as part
> > > of the whole check-gcc-c++ target.
> > >
> > > I do see that there are target-dependent functions in
> > > testsuite/lib/*.exp that will change dg-do-what-default under some
> > > circumstances... but I also see in dg-pch.exp (which is the one
> > > relevant for this test g++.dg/pch/pr36887.C) that dg-do-what-default
> > > is set to compile explicitly.
> >
> > Indeed, thanks for checking.
> >
> > > Note sure what the best next step is, should I just add { dg-do
> > > compile } since it's harmless in any case, or is there something else
> > > worth looking into here? I'm not sure why I couldn't reproduce the
> > > issue on the compile farm machine either, maybe you wouldn't mind
> > > please check if adding this line fixes it for you anyway? Thanks...
> >
> > Can you share the compile line for this test in g++.log?
> >
>
> Sure, here is what I got on aarch64 for
>
> make RUNTESTFLAGS=pch.exp=pr36887.C check-gcc-c++
>
> For making the PCH:
>
> xg++ -B/dev/shm/lhyatt/build/gcc/testsuite/g++/../../ ./pr36887.H
> -fdiagnostics-plain-output -nostdinc++
> -I/dev/shm/lhyatt/build/aarch64-unknown-linux-gnu/libstdc++-v3/include/aarch64-unknown-linux-gnu
> -I/dev/shm/lhyatt/build/aarch64-unknown-linux-gnu/libstdc++-v3/include
> -I/dev/shm/lhyatt/src/libstdc++-v3/libsupc++
> -I/dev/shm/lhyatt/src/libstdc++-v3/include/backward
> -I/dev/shm/lhyatt/src/libstdc++-v3/testsuite/util -fmessage-length=0
> -g -o pr36887.H.gch
>
> For compiling the test:
>
> xg++ -B/dev/shm/lhyatt/build/gcc/testsuite/g++/../../
> /dev/shm/lhyatt/src/gcc/testsuite/g++.dg/pch/pr36887.C
> -fdiagnostics-plain-output -nostdinc++
> -I/dev/shm/lhyatt/build/aarch64-unknown-linux-gnu/libstdc++-v3/include/aarch64-unknown-linux-gnu
> -I/dev/shm/lhyatt/build/aarch64-unknown-linux-gnu/libstdc++-v3/include
> -I/dev/shm/lhyatt/src/libstdc++-v3/libsupc++
> -I/dev/shm/lhyatt/src/libstdc++-v3/include/backward
> -I/dev/shm/lhyatt/src/libstdc++-v3/testsuite/util -fmessage-length=0
> -g -I. -Dwith_PCH -S -o pr36887.s
>
> (and then it repeats with -O2 added, or with -g removed as well)

OK, thanks.

Our CI harness adds a few linker options to g++
(-Wl,--dynamic-linker=XXX, -Wl-,-rpath...)
which seem to force the GCC driver to invoke the linker.

So, nothing to fix on the testsuite side ;-)

I'll have a look to check if this is a driver bug, or if it can be
fixed in our scripts.

Sorry for the false alarm,

Christophe

>
> > Actually I'm seeing several similar errors in our g++.log, not
> > reported before because they were "pre-existing" failures.
> > So something is confusing the testsuite and puts it into link mode.
> >
> > I am currently building from scratch, without our CI scripts to get
> > some additional logs in a setup that probably matches yours. Then I
> > should be able to add more traces a dejagnu level to understand what's
> > happening.
> >
> > Thanks,
> >
> > Christophe