This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, c-family] Fix a PCH thinko (and thus PR61250).


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]