| Message ID | ZvXR/HV3Kbb8YtGr@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | diagnostic: Save/restore diagnostic context history and push/pop state for PCH [PR116847] | expand |
On Thu, 2024-09-26 at 23:28 +0200, Jakub Jelinek wrote: > Hi! > > The following patch on top of the just posted cleanup patch > saves/restores the m_classification_history and m_push_list > vectors for PCH. Without that as the testcase shows during parsing > of the templates we don't report ignored diagnostics, but after > loading > PCH header when instantiating those templates those warnings can be > emitted. This doesn't show up on x86_64-linux build because > configure > injects there -fcf-protection -mshstk flags during library build (and > so > also during PCH header creation), but make check doesn't use those > flags > and so the PCH header is ignored. > > Bootstrapped on i686-linux so far, bootstrap/regtest on x86_64-linux > and > i686-linux still pending, ok for trunk if it passes it? Thanks, yes please Dave
On Fri, Sep 27, 2024 at 9:41 AM David Malcolm <dmalcolm@redhat.com> wrote: > > On Thu, 2024-09-26 at 23:28 +0200, Jakub Jelinek wrote: > > Hi! > > > > The following patch on top of the just posted cleanup patch > > saves/restores the m_classification_history and m_push_list > > vectors for PCH. Without that as the testcase shows during parsing > > of the templates we don't report ignored diagnostics, but after > > loading > > PCH header when instantiating those templates those warnings can be > > emitted. This doesn't show up on x86_64-linux build because > > configure > > injects there -fcf-protection -mshstk flags during library build (and > > so > > also during PCH header creation), but make check doesn't use those > > flags > > and so the PCH header is ignored. > > > > Bootstrapped on i686-linux so far, bootstrap/regtest on x86_64-linux > > and > > i686-linux still pending, ok for trunk if it passes it? > > Thanks, yes please > > Dave > A couple comments that may be helpful... -This is also PR 64117 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64117) -I submitted a patch last year for that but did not get any response (https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635648.html). I guess I never pinged it because I am still trying to ping two other ones :). My patch did not switch to vec so it was not as nice as this one. I wonder though, if some of the testcases I added could be incorporated? In particular the testcase from my patch pragma-diagnostic-3.C I believe will still be failing after this one. There is an issue with C++ because it processes the pragmas twice, once in early mode and once in normal mode, that makes it do the wrong thing for this case: t.h: ---- #pragma GCC diagnostic push #pragma GCC diagnostic ignored... //no pop at end of the file t.c ---- #include "t.h" #pragma GCC diagnostic pop //expect to be at the initial state here, but won't be if t.h is a PCH In my patch I had separated the PCH restore from a more general "state restore" logic so that the C++ frontend can restore the state after the first pass through the data. -Lewis
On Fri, 2024-09-27 at 09:54 -0400, Lewis Hyatt wrote: > On Fri, Sep 27, 2024 at 9:41 AM David Malcolm <dmalcolm@redhat.com> > wrote: > > > > On Thu, 2024-09-26 at 23:28 +0200, Jakub Jelinek wrote: > > > Hi! > > > > > > The following patch on top of the just posted cleanup patch > > > saves/restores the m_classification_history and m_push_list > > > vectors for PCH. Without that as the testcase shows during > > > parsing > > > of the templates we don't report ignored diagnostics, but after > > > loading > > > PCH header when instantiating those templates those warnings can > > > be > > > emitted. This doesn't show up on x86_64-linux build because > > > configure > > > injects there -fcf-protection -mshstk flags during library build > > > (and > > > so > > > also during PCH header creation), but make check doesn't use > > > those > > > flags > > > and so the PCH header is ignored. > > > > > > Bootstrapped on i686-linux so far, bootstrap/regtest on x86_64- > > > linux > > > and > > > i686-linux still pending, ok for trunk if it passes it? > > > > Thanks, yes please > > > > Dave > > > > A couple comments that may be helpful... > > -This is also PR 64117 > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64117) > > -I submitted a patch last year for that but did not get any response > ( > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635648.html). > I guess I never pinged it because I am still trying to ping two other > ones :). Gahhh, I'm sorry about this. What are the other two patches? > My patch did not switch to vec so it was not as nice as this > one. I wonder though, if some of the testcases I added could be > incorporated? In particular the testcase from my patch > pragma-diagnostic-3.C I believe will still be failing after this one. > There is an issue with C++ because it processes the pragmas twice, > once in early mode and once in normal mode, that makes it do the > wrong > thing for this case: > > t.h: > ---- > #pragma GCC diagnostic push > #pragma GCC diagnostic ignored... > //no pop at end of the file > > t.c > ---- > #include "t.h" > #pragma GCC diagnostic pop > //expect to be at the initial state here, but won't be if t.h is a > PCH > > In my patch I had separated the PCH restore from a more general > "state > restore" logic so that the C++ frontend can restore the state after > the first pass through the data. It sounds like the ideal here would be to incorporate the test cases from Lewis's patch into Jakub's, if the latter can be tweaked to fix pragma-diagnostic-3.C Dave
On Fri, Sep 27, 2024 at 09:54:13AM -0400, Lewis Hyatt wrote: > A couple comments that may be helpful... > > -This is also PR 64117 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64117) > > -I submitted a patch last year for that but did not get any response > (https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635648.html). Oops, sorry, wasn't aware of that. Note, I've already committed it. > I guess I never pinged it because I am still trying to ping two other > ones :). My patch did not switch to vec so it was not as nice as this > one. I wonder though, if some of the testcases I added could be > incorporated? In particular the testcase from my patch Maybe. > pragma-diagnostic-3.C I believe will still be failing after this one. > There is an issue with C++ because it processes the pragmas twice, > once in early mode and once in normal mode, that makes it do the wrong > thing for this case: > > t.h: > ---- > #pragma GCC diagnostic push > #pragma GCC diagnostic ignored... > //no pop at end of the file > > t.c > ---- > #include "t.h" > #pragma GCC diagnostic pop > //expect to be at the initial state here, but won't be if t.h is a PCH > > In my patch I had separated the PCH restore from a more general "state > restore" logic so that the C++ frontend can restore the state after > the first pass through the data. People shouldn't be doing this, without PCH or with it, and especially not with PCH, that is simply too ugly. That said, this was the reason why I have saved also the m_push_list vector and not just the history. If that isn't enough and there is some other state on the libcpp side, I'd think we should go with a sorry and tell the user not to do this with PCH. It becomes a nightmare already if e.g. the command line -Werror=something -Wno-error=somethingelse overrides differ between the PCH creation and PCH reading (my first version of the patch saved m_classify_diagnostic array but that broke tests relying on those differences). Jakub
On Fri, 2024-09-27 at 10:23 -0400, David Malcolm wrote: > On Fri, 2024-09-27 at 09:54 -0400, Lewis Hyatt wrote: > > On Fri, Sep 27, 2024 at 9:41 AM David Malcolm <dmalcolm@redhat.com> > > wrote: > > > > > > On Thu, 2024-09-26 at 23:28 +0200, Jakub Jelinek wrote: > > > > Hi! > > > > > > > > The following patch on top of the just posted cleanup patch > > > > saves/restores the m_classification_history and m_push_list > > > > vectors for PCH. Without that as the testcase shows during > > > > parsing > > > > of the templates we don't report ignored diagnostics, but after > > > > loading > > > > PCH header when instantiating those templates those warnings > > > > can > > > > be > > > > emitted. This doesn't show up on x86_64-linux build because > > > > configure > > > > injects there -fcf-protection -mshstk flags during library > > > > build > > > > (and > > > > so > > > > also during PCH header creation), but make check doesn't use > > > > those > > > > flags > > > > and so the PCH header is ignored. > > > > > > > > Bootstrapped on i686-linux so far, bootstrap/regtest on x86_64- > > > > linux > > > > and > > > > i686-linux still pending, ok for trunk if it passes it? > > > > > > Thanks, yes please > > > > > > Dave > > > > > > > A couple comments that may be helpful... > > > > -This is also PR 64117 > > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64117) > > > > -I submitted a patch last year for that but did not get any > > response > > ( > > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635648.html > > ). > > I guess I never pinged it because I am still trying to ping two > > other > > ones :). > > Gahhh, I'm sorry about this. > > What are the other two patches? > > > My patch did not switch to vec so it was not as nice as this > > one. I wonder though, if some of the testcases I added could be > > incorporated? In particular the testcase from my patch > > pragma-diagnostic-3.C I believe will still be failing after this > > one. > > There is an issue with C++ because it processes the pragmas twice, > > once in early mode and once in normal mode, that makes it do the > > wrong > > thing for this case: > > > > t.h: > > ---- > > #pragma GCC diagnostic push > > #pragma GCC diagnostic ignored... > > //no pop at end of the file > > > > t.c > > ---- > > #include "t.h" > > #pragma GCC diagnostic pop > > //expect to be at the initial state here, but won't be if t.h is a > > PCH > > > > In my patch I had separated the PCH restore from a more general > > "state > > restore" logic so that the C++ frontend can restore the state after > > the first pass through the data. > > It sounds like the ideal here would be to incorporate the test cases > from Lewis's patch into Jakub's, if the latter can be tweaked to fix > pragma-diagnostic-3.C ...and I see that Jakub has pushed his patch (as https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=64072e60b1599ae7d347c2cdee46c3b0e37fc338 so is there a way to do Lewis's patch on top of that? Sorry about this Dave
On Fri, Sep 27, 2024 at 10:23 AM David Malcolm <dmalcolm@redhat.com> wrote: > > -I submitted a patch last year for that but did not get any response > > ( > > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635648.html). > > I guess I never pinged it because I am still trying to ping two other > > ones :). > > Gahhh, I'm sorry about this. > > What are the other two patches? > Oh thanks, no worries... everyone has a lot to do :). I think, libcpp patches in particular often don't get a lot of attention, but there is a lot more going on. I'm happy to help where I can. Anyway the two I was hoping to get in were: https://gcc.gnu.org/pipermail/gcc-patches/2024-September/663853.html https://gcc.gnu.org/pipermail/gcc-patches/2024-March/648297.html In case there is time for them :). -Lewis
On Fri, Sep 27, 2024 at 10:26 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Fri, Sep 27, 2024 at 09:54:13AM -0400, Lewis Hyatt wrote: > > A couple comments that may be helpful... > > > > -This is also PR 64117 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64117) > > > > -I submitted a patch last year for that but did not get any response > > (https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635648.html). > > Oops, sorry, wasn't aware of that. > Note, I've already committed it. > > > I guess I never pinged it because I am still trying to ping two other > > ones :). My patch did not switch to vec so it was not as nice as this > > one. I wonder though, if some of the testcases I added could be > > incorporated? In particular the testcase from my patch > > Maybe. > > > pragma-diagnostic-3.C I believe will still be failing after this one. > > There is an issue with C++ because it processes the pragmas twice, > > once in early mode and once in normal mode, that makes it do the wrong > > thing for this case: > > > > t.h: > > ---- > > #pragma GCC diagnostic push > > #pragma GCC diagnostic ignored... > > //no pop at end of the file > > > > t.c > > ---- > > #include "t.h" > > #pragma GCC diagnostic pop > > //expect to be at the initial state here, but won't be if t.h is a PCH > > > > In my patch I had separated the PCH restore from a more general "state > > restore" logic so that the C++ frontend can restore the state after > > the first pass through the data. > > People shouldn't be doing this, without PCH or with it, and especially not > with PCH, that is simply too ugly. > That said, this was the reason why I have saved also the m_push_list > vector and not just the history. If that isn't enough and there is some > other state on the libcpp side, I'd think we should go with a sorry and tell > the user not to do this with PCH. It becomes a nightmare already if e.g. > the command line -Werror=something -Wno-error=somethingelse overrides > differ between the PCH creation and PCH reading (my first version of > the patch saved m_classify_diagnostic array but that broke tests relying > on those differences). > > Jakub > Definitely agreed that people shouldn't be doing this :). I think I felt like I should support it only because it was one of my patches that caused C++ frontend to process the pragmas twice, but yeah I can see how it's probably not worth the complexity. One other note is that there's also been requests for the diagnostic pragmas to make it through the LTO streaming process so that they can be suppressed in the LTO frontend as well. That would also require some more general support for saving the state, but it's another topic. -Lewis
--- gcc/diagnostic.h.jj 2024-09-26 15:42:05.696731787 +0200 +++ gcc/diagnostic.h 2024-09-26 17:32:16.419161700 +0200 @@ -256,6 +256,9 @@ public: diagnostic_t update_effective_level_from_pragmas (diagnostic_info *diagnostic) const; + int pch_save (FILE *); + int pch_restore (FILE *); + private: /* Each time a diagnostic's classification is changed with a pragma, we record the change and the location of the change in an array of @@ -551,6 +554,18 @@ public: const char *, const char *, va_list *, diagnostic_t) ATTRIBUTE_GCC_DIAG(7,0); + int + pch_save (FILE *f) + { + return m_option_classifier.pch_save (f); + } + + int + pch_restore (FILE *f) + { + return m_option_classifier.pch_restore (f); + } + private: void error_recursion () ATTRIBUTE_NORETURN; --- gcc/diagnostic.cc.jj 2024-09-26 16:11:51.664502977 +0200 +++ gcc/diagnostic.cc 2024-09-26 22:59:05.204135316 +0200 @@ -156,6 +156,46 @@ diagnostic_option_classifier::fini () m_push_list.release (); } +/* Save the diagnostic_option_classifier state to F for PCH + output. Returns 0 on success, -1 on error. */ + +int +diagnostic_option_classifier::pch_save (FILE *f) +{ + unsigned int lengths[2] = { m_classification_history.length (), + m_push_list.length () }; + if (fwrite (lengths, sizeof (lengths), 1, f) != 1 + || fwrite (m_classification_history.address (), + sizeof (diagnostic_classification_change_t), + lengths[0], f) != lengths[0] + || fwrite (m_push_list.address (), sizeof (int), + lengths[1], f) != lengths[1]) + return -1; + return 0; +} + +/* Read the diagnostic_option_classifier state from F for PCH + read. Returns 0 on success, -1 on error. */ + +int +diagnostic_option_classifier::pch_restore (FILE *f) +{ + unsigned int lengths[2]; + if (fread (lengths, sizeof (lengths), 1, f) != 1) + return -1; + gcc_checking_assert (m_classification_history.is_empty ()); + gcc_checking_assert (m_push_list.is_empty ()); + m_classification_history.safe_grow (lengths[0]); + m_push_list.safe_grow (lengths[1]); + if (fread (m_classification_history.address (), + sizeof (diagnostic_classification_change_t), + lengths[0], f) != lengths[0] + || fread (m_push_list.address (), sizeof (int), + lengths[1], f) != lengths[1]) + return -1; + return 0; +} + /* Save all diagnostic classifications in a stack. */ void --- gcc/c-family/c-pch.cc.jj 2024-02-01 16:00:37.149739756 +0100 +++ gcc/c-family/c-pch.cc 2024-09-26 18:12:31.097700113 +0200 @@ -28,6 +28,7 @@ along with GCC; see the file COPYING3. #include "c-pragma.h" #include "langhooks.h" #include "hosthooks.h" +#include "diagnostic.h" /* This is a list of flag variables that must match exactly, and their names for the error message. The possible values for *flag_var must @@ -178,7 +179,8 @@ c_common_write_pch (void) cpp_write_pch_state (parse_in, pch_outfile); timevar_pop (TV_PCH_CPP_SAVE); - if (fseek (pch_outfile, 0, SEEK_SET) != 0 + if (global_dc->pch_save (pch_outfile) < 0 + || fseek (pch_outfile, 0, SEEK_SET) != 0 || fwrite (get_ident (), IDENT_LENGTH, 1, pch_outfile) != 1) fatal_error (input_location, "cannot write %s: %m", pch_file); @@ -359,6 +361,10 @@ c_common_read_pch (cpp_reader *pfile, co linemap_line_start (line_table, saved_loc.line, 0); timevar_pop (TV_PCH_CPP_RESTORE); + + if (global_dc->pch_restore (f) < 0) + fatal_error (input_location, "cannot read %s: %m", name); + fclose (f); if (cpp_result != 0) --- gcc/testsuite/g++.dg/pch/pr116847.C.jj 2024-09-26 18:29:02.679292807 +0200 +++ gcc/testsuite/g++.dg/pch/pr116847.C 2024-09-26 18:27:42.679374530 +0200 @@ -0,0 +1,10 @@ +// { dg-do compile { target c++11 } } + +#include "pr116847.H" + +int a = S<0>::bar (); + +int +main () +{ +} --- gcc/testsuite/g++.dg/pch/pr116847.Hs.jj 2024-09-26 18:28:58.866344362 +0200 +++ gcc/testsuite/g++.dg/pch/pr116847.Hs 2024-09-26 18:27:35.579470530 +0200 @@ -0,0 +1,8 @@ +[[deprecated]] int foo () { return 42; } +template <int N> +struct S { +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" +static int bar () { return foo (); } +#pragma GCC diagnostic pop +};