c++: Fix up erroneous template error recovery ICE [PR117826]
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Test passed
|
Commit Message
Hi!
The testcase in the PR (which can't be easily reduced and is
way too large and has way too many errors) results in an ICE,
because the erroneous_templates hash_map holds trees of erroneous
templates across ggc_collect and some of the templates in there
could be removed, so the later lookup can crash on comparison of
already freed and reused trees.
The following patch makes the hash_map GTY(()) marked.
The cp-tree.h changes before the erroneous_template declaration
are needed to make gengtype happy, it didn't like using
directive nor using a template-id as a template parameter.
I was wondering if it should be GTY((cacheable)) or something similar
(I think deletable would be wrong, we don't want to clear it just
because we've done a GC, cacheable maybe because if some erroneous
template decl becomes completely unused and without erroneous_templates
could be garbage collected, then one can't look it up) but because
in correct programs erroneous_templates is always NULL, holding a few
more trees around during error recovery didn't look too bad to me.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2024-12-03 Jakub Jelinek <jakub@redhat.com>
PR c++/117826
* cp-tree.h (struct decl_location_traits): New type.
(erroneous_templates_t): Change using into typedef.
(erroneous_templates): Add GTY(()).
* error.cc (cp_adjust_diagnostic_info): Use
hash_map_safe_get_or_insert<true> rather than
hash_map_safe_get_or_insert<false> for erroneous_templates.
Jakub
Comments
On 12/3/24 3:42 AM, Jakub Jelinek wrote:
> Hi!
>
> The testcase in the PR (which can't be easily reduced and is
> way too large and has way too many errors) results in an ICE,
> because the erroneous_templates hash_map holds trees of erroneous
> templates across ggc_collect and some of the templates in there
> could be removed, so the later lookup can crash on comparison of
> already freed and reused trees.
>
> The following patch makes the hash_map GTY(()) marked.
> The cp-tree.h changes before the erroneous_template declaration
> are needed to make gengtype happy, it didn't like using
> directive nor using a template-id as a template parameter.
>
> I was wondering if it should be GTY((cacheable)) or something similar
> (I think deletable would be wrong, we don't want to clear it just
> because we've done a GC, cacheable maybe because if some erroneous
> template decl becomes completely unused and without erroneous_templates
> could be garbage collected, then one can't look it up) but because
> in correct programs erroneous_templates is always NULL, holding a few
> more trees around during error recovery didn't look too bad to me.
GTY((cache)) sounds right to me. OK with that change.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2024-12-03 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/117826
> * cp-tree.h (struct decl_location_traits): New type.
> (erroneous_templates_t): Change using into typedef.
> (erroneous_templates): Add GTY(()).
> * error.cc (cp_adjust_diagnostic_info): Use
> hash_map_safe_get_or_insert<true> rather than
> hash_map_safe_get_or_insert<false> for erroneous_templates.
>
> --- gcc/cp/cp-tree.h.jj 2024-11-29 09:28:09.665459069 +0100
> +++ gcc/cp/cp-tree.h 2024-12-02 12:39:50.625930029 +0100
> @@ -7226,9 +7226,10 @@ extern location_t location_of
> extern void qualified_name_lookup_error (tree, tree, tree,
> location_t);
>
> -using erroneous_templates_t
> - = hash_map<tree, location_t, simple_hashmap_traits<tree_decl_hash, location_t>>;
> -extern erroneous_templates_t *erroneous_templates;
> +struct decl_location_traits
> + : simple_hashmap_traits<tree_decl_hash, location_t> { };
> +typedef hash_map<tree, location_t, decl_location_traits> erroneous_templates_t;
> +extern GTY(()) erroneous_templates_t *erroneous_templates;
>
> extern bool cp_seen_error ();
> #define seen_error() cp_seen_error ()
> --- gcc/cp/error.cc.jj 2024-11-29 09:28:09.679458868 +0100
> +++ gcc/cp/error.cc 2024-12-02 12:52:55.079856906 +0100
> @@ -237,8 +237,8 @@ cp_adjust_diagnostic_info (diagnostic_co
>
> bool existed;
> location_t &error_loc
> - = hash_map_safe_get_or_insert<false> (erroneous_templates,
> - tmpl, &existed);
> + = hash_map_safe_get_or_insert<true> (erroneous_templates,
> + tmpl, &existed);
> if (!existed)
> /* Remember that this template had a parse-time error so
> that we'll ensure a hard error has been issued upon
>
> Jakub
>
@@ -7226,9 +7226,10 @@ extern location_t location_of
extern void qualified_name_lookup_error (tree, tree, tree,
location_t);
-using erroneous_templates_t
- = hash_map<tree, location_t, simple_hashmap_traits<tree_decl_hash, location_t>>;
-extern erroneous_templates_t *erroneous_templates;
+struct decl_location_traits
+ : simple_hashmap_traits<tree_decl_hash, location_t> { };
+typedef hash_map<tree, location_t, decl_location_traits> erroneous_templates_t;
+extern GTY(()) erroneous_templates_t *erroneous_templates;
extern bool cp_seen_error ();
#define seen_error() cp_seen_error ()
@@ -237,8 +237,8 @@ cp_adjust_diagnostic_info (diagnostic_co
bool existed;
location_t &error_loc
- = hash_map_safe_get_or_insert<false> (erroneous_templates,
- tmpl, &existed);
+ = hash_map_safe_get_or_insert<true> (erroneous_templates,
+ tmpl, &existed);
if (!existed)
/* Remember that this template had a parse-time error so
that we'll ensure a hard error has been issued upon