On 18 October 2012 00:24, Gabriel Dos Reis <g...@integrable-solutions.net> wrote: > On Wed, Oct 17, 2012 at 6:26 AM, Manuel López-Ibáñez > <lopeziba...@gmail.com> wrote: >> On 17 October 2012 11:55, Dodji Seketeli <do...@redhat.com> wrote: >>> Hello Manuel, >>> >>> Let's CC Gaby on this one as well. >>> >>> Manuel López-Ibáñez <lopeziba...@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 <m...@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.
fix-pr54928.diff
Description: Binary data
