PR c++/54928 infinite ICE when reporting ICE on macro expansion

PR c++/54928 infinite ICE when reporting ICE on macro expansion

[Manuel López-Ibáñez]

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

The problem is that the macro unwinder code is changing the original
diagnostic type and not restoring it, so the code detecting that we
ICE fails to abort, which triggers another ICE, and so on. But there
is no point in modifying the original diagnostic, we can simply create
a temporary copy and use that for macro unwinding.

Bootstrapped and regression tested. I have tested that it fixes the
infiite ICE in the original example, but I am not sure how to add a
testcase for this because I expect the original ICE to be fixed soon
so the testcase will be useless.

Dodji, does it look ok? I am not sure how much testsuite coverage we
have for the macro unwinder, so I hope I didn't mess it up in some
non-trivial testcase.

OK?

2012-10-17  Manuel López-Ibáñez  <manu@gcc.gnu.org>

gcc/
	PR c++/54928
	* tree-diagnostic.c (maybe_unwind_expanded_macro_loc):
	Do not modify diagnostic passed as argument but a temporary copy.

[-- Attachment #2: pr54928.diff --]
[-- Type: application/octet-stream, Size: 4548 bytes --]

Index: gcc/tree-diagnostic.c
===================================================================
--- gcc/tree-diagnostic.c	(revision 192379)
+++ gcc/tree-diagnostic.c	(working copy)
@@ -100,11 +100,11 @@ DEF_VEC_ALLOC_O (loc_map_pair, heap);
    unwound macro expansion trace.  That's the part generated by this
    function.  */
 
 static void
 maybe_unwind_expanded_macro_loc (diagnostic_context *context,
-                                 diagnostic_info *diagnostic,
+                                 const diagnostic_info *diagnostic_orig,
                                  source_location where)
 {
   const struct line_map *map;
   VEC(loc_map_pair,heap) *loc_vec = NULL;
   unsigned ix;
@@ -142,18 +142,21 @@ maybe_unwind_expanded_macro_loc (diagnos
      first triggered the macro expansion.  This must be an ordinary map.  */
 
   /* Walk LOC_VEC and print the macro expansion trace, unless the
      first macro which expansion triggered this trace was expanded
      inside a system header.  */
+  const char *saved_prefix = pp_get_prefix (context->printer);
+  diagnostic_info diagnostic = *diagnostic_orig;
+  diagnostic.kind = DK_NOTE;
+  int saved_location_line =
+    expand_location_to_spelling_point (diagnostic.location).line;
+
   if (!LINEMAP_SYSP (map))
     FOR_EACH_VEC_ELT (loc_map_pair, loc_vec, ix, iter)
       {
-        source_location resolved_def_loc = 0, resolved_exp_loc = 0,
-	  saved_location = 0;
-	int resolved_def_loc_line = 0, saved_location_line = 0;
-        diagnostic_t saved_kind;
-        const char *saved_prefix;
+        source_location resolved_def_loc = 0, resolved_exp_loc = 0;
+	int resolved_def_loc_line = 0;
 	/* Sometimes, in the unwound macro expansion trace, we want to
 	   print a part of the context that shows where, in the
 	   definition of the relevant macro, is the token (we are
 	   looking at) used.  That is the case in the introductory
 	   comment of this function, where we print:
@@ -211,55 +214,45 @@ maybe_unwind_expanded_macro_loc (diagnos
         resolved_exp_loc =
           linemap_resolve_location (line_table,
                                     MACRO_MAP_EXPANSION_POINT_LOCATION (iter->map),
                                     LRK_MACRO_DEFINITION_LOCATION, NULL);
 
-        saved_kind = diagnostic->kind;
-        saved_prefix = pp_get_prefix (context->printer);
-        saved_location = diagnostic->location;
-	saved_location_line =
-	  expand_location_to_spelling_point (saved_location).line;
-
-        diagnostic->kind = DK_NOTE;
 
 	/* We need to print the context of the macro definition only
 	   when the locus of the first displayed diagnostic (displayed
 	   before this trace) was inside the definition of the
 	   macro.  */
 	print_definition_context_p =
 	  (ix == 0 && (saved_location_line != resolved_def_loc_line));
 
 	if (print_definition_context_p)
 	  {
-	    diagnostic->location = resolved_def_loc;
+	    diagnostic.location = resolved_def_loc;
 	    pp_set_prefix (context->printer,
-			   diagnostic_build_prefix (context, diagnostic));
+			   diagnostic_build_prefix (context, &diagnostic));
 	    pp_newline (context->printer);
 	    pp_printf (context->printer, "in definition of macro '%s'",
 		       linemap_map_get_macro_name (iter->map));
 	    pp_destroy_prefix (context->printer);
-	    diagnostic_show_locus (context, diagnostic);
+	    diagnostic_show_locus (context, &diagnostic);
 	    /* At this step, as we've printed the context of the macro
 	       definition, we don't want to print the context of its
 	       expansion, otherwise, it'd be redundant.  */
 	    continue;
 	  }
 
-	diagnostic->location = resolved_exp_loc;
+	diagnostic.location = resolved_exp_loc;
 	pp_set_prefix (context->printer,
-                       diagnostic_build_prefix (context, diagnostic));
+                       diagnostic_build_prefix (context, &diagnostic));
 	pp_newline (context->printer);
 	pp_printf (context->printer, "in expansion of macro '%s'",
 		   linemap_map_get_macro_name (iter->map));
         pp_destroy_prefix (context->printer);
-        diagnostic_show_locus (context, diagnostic);
-
-        diagnostic->kind = saved_kind;
-        diagnostic->location = saved_location;
-        pp_set_prefix (context->printer, saved_prefix);
+        diagnostic_show_locus (context, &diagnostic);
       }
 
+  pp_set_prefix (context->printer, saved_prefix);
   VEC_free (loc_map_pair, heap, loc_vec);
 }
 
 /*  This is a diagnostic finalizer implementation that is aware of
     virtual locations produced by libcpp.

Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion

[Dodji Seketeli]

Hello Manuel,

Let's CC Gaby on this one as well.

Manuel López-Ibáñez <lopezibanez@gmail.com> writes:

> The problem is that the macro unwinder code is changing the original
> diagnostic type and not restoring it, so the code detecting that we
> ICE fails to abort, which triggers another ICE, and so on. But there
> is no point in modifying the original diagnostic, we can simply create
> a temporary copy and use that for macro unwinding.

We modify the context as well, and we set it back to its original value
before getting out.  Why not just doing the same for the diagnostic_info
type? I mean, just save diagnostics->kind before changing it, and set it
back to the saved value before getting out?  That is less expensive than
copying all of the diagnostic_info.

I don't intent to fight either way, but I'd be more inclined to just
setting the initial value back.  If the maintainers think otherwise,
I'll abide.

> Dodji, does it look ok? I am not sure how much testsuite coverage we
> have for the macro unwinder, so I hope I didn't mess it up in some
> non-trivial testcase.

The test cases for the unwinder are not really tight.  They are
mainly gcc.dg/cpp/macro-exp-tracking-{1,2,3}.c, AFAICT.

Also, the patch does some cleanup.  Maybe it'd be best to post the
cleanup by removing a useless variable and hoisting some variable
accesses; maybe that cleanup in a separate patch that would be committed
at the same time?

Thank you for your time.

-- 
		Dodji

Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion

[Manuel López-Ibáñez]

On 17 October 2012 11:55, Dodji Seketeli <dodji@redhat.com> wrote:
> Hello Manuel,
>
> Let's CC Gaby on this one as well.
>
> Manuel López-Ibáñez <lopezibanez@gmail.com> writes:
>
>> The problem is that the macro unwinder code is changing the original
>> diagnostic type and not restoring it, so the code detecting that we
>> ICE fails to abort, which triggers another ICE, and so on. But there
>> is no point in modifying the original diagnostic, we can simply create
>> a temporary copy and use that for macro unwinding.
>
> We modify the context as well, and we set it back to its original value
> before getting out.  Why not just doing the same for the diagnostic_info
> type? I mean, just save diagnostics->kind before changing it, and set it
> back to the saved value before getting out?  That is less expensive than
> copying all of the diagnostic_info.

Well, the difference is that for context, we are not sure that what we
get at the end of the function is actually the same that we received.
I am not sure if there is some buffer/obstack growth there. For
diagnostic_info it is very different: we want to return exactly the
original. (And in fact, both maybe_unwind_expanded_macro_loc and
diagnostic_build_prefix should take a const * diagnostic_info).

Also, I am not sure why we need to restore the prefix. Once the
warning/error has been printed and we are just attaching notes from
the unwinder, we don't care about the original prefix so we may simply
destroy it. In fact, I think we are *leaking memory* by not destroying
the prefix. Perhaps the prefix should be destroyed always after the
finalizer and the default finalizer should be empty?

Actually, I would propose going even a step further and use a more
high-level api instead of calling into the pretty-printer directly.
Something like: diagnostic_attach_note(context, const *
diagnostic_info, location_t, const char * message, ...) that for
example will check for context->inhibit_notes_p, will make sure the
message is translated, will make sure that diagnostic_info is not
overriden, will print the caret (or not), etc. This will live in
diagnostic.c and could be used by customized diagnostic hooks to
attach notes to an existing diagnostic. It would be a bit less
optimized than the current code, but more re-usable.

What do you think?

It would be good to have Gaby's opinion as well, since what I am
proposing is more far-reaching.

Cheers,

Manuel.

Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion

[Gabriel Dos Reis]

On Wed, Oct 17, 2012 at 6:26 AM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> On 17 October 2012 11:55, Dodji Seketeli <dodji@redhat.com> wrote:
>> Hello Manuel,
>>
>> Let's CC Gaby on this one as well.
>>
>> Manuel López-Ibáñez <lopezibanez@gmail.com> writes:
>>
>>> The problem is that the macro unwinder code is changing the original
>>> diagnostic type and not restoring it, so the code detecting that we
>>> ICE fails to abort, which triggers another ICE, and so on. But there
>>> is no point in modifying the original diagnostic, we can simply create
>>> a temporary copy and use that for macro unwinding.
>>
>> We modify the context as well, and we set it back to its original value
>> before getting out.  Why not just doing the same for the diagnostic_info
>> type? I mean, just save diagnostics->kind before changing it, and set it
>> back to the saved value before getting out?  That is less expensive than
>> copying all of the diagnostic_info.
>
> Well, the difference is that for context, we are not sure that what we
> get at the end of the function is actually the same that we received.
> I am not sure if there is some buffer/obstack growth there. For
> diagnostic_info it is very different: we want to return exactly the
> original. (And in fact, both maybe_unwind_expanded_macro_loc and
> diagnostic_build_prefix should take a const * diagnostic_info).
>
> Also, I am not sure why we need to restore the prefix. Once the
> warning/error has been printed and we are just attaching notes from
> the unwinder, we don't care about the original prefix so we may simply
> destroy it. In fact, I think we are *leaking memory* by not destroying
> the prefix. Perhaps the prefix should be destroyed always after the
> finalizer and the default finalizer should be empty?
>
> Actually, I would propose going even a step further and use a more
> high-level api instead of calling into the pretty-printer directly.
> Something like: diagnostic_attach_note(context, const *
> diagnostic_info, location_t, const char * message, ...) that for
> example will check for context->inhibit_notes_p, will make sure the
> message is translated, will make sure that diagnostic_info is not
> overriden, will print the caret (or not), etc. This will live in
> diagnostic.c and could be used by customized diagnostic hooks to
> attach notes to an existing diagnostic. It would be a bit less
> optimized than the current code, but more re-usable.
>
> What do you think?

That makes sense and is what we should do.

>
> It would be good to have Gaby's opinion as well, since what I am
> proposing is more far-reaching.
>
> Cheers,
>
> Manuel.

Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion

[Manuel López-Ibáñez]

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

On 18 October 2012 00:24, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote:
> On Wed, Oct 17, 2012 at 6:26 AM, Manuel López-Ibáñez
> <lopezibanez@gmail.com> wrote:
>> On 17 October 2012 11:55, Dodji Seketeli <dodji@redhat.com> wrote:
>>> Hello Manuel,
>>>
>>> Let's CC Gaby on this one as well.
>>>
>>> Manuel López-Ibáñez <lopezibanez@gmail.com> writes:
>>>
>>>> The problem is that the macro unwinder code is changing the original
>>>> diagnostic type and not restoring it, so the code detecting that we
>>>> ICE fails to abort, which triggers another ICE, and so on. But there
>>>> is no point in modifying the original diagnostic, we can simply create
>>>> a temporary copy and use that for macro unwinding.
>>>
>>> We modify the context as well, and we set it back to its original value
>>> before getting out.  Why not just doing the same for the diagnostic_info
>>> type? I mean, just save diagnostics->kind before changing it, and set it
>>> back to the saved value before getting out?  That is less expensive than
>>> copying all of the diagnostic_info.
>>
>> Well, the difference is that for context, we are not sure that what we
>> get at the end of the function is actually the same that we received.
>> I am not sure if there is some buffer/obstack growth there. For
>> diagnostic_info it is very different: we want to return exactly the
>> original. (And in fact, both maybe_unwind_expanded_macro_loc and
>> diagnostic_build_prefix should take a const * diagnostic_info).
>>
>> Also, I am not sure why we need to restore the prefix. Once the
>> warning/error has been printed and we are just attaching notes from
>> the unwinder, we don't care about the original prefix so we may simply
>> destroy it. In fact, I think we are *leaking memory* by not destroying
>> the prefix. Perhaps the prefix should be destroyed always after the
>> finalizer and the default finalizer should be empty?
>>
>> Actually, I would propose going even a step further and use a more
>> high-level api instead of calling into the pretty-printer directly.
>> Something like: diagnostic_attach_note(context, const *
>> diagnostic_info, location_t, const char * message, ...) that for
>> example will check for context->inhibit_notes_p, will make sure the
>> message is translated, will make sure that diagnostic_info is not
>> overriden, will print the caret (or not), etc. This will live in
>> diagnostic.c and could be used by customized diagnostic hooks to
>> attach notes to an existing diagnostic. It would be a bit less
>> optimized than the current code, but more re-usable.
>>
>> What do you think?
>
> That makes sense and is what we should do.

The attached patch implements this idea. I have bootstrapped and
regression tested it on x86_64-linux-gnu. And I have checked that it
fixes the cascade of ICEs. I don't think there is any point in adding
the original testcase because it will be added when the first ICE is
fixed, and then, it won't work as a test of this fix. I couldn't
figure out a way to trigger this issue without triggering an ICE
first.

OK?


2012-10-23  Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR c++/54928
gcc/
	* tree-diagnostic.c (maybe_unwind_expanded_macro_loc):
	Use diagnostic_attach_note.
	* diagnostic.c (diagnostic_build_prefix): Make diagnostic const.
	(default_diagnostic_finalizer): Do not destroy prefix here.
	(diagnostic_report_diagnostic): Destroy it here.
	(diagnostic_attach_note): New.
	* diagnostic.h (diagnostic_attach_note): Declare.

[-- Attachment #2: fix-pr54928.diff --]
[-- Type: application/octet-stream, Size: 10551 bytes --]

Index: gcc/tree-diagnostic.c
===================================================================
--- gcc/tree-diagnostic.c	(revision 192379)
+++ gcc/tree-diagnostic.c	(working copy)
@@ -100,11 +100,11 @@ DEF_VEC_ALLOC_O (loc_map_pair, heap);
    unwound macro expansion trace.  That's the part generated by this
    function.  */
 
 static void
 maybe_unwind_expanded_macro_loc (diagnostic_context *context,
-                                 diagnostic_info *diagnostic,
+                                 const diagnostic_info *diagnostic,
                                  source_location where)
 {
   const struct line_map *map;
   VEC(loc_map_pair,heap) *loc_vec = NULL;
   unsigned ix;
@@ -142,18 +142,16 @@ maybe_unwind_expanded_macro_loc (diagnos
      first triggered the macro expansion.  This must be an ordinary map.  */
 
   /* Walk LOC_VEC and print the macro expansion trace, unless the
      first macro which expansion triggered this trace was expanded
      inside a system header.  */
+  int saved_location_line =
+    expand_location_to_spelling_point (diagnostic->location).line;
+
   if (!LINEMAP_SYSP (map))
     FOR_EACH_VEC_ELT (loc_map_pair, loc_vec, ix, iter)
       {
-        source_location resolved_def_loc = 0, resolved_exp_loc = 0,
-	  saved_location = 0;
-	int resolved_def_loc_line = 0, saved_location_line = 0;
-        diagnostic_t saved_kind;
-        const char *saved_prefix;
 	/* Sometimes, in the unwound macro expansion trace, we want to
 	   print a part of the context that shows where, in the
 	   definition of the relevant macro, is the token (we are
 	   looking at) used.  That is the case in the introductory
 	   comment of this function, where we print:
@@ -172,92 +170,59 @@ maybe_unwind_expanded_macro_loc (diagnos
 	   macro definition context to the user somehow.
 
 	   A contrario, when the first interesting diagnostic line
 	   points into the definition of the macro, we don't need to
 	   display any line for that macro definition in the trace
-	   anymore, otherwise it'd be redundant.
-
-	   This flag is true when we need to display the context of
-	   the macro definition.  */
-	bool print_definition_context_p = false;
+	   anymore, otherwise it'd be redundant.  */
 
         /* Okay, now here is what we want.  For each token resulting
            from macro expansion we want to show: 1/ where in the
            definition of the macro the token comes from; 2/ where the
            macro got expanded.  */
 
         /* Resolve the location iter->where into the locus 1/ of the
            comment above.  */
-        resolved_def_loc =
+        source_location resolved_def_loc =
           linemap_resolve_location (line_table, iter->where,
                                     LRK_MACRO_DEFINITION_LOCATION, NULL);
 
 	/* Don't print trace for locations that are reserved or from
 	   within a system header.  */
-	{
-	  const struct line_map *m = NULL;
-	  source_location l = linemap_resolve_location (line_table, resolved_def_loc,
-							LRK_SPELLING_LOCATION,
-							&m);
-	  if (l < RESERVED_LOCATION_COUNT
-	      || LINEMAP_SYSP (m))
-	    continue;
-
-	  resolved_def_loc_line = SOURCE_LINE (m, l);
-	}
+        const struct line_map *m = NULL;
+        source_location l = 
+          linemap_resolve_location (line_table, resolved_def_loc,
+                                    LRK_SPELLING_LOCATION,  &m);
+        if (l < RESERVED_LOCATION_COUNT || LINEMAP_SYSP (m))
+          continue;
+        
+	/* We need to print the context of the macro definition only
+	   when the locus of the first displayed diagnostic (displayed
+	   before this trace) was inside the definition of the
+	   macro.  */
+        int resolved_def_loc_line = SOURCE_LINE (m, l);
+        if (ix == 0 && saved_location_line != resolved_def_loc_line)
+          {
+            diagnostic_attach_note (context, resolved_def_loc, 
+                                    "in definition of macro %qs",
+                                    linemap_map_get_macro_name (iter->map));
+            /* At this step, as we've printed the context of the macro
+               definition, we don't want to print the context of its
+               expansion, otherwise, it'd be redundant.  */
+            continue;
+          }
 
         /* Resolve the location of the expansion point of the macro
            which expansion gave the token represented by def_loc.
            This is the locus 2/ of the earlier comment.  */
-        resolved_exp_loc =
+        source_location resolved_exp_loc =
           linemap_resolve_location (line_table,
                                     MACRO_MAP_EXPANSION_POINT_LOCATION (iter->map),
                                     LRK_MACRO_DEFINITION_LOCATION, NULL);
 
-        saved_kind = diagnostic->kind;
-        saved_prefix = pp_get_prefix (context->printer);
-        saved_location = diagnostic->location;
-	saved_location_line =
-	  expand_location_to_spelling_point (saved_location).line;
-
-        diagnostic->kind = DK_NOTE;
-
-	/* We need to print the context of the macro definition only
-	   when the locus of the first displayed diagnostic (displayed
-	   before this trace) was inside the definition of the
-	   macro.  */
-	print_definition_context_p =
-	  (ix == 0 && (saved_location_line != resolved_def_loc_line));
-
-	if (print_definition_context_p)
-	  {
-	    diagnostic->location = resolved_def_loc;
-	    pp_set_prefix (context->printer,
-			   diagnostic_build_prefix (context, diagnostic));
-	    pp_newline (context->printer);
-	    pp_printf (context->printer, "in definition of macro '%s'",
-		       linemap_map_get_macro_name (iter->map));
-	    pp_destroy_prefix (context->printer);
-	    diagnostic_show_locus (context, diagnostic);
-	    /* At this step, as we've printed the context of the macro
-	       definition, we don't want to print the context of its
-	       expansion, otherwise, it'd be redundant.  */
-	    continue;
-	  }
-
-	diagnostic->location = resolved_exp_loc;
-	pp_set_prefix (context->printer,
-                       diagnostic_build_prefix (context, diagnostic));
-	pp_newline (context->printer);
-	pp_printf (context->printer, "in expansion of macro '%s'",
-		   linemap_map_get_macro_name (iter->map));
-        pp_destroy_prefix (context->printer);
-        diagnostic_show_locus (context, diagnostic);
-
-        diagnostic->kind = saved_kind;
-        diagnostic->location = saved_location;
-        pp_set_prefix (context->printer, saved_prefix);
+        diagnostic_attach_note (context, resolved_exp_loc, 
+                                "in expansion of macro %qs",
+                                linemap_map_get_macro_name (iter->map));
       }
 
   VEC_free (loc_map_pair, heap, loc_vec);
 }
 
Index: gcc/diagnostic.c
===================================================================
--- gcc/diagnostic.c	(revision 192379)
+++ gcc/diagnostic.c	(working copy)
@@ -206,11 +206,11 @@ diagnostic_set_info (diagnostic_info *di
 
 /* Return a malloc'd string describing a location.  The caller is
    responsible for freeing the memory.  */
 char *
 diagnostic_build_prefix (diagnostic_context *context,
-			 diagnostic_info *diagnostic)
+			 const diagnostic_info *diagnostic)
 {
   static const char *const diagnostic_kind_text[] = {
 #define DEFINE_DIAGNOSTIC_KIND(K, T) (T),
 #include "diagnostic.def"
 #undef DEFINE_DIAGNOSTIC_KIND
@@ -517,14 +517,13 @@ default_diagnostic_starter (diagnostic_c
   pp_set_prefix (context->printer, diagnostic_build_prefix (context,
 							    diagnostic));
 }
 
 void
-default_diagnostic_finalizer (diagnostic_context *context,
+default_diagnostic_finalizer (diagnostic_context *context ATTRIBUTE_UNUSED,
 			      diagnostic_info *diagnostic ATTRIBUTE_UNUSED)
 {
-  pp_destroy_prefix (context->printer);
 }
 
 /* Interface to specify diagnostic kind overrides.  Returns the
    previous setting, or DK_UNSPECIFIED if the parameters are out of
    range.  */
@@ -757,10 +756,11 @@ diagnostic_report_diagnostic (diagnostic
   pp_format (context->printer, &diagnostic->message);
   (*diagnostic_starter (context)) (context, diagnostic);
   pp_output_formatted_text (context->printer);
   diagnostic_show_locus (context, diagnostic);
   (*diagnostic_finalizer (context)) (context, diagnostic);
+  pp_destroy_prefix (context->printer);
   pp_newline_and_flush (context->printer);
   diagnostic_action_after_output (context, diagnostic);
   diagnostic->message.format_spec = saved_format_spec;
   diagnostic->x_data = NULL;
 
@@ -819,10 +819,32 @@ verbatim (const char *gmsgid, ...)
   pp_format_verbatim (global_dc->printer, &text);
   pp_newline_and_flush (global_dc->printer);
   va_end (ap);
 }
 
+void
+diagnostic_attach_note (diagnostic_context *context,
+                        location_t location,
+                        const char * gmsgid, ...)
+{
+  diagnostic_info diagnostic;
+  va_list ap;
+
+  va_start (ap, gmsgid);
+  diagnostic_set_info (&diagnostic, gmsgid, &ap, location, DK_NOTE);
+  if (context->inhibit_notes_p)
+    return;
+  pp_set_prefix (context->printer,
+                 diagnostic_build_prefix (context, &diagnostic));
+  pp_newline (context->printer);
+  pp_format (context->printer, &diagnostic.message);
+  pp_output_formatted_text (context->printer);
+  pp_destroy_prefix (context->printer);
+  diagnostic_show_locus (context, &diagnostic);
+  va_end(ap);
+}
+
 bool
 emit_diagnostic (diagnostic_t kind, location_t location, int opt,
 		 const char *gmsgid, ...)
 {
   diagnostic_info diagnostic;
Index: gcc/diagnostic.h
===================================================================
--- gcc/diagnostic.h	(revision 192379)
+++ gcc/diagnostic.h	(working copy)
@@ -280,12 +280,14 @@ extern void diagnostic_set_info (diagnos
 				 location_t, diagnostic_t) ATTRIBUTE_GCC_DIAG(2,0);
 extern void diagnostic_set_info_translated (diagnostic_info *, const char *,
 					    va_list *, location_t,
 					    diagnostic_t)
      ATTRIBUTE_GCC_DIAG(2,0);
+extern void diagnostic_attach_note (diagnostic_context *, location_t,
+                                    const char *, ...) ATTRIBUTE_GCC_DIAG(3,4);
 #endif
-extern char *diagnostic_build_prefix (diagnostic_context *, diagnostic_info *);
+extern char *diagnostic_build_prefix (diagnostic_context *, const diagnostic_info *);
 void default_diagnostic_starter (diagnostic_context *, diagnostic_info *);
 void default_diagnostic_finalizer (diagnostic_context *, diagnostic_info *);
 void diagnostic_set_caret_max_width (diagnostic_context *context, int value);
 
 

Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion

[Dodji Seketeli]

I am not a maintainer so the below are just some casual comments of
mine.  I am deferring to the maintainers for a real review.

Manuel López-Ibáñez <lopezibanez@gmail.com> writes:

> gcc/
> 	* tree-diagnostic.c (maybe_unwind_expanded_macro_loc):
> 	Use diagnostic_attach_note.
> 	* diagnostic.c (diagnostic_build_prefix): Make diagnostic const.
> 	(default_diagnostic_finalizer): Do not destroy prefix here.
> 	(diagnostic_report_diagnostic): Destroy it here.
> 	(diagnostic_attach_note): New.
> 	* diagnostic.h (diagnostic_attach_note): Declare.
>

These bits looks good to me, barring ...

[...]

> +void
> +diagnostic_attach_note (diagnostic_context *context,
> +                        location_t location,
> +                        const char * gmsgid, ...)
> +{

... this function that lacks a comment.

Thanks.

-- 
		Dodji

Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion

[Jason Merrill]

Agreed.  OK with the comment added.

Jason

Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion

[Manuel López-Ibáñez]

What about?

/* Add a note with text GMSGID and with LOCATION to the diagnostic CONTEXT.  */

Actually, I don't know why I call it "attach". diagnostic_add_note()
or diagnostic_print_note() seems clearer. What do you think?

Cheers,

Manuel.


On 24 October 2012 19:27, Jason Merrill <jason@redhat.com> wrote:
> Agreed.  OK with the comment added.
>
> Jason

Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion

[Jason Merrill]

On 10/24/2012 01:54 PM, Manuel López-Ibáñez wrote:
> /* Add a note with text GMSGID and with LOCATION to the diagnostic CONTEXT.  */

Sure.

> Actually, I don't know why I call it "attach". diagnostic_add_note()
> or diagnostic_print_note() seems clearer. What do you think?

How about "append"?

Jason