[PATCH] D102817: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 2

[Joseph Tremoulet via Phabricator via llvm-commits]

JosephTremoulet added a comment.

Thanks for the explanations and your patience.  This patch LGTM, modulo resolution of @efriedma's concerns (so I'll leave it to him to mark accepted).



================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:267-269
+//   Side exits can ONLY jump into parent scopes (lower state number).
+//   Thus, when a block succeeds various states from its predecessors,
+//     the lowest State triumphs others.
----------------
tentzen wrote:
> JosephTremoulet wrote:
> > tentzen wrote:
> > > JosephTremoulet wrote:
> > > > tentzen wrote:
> > > > > JosephTremoulet wrote:
> > > > > > I think you're saying here that it's legal for a side exit to target a successor with any ancestor state.  But also you're assigning each such successor the exact parent state of its least predecessor ("least" by comparing state numbers).  How do you know that it shouldn't be the grandparent (or any other ancestor) of the least predecessor?
> > > > > the new state only assigned to successor when it's smaller than successor's current state.  See the first line in the while loop. 
> > > > > if ( ... && EHInfo.BlockToStateMap[BB] <= State) ..
> > > > Yes I see that you're taking the state of the least connected predecessor.  What I'm not following is how you know that the least connected predecessor is a sibling, rather than say a neice -- what if the sibling ends in unreached, for example?  Something like:
> > > > 
> > > > ```
> > > > B1: (state = 0
> > > > { // new scope
> > > > B2: (state = 1)
> > > >   { // new scope
> > > >   B2: (state = 2)
> > > >      { // new scope
> > > >      B3: (state = 3)
> > > >         if (_) {
> > > >         B4:
> > > >             goto B7;
> > > >         }
> > > >         B5: (state = 3)
> > > >         __assume(0);
> > > >      } // exit scope
> > > >      B6: (state TBD)
> > > >   } // exit scope
> > > >  B7: (state TBD)
> > > > } // exit scope
> > > > B8: (state TBD)
> > > > ```
> > > > 
> > > > And maybe optimization turned __assume(0) into unreachable and got rid of B6 entirely.  So then B4 is the only predecessor of B7.  Doesn't that mean we'll assign state 2 to B7?  But B7 should have state 1.  How do we know this doesn't happen?
> > > > 
> > > > 
> > > >   
> > > Very good point. In this case, B7 actually is a side-entry that is led by a block of seh_scope_begin() intrinsic (see CpodeGen/CGStmt.cpp change in Part-1 patch).  As such, the state will be reset via EHInfo.InvokeStateMap[] when real code blocks are proceeded. 
> > I apologize for the ridiculous delay in responding, until just now I was under the mistaken impression that you hadn't replied.
> > 
> > I'm still confused by the logic here.  Your comment states "Side exits can ONLY jump into parent scopes (lower state number)".  My example had a jump to a grandparent, and you pointed out that its destination would be marked as a side entry.  Are those the only two cases, would it be equally correct to say "Side exits can ONLY jump to either the immediate parent scope or a side-entry of another ancestor" ?  Or is there a way to go straight to grandparent that doesn't get a side entry?
> > 
> > I think an equivalent question is:  at the continue on line 300, could you assert `EHInfo.BlockToStateMap[BB] == State || (block is side entry)` ?
> Sorry for the confusion. the comment "jump into parent scopes" here does not imply it's the immediate parent. it can be grandparents, etc as in your example. Will fix the comment.
> Those two are actually refer to the same scenario.  Jumping into a protected parant/grandparent's scope induces a warning, and under that situation the target label is marked as side-entry (JumpDiagnostics.cpp change in Part-1 patch). 
> Jumping into a protected [ancestor scope other than the immediate parent scope] ... [can't occur unless] the target label is marked as side-entry

That is the key point I was missing.  Yes, I think it would be great to include that in the comment and/or as an assertion.


> a protected parant/grandparent's

What happens if an unprotected block is only reachable from side-exits of protected regions?  Do we end up protecting it, and is that okay?  This too would be good to include in comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102817/new/

https://reviews.llvm.org/D102817