ping: [PATCH] libcpp: Improve the diagnostic for poisoned identifiers [PR36887]

ping: [PATCH] libcpp: Improve the diagnostic for poisoned identifiers [PR36887]

[Lewis Hyatt]

Hello-

May I please ping this one? Thanks...
https://gcc.gnu.org/pipermail/gcc-patches/2023-September/630967.html

-Lewis

On Wed, Sep 20, 2023 at 12:12 AM 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.
> ---
>  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