[PATCH] Fix missed exception spec checks and crashes

[Richard Smith]

On Thu, Oct 17, 2013 at 7:08 PM, Alp Toker <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. =(


>
> 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>> 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>
> >>     http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >>
> >>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131017/e4182784/attachment.html>