| Message ID | D1BC39FE-B603-4122-8AD2-F64BCE6C337A@sandoe.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | [c-family] Fix a PCH thinko (and thus PR61250). | expand |
On 8/22/19 1:59 PM, Iain Sandoe wrote: > Hi, > > When we are parsing a source file, the very first token might > be a PRAGMA_GCC_PCH_PREPROCESS. This indicates that we are going > read in a PCH file (named as the value of the pragma). If we don't > see this pragma, then we know that it's OK to release any resources > that the host might have set aside for the PCH file. > > There is a thinko in the current implementation, in that the decision > to release resources is happening unconditionally right after the first > token is extracted but before it's been checked or acted upon. > > This leads to the pch bug on Darwin, because we actually do release > resources - which are subsequently (reasonably) assumed to be available > when reading a PCH file. We then get random crashes or hangs depending > on the interaction between unmmap and malloc. > > The bug is present everywhere but doesn't show on (say) Linux, since > the release of PCH resources is a NOP there. > > This effects all the c-family front ends, because they all use c_lex_with_flags () > to implement this. > > The solution is to check for the PRAGMA_GCC_PCH_PREPROCESS and only call > c_common_no_more_pch () when that is not the first token. > > A secondary effect of the collection is that the name of the PCH file > can be collected during the ggc_pch_read() reset of state. Therefore > we should issue any diagnostic that might name the file before the > collections are triggered. > > Tested on x86-64-darwin16/18 (where the pch.exp tests fail roughly half the > time for any parallel testing) and pass reliably without it. > Tested on x86_64,powerpc-linux-gnu with no regressions (and no expectation > of any progression either). > > Since the fix is in common code, it needs the ack of both C and C++ to apply > (I’m obviously OK with applying it from the Objective-C/C++ PoV) > > OK for trunk? > > given that this is a show-stopper for PCH + -save-temps I would also like > to fix it on the open branches? > > thanks > Iain > > gcc/c-family/ > > 2019-08-22 Iain Sandoe <iain@sandoe.co.uk> > > PR pch/61250 > * c-lex.c (c_lex_with_flags): Don't call > c_common_no_more_pch () from here. > > gcc/c/ > > 2019-08-22 Iain Sandoe <iain@sandoe.co.uk> > > PR pch/61250 > * c-parser.c (c_parse_file): Call c_common_no_more_pch () > after determining that the first token is not > PRAGMA_GCC_PCH_PREPROCESS. > > gcc/cp/ > > 2019-08-22 Iain Sandoe <iain@sandoe.co.uk> > > PR pch/61250 > * parser.c (cp_parser_initial_pragma): Call c_common_no_more_pch () > after determining that the first token is not > PRAGMA_GCC_PCH_PREPROCESS. > > gcc/ > > 2019-08-22 Iain Sandoe <iain@sandoe.co.uk> > > PR pch/61250 > * ggc-page.c (ggc_pch_read): Read the ggc_pch_ondisk structure > and issue any diagnostics needed before collecting the pre-PCH > state. OK jeff
On Thu, Aug 22, 2019 at 2:39 PM Jeff Law <law@redhat.com> wrote: > > On 8/22/19 1:59 PM, Iain Sandoe wrote: > > Hi, > > > > When we are parsing a source file, the very first token might > > be a PRAGMA_GCC_PCH_PREPROCESS. This indicates that we are going > > read in a PCH file (named as the value of the pragma). If we don't > > see this pragma, then we know that it's OK to release any resources > > that the host might have set aside for the PCH file. > > > > There is a thinko in the current implementation, in that the decision > > to release resources is happening unconditionally right after the first > > token is extracted but before it's been checked or acted upon. > > > > This leads to the pch bug on Darwin, because we actually do release > > resources - which are subsequently (reasonably) assumed to be available > > when reading a PCH file. We then get random crashes or hangs depending > > on the interaction between unmmap and malloc. > > > > The bug is present everywhere but doesn't show on (say) Linux, since > > the release of PCH resources is a NOP there. > > > > This effects all the c-family front ends, because they all use c_lex_with_flags () > > to implement this. > > > > The solution is to check for the PRAGMA_GCC_PCH_PREPROCESS and only call > > c_common_no_more_pch () when that is not the first token. > > > > A secondary effect of the collection is that the name of the PCH file > > can be collected during the ggc_pch_read() reset of state. Therefore > > we should issue any diagnostic that might name the file before the > > collections are triggered. > > > > Tested on x86-64-darwin16/18 (where the pch.exp tests fail roughly half the > > time for any parallel testing) and pass reliably without it. > > Tested on x86_64,powerpc-linux-gnu with no regressions (and no expectation > > of any progression either). > > > > Since the fix is in common code, it needs the ack of both C and C++ to apply > > (I’m obviously OK with applying it from the Objective-C/C++ PoV) > > > > OK for trunk? > > > > given that this is a show-stopper for PCH + -save-temps I would also like > > to fix it on the open branches? > > > > thanks > > Iain > > > > gcc/c-family/ > > > > 2019-08-22 Iain Sandoe <iain@sandoe.co.uk> > > > > PR pch/61250 > > * c-lex.c (c_lex_with_flags): Don't call > > c_common_no_more_pch () from here. > > > > gcc/c/ > > > > 2019-08-22 Iain Sandoe <iain@sandoe.co.uk> > > > > PR pch/61250 > > * c-parser.c (c_parse_file): Call c_common_no_more_pch () > > after determining that the first token is not > > PRAGMA_GCC_PCH_PREPROCESS. > > > > gcc/cp/ > > > > 2019-08-22 Iain Sandoe <iain@sandoe.co.uk> > > > > PR pch/61250 > > * parser.c (cp_parser_initial_pragma): Call c_common_no_more_pch () > > after determining that the first token is not > > PRAGMA_GCC_PCH_PREPROCESS. > > > > gcc/ > > > > 2019-08-22 Iain Sandoe <iain@sandoe.co.uk> > > > > PR pch/61250 > > * ggc-page.c (ggc_pch_read): Read the ggc_pch_ondisk structure > > and issue any diagnostics needed before collecting the pre-PCH > > state. > OK OK with me, too. Joseph recently mentioned being swamped with reviews, so I wouldn't worry about waiting for his review. Jason
diff --git a/gcc/c-family/c-lex.c b/gcc/c-family/c-lex.c index 851fd704e5..e3c602fbb8 100644 --- a/gcc/c-family/c-lex.c +++ b/gcc/c-family/c-lex.c @@ -394,7 +394,6 @@ enum cpp_ttype c_lex_with_flags (tree *value, location_t *loc, unsigned char *cpp_flags, int lex_flags) { - static bool no_more_pch; const cpp_token *tok; enum cpp_ttype type; unsigned char add_flags = 0; @@ -628,12 +627,6 @@ c_lex_with_flags (tree *value, location_t *loc, unsigned char *cpp_flags, if (cpp_flags) *cpp_flags = tok->flags | add_flags; - if (!no_more_pch) - { - no_more_pch = true; - c_common_no_more_pch (); - } - timevar_pop (TV_CPP); return type; diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index 81919a89cc..cf973b3c8d 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -20232,6 +20232,8 @@ c_parse_file (void) if (c_parser_peek_token (&tparser)->pragma_kind == PRAGMA_GCC_PCH_PREPROCESS) c_parser_pragma_pch_preprocess (&tparser); + else + c_common_no_more_pch (); the_parser = ggc_alloc<c_parser> (); *the_parser = tparser; diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index dbbfe1dbc2..ab8a0cabb4 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -41353,7 +41353,10 @@ cp_parser_initial_pragma (cp_token *first_token) cp_lexer_get_preprocessor_token (NULL, first_token); if (cp_parser_pragma_kind (first_token) != PRAGMA_GCC_PCH_PREPROCESS) - return; + { + c_common_no_more_pch (); + return; + } cp_lexer_get_preprocessor_token (NULL, first_token); if (first_token->type == CPP_STRING) diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c index a2736bc1df..220f20c5cf 100644 --- a/gcc/ggc-page.c +++ b/gcc/ggc-page.c @@ -2556,6 +2556,9 @@ ggc_pch_read (FILE *f, void *addr) count_old_page_tables = G.by_depth_in_use; + if (fread (&d, sizeof (d), 1, f) != 1) + fatal_error (input_location, "cannot read PCH file: %m"); + /* We've just read in a PCH file. So, every object that used to be allocated is now free. */ clear_marks (); @@ -2584,8 +2587,6 @@ ggc_pch_read (FILE *f, void *addr) /* Allocate the appropriate page-table entries for the pages read from the PCH file. */ - if (fread (&d, sizeof (d), 1, f) != 1) - fatal_error (input_location, "cannot read PCH file: %m"); for (i = 0; i < NUM_ORDERS; i++) {