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

[Manuel López-Ibáñez <lopezibanez at gmail dot com>]

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.