Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion
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
Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion
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 wrote: > Agreed. OK with the comment added. > > Jason
Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion
Agreed. OK with the comment added. Jason
Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion
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 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
On 18 October 2012 00:24, Gabriel Dos Reis wrote: > On Wed, Oct 17, 2012 at 6:26 AM, Manuel López-Ibáñez > wrote: >> On 17 October 2012 11:55, Dodji Seketeli wrote: >>> Hello Manuel, >>> >>> Let's CC Gaby on this one as well. >>> >>> Manuel López-Ibáñez 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 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
Re: PR c++/54928 infinite ICE when reporting ICE on macro expansion
On Wed, Oct 17, 2012 at 6:26 AM, Manuel López-Ibáñez wrote: > On 17 October 2012 11:55, Dodji Seketeli wrote: >> Hello Manuel, >> >> Let's CC Gaby on this one as well. >> >> Manuel López-Ibáñez 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
On 17 October 2012 11:55, Dodji Seketeli wrote: > Hello Manuel, > > Let's CC Gaby on this one as well. > > Manuel López-Ibáñez 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
Hello Manuel,
Let's CC Gaby on this one as well.
Manuel López-Ibáñez 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
PR c++/54928 infinite ICE when reporting ICE on macro expansion
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 gcc/ PR c++/54928 * tree-diagnostic.c (maybe_unwind_expanded_macro_loc): Do not modify diagnostic passed as argument but a temporary copy. pr54928.diff Description: Binary data
