[PATCH] Fix missed exception spec checks and crashes
Alp Toker
alp at nuanti.com
Thu Oct 17 22:58:54 PDT 2013
On 18/10/2013 06:21, Richard Smith wrote:
> On Thu, Oct 17, 2013 at 7:08 PM, Alp Toker <alp at nuanti.com
> <mailto:alp at nuanti.com>> wrote:
>
> Hi Richard,
>
> The simplification didn't work out because at least
> CheckOverridingFunctionExceptionSpec() pushes back specs where the
> parent isn't fully defined yet, causing an infinite loop. This was hit
> in the libc++ selfhost build.
>
> So the best simplification I can offer is to remove the outer loop
> that
> was in the original patch. Swapping and processing seems the most
> reliable way to delay late additions to the list for further checking
> given the assumptions in existing code.
>
> I've attached a rework that now passes all tests and the libc++
> bootstrap as well. Does this look OK to you?
>
>
> Yes, LGTM, thanks for the cleanup =)
>
> It'd be nice to restructure this so that we only try to perform the
> dtor checks for the class we just completed, rather than running all
> the pending dtor checks and putting back the ones which we're not
> ready for yet. This seems likely to blow up in someone's face again. =(
Agree fully, the delayed exception checks need tightening, as do the
FIXMEs in Sema related to missing point-of-instantiation records. At
least with the functionality stabilised we can start to look at that.
Landed in r192947. Thanks for the review!
Alp.
>
>
>
> Alp.
>
>
> On 17/10/2013 20:21, Alp Toker wrote:
> > Landed in r192914 with suggested simplifications.
> >
> > With the pop_back() approach the two loops are no longer tied --
> in the
> > off chance this becomes a problem due to the second mutating the
> first,
> > we can take a look at using the heavier approach in the original
> patch
> > again.
> >
> > Thanks for the review Richard!
> >
> > Marshall, let us know if this covers all bases.
> >
> > Alp.
> >
> > On 07/10/2013 21:36, Richard Smith wrote:
> >> On Sun, Oct 6, 2013 at 11:54 AM, Alp Toker <alp at nuanti.com
> <mailto:alp at nuanti.com>
> >> <mailto:alp at nuanti.com <mailto:alp at nuanti.com>>> wrote:
> >>
> >> This patch fixes a crash and rejects-valid the libcxx
> developers were
> >> hitting.
> >>
> >> While investigating the defaulted member cases I noticed a
> similar
> >> problem in delayed destructor checking, with checks
> accumulating but
> >> never running or getting cleared if they were instantiated
> within an
> >> exception spec.
> >>
> >> I'll appreciate thoughts on both parts of the fix,
> particularly on the
> >> correctness of moving the destructor checks from
> >> ActOnFinishCXXMemberDecls() to
> ActOnFinishDelayedMemberInitializers()
> >> alongside the member checks, which seems to be the only way
> to get
> >> them
> >> handled consistently.
> >>
> >>
> >> Both changes look correct to me. Rather than swapping the worklists
> >> out in each iteration of the loop, can you instead process them in
> >> reverse order (and pop them as you go)? This should make the code a
> >> little simpler (and a little more efficient too).
> >>
> >>
> >> Marshall, can you think of anything else to excercise in
> the test
> >> case?
> >>
> >> I've attached the patch and crash backtrace. Here's the
> changelog:
> >>
> >> Delayed exception specification checking for defaulted
> members and
> >> virtual destructors are both susceptible to mutation during
> >> iteration so
> >> we need to swap and process the worklists iteratively.
> >>
> >> This resolves both accepts-invalid and rejects-valid issues and
> >> moreover
> >> fixes potential invalid memory access as the contents of
> the vectors
> >> change during iteration and recursive template instantiation.
> >>
> >> This patch also adds two assertions at end of TU to ensure no
> >> specs are
> >> left unchecked as was happenning before the fix, plus a
> test case from
> >> Marshall Clow for the defaulted member crash extracted from
> the libcxx
> >> headers.
> >>
> >> Alp.
> >>
> >> --
> >> http://www.nuanti.com
> >> the browser experts
> >>
> >>
> >> _______________________________________________
> >> cfe-commits mailing list
> >> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
> <mailto:cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>>
> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >>
> >>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list