[PATCH] Fix missed exception spec checks and crashes

[Alp Toker]

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