llvm dev - Apr 2015 - [LLVMdev] [WinEH] Cloning blocks that reference non-cloned PHI nodes

Hi Reid and David,

I just wanted to give you a heads up that I'm currently working on a problem
where the WinEHPrepare outlining code stumbles and asserts while cloning code
that references extracted landing pad values via PHI nodes that are in
intermediate blocks that aren't being cloned.  The test I'm looking at
fails with an assertion claiming that llvm.eh.begincatch was called twice inside
a handler.

I have an idea for how to address this (giving the cloning director a chance to
replace operands before an instruction is remapped).  I'll put something up
for review as soon as I have it working.  In the mean time I wanted to make sure
you weren't working on the same problem.

I'm seeing it with some C++ EH tests I'm working on, but I think it may
be possible for it to happen with SEH cleanup.

If you're curious, here's a reproducer.

int main(void) {
  try {
    try {
      throw 'a';
    } catch (char c) {
      printf("%c\n", c);
    }
    throw 1;
  } catch(int x) {
    printf("%d\n", x);
  } catch(...) {
    printf("...\n");
  }

  try {
    try {
      throw 'b';
    } catch (char c) {
      printf("%c\n", c);
    }
    throw 2;
  } catch(int x) {
    printf("%d\n", x);
  } catch (char c) {
    printf("%c\n", c);
  } catch(...) {
    printf("...\n");
  }
  return 0;
}

-Andy

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150410/bd114cbc/attachment.html>

Sounds reasonable. Honestly, we've been having some serious whiteboard
discussions about how to get out of doing all this selector pattern
matching. I think that's basically the most fragile part of winehprepare,
and if we can add some new constructs that wrap that up in something opaque
to mid-level passes, that would be great.

Unfortunately, I haven't really been able to get enough consensus to write
something down. If I can't convince my coworkers that we need a new
construct in person, there's not much point in writing up a detailed RFC. :(

Here's the gist of the new idea:
- Leave invoke alone, too many optimizations care about it
- Add a new 'landingswitch' (name under debate) instruction that fills
the
landingpad role and is also a terminator
- landingswitch clauses have some fixed operands (RTTI, handler label) and
some variable number of operands as required by the EH personality (catch
object, adjectives)
- Allow the 'resume' instruction to have 0, 1, or 2 successors, one of
which can be a landingpad.
- Add a 'resume' clause to landingswitches to allow them to continue the
unwind process at another landingpad in the same function

I've tried to sell this idea, but I'm getting push back suggesting that
we
fix it a different way. The most compelling suggestion is to have the
existing landingpad instruction produce an i8* selector instead of an i32
selector, and then we indirectbr on it. This would save the selector
pattern matching that we do today, but I don't particularly like it.
Optimization passes can do basic block commoning, tail merging, and hoist
instructions between the landingpad and the indirectbr, and I don't like
dealing with that.

I also don't like it because we'd need to teach the inliner about
indirectbr, which today it refuses to inline.

On Fri, Apr 10, 2015 at 3:45 PM, Kaylor, Andrew <andrew.kaylor at
intel.com>
wrote:
>  Hi Reid and David,
>
>
>
> I just wanted to give you a heads up that I’m currently working on a
> problem where the WinEHPrepare outlining code stumbles and asserts while
> cloning code that references extracted landing pad values via PHI nodes
> that are in intermediate blocks that aren’t being cloned.  The test I’m
> looking at fails with an assertion claiming that llvm.eh.begincatch was
> called twice inside a handler.
>
>
>
> I have an idea for how to address this (giving the cloning director a
> chance to replace operands before an instruction is remapped).  I’ll put
> something up for review as soon as I have it working.  In the mean time I
> wanted to make sure you weren’t working on the same problem.
>
>
>
> I’m seeing it with some C++ EH tests I’m working on, but I think it may be
> possible for it to happen with SEH cleanup.
>
>
>
> If you’re curious, here’s a reproducer.
>
>
>
> int main(void) {
>
>   try {
>
>     try {
>
>       throw 'a';
>
>     } catch (char c) {
>
>       printf("%c\n", c);
>
>     }
>
>     throw 1;
>
>   } catch(int x) {
>
>     printf("%d\n", x);
>
>   } catch(...) {
>
>     printf("...\n");
>
>   }
>
>
>
>   try {
>
>     try {
>
>       throw 'b';
>
>     } catch (char c) {
>
>       printf("%c\n", c);
>
>     }
>
>     throw 2;
>
>   } catch(int x) {
>
>     printf("%d\n", x);
>
>   } catch (char c) {
>
>     printf("%c\n", c);
>
>   } catch(...) {
>
>     printf("...\n");
>
>   }
>
>   return 0;
>
> }
>
>
>
> -Andy
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150410/2a33a49f/attachment.html>

I can definitely see why you’d want to work around this selector matching stuff,
but I can also understand why it’s difficult to get traction with the
alternatives.  I’ll give it some thought to see if I can come up with something
worth adding to the conversation, but in the mean time I’ll keep trying to fix
what I’ve got.

BTW, I’ve got five other modes of failure from the suite of 22 C++ EH tests I
was running.  Do you want me to file Bugzilla reports or just keep them on my
own plate for now?

The quick summary is as follows:

Assert in CloneAndPruneIntoFromInst because of an unrecognized PHINode (almost
certainly a different manifestation of the problem I’m currently working on)
Assert in WinEHPrepare::addStubInvokeToHandlerIfNeed because no return
instruction was found.
Assert in Win64Exception::emitCXXFrameHandler3Table because CatchHigh >
TBME.TryHigh
Assert in MCWinCOFFStreamer::EmitLabel because the symbol being emitted was
previously defined.
Assert in X86DAGToDAGISel:: getAddressOperands because non-zero placement is
ignored with ES.

-Andy


From: Reid Kleckner [mailto:rnk at google.com]
Sent: Friday, April 10, 2015 4:06 PM
To: Kaylor, Andrew
Cc: David Majnemer <david.majnemer at gmail.com> (david.majnemer at
gmail.com); LLVM Developers Mailing List
Subject: Re: [WinEH] Cloning blocks that reference non-cloned PHI nodes

Sounds reasonable. Honestly, we've been having some serious whiteboard
discussions about how to get out of doing all this selector pattern matching. I
think that's basically the most fragile part of winehprepare, and if we can
add some new constructs that wrap that up in something opaque to mid-level
passes, that would be great.

Unfortunately, I haven't really been able to get enough consensus to write
something down. If I can't convince my coworkers that we need a new
construct in person, there's not much point in writing up a detailed RFC. :(

Here's the gist of the new idea:
- Leave invoke alone, too many optimizations care about it
- Add a new 'landingswitch' (name under debate) instruction that fills
the landingpad role and is also a terminator
- landingswitch clauses have some fixed operands (RTTI, handler label) and some
variable number of operands as required by the EH personality (catch object,
adjectives)
- Allow the 'resume' instruction to have 0, 1, or 2 successors, one of
which can be a landingpad.
- Add a 'resume' clause to landingswitches to allow them to continue the
unwind process at another landingpad in the same function

I've tried to sell this idea, but I'm getting push back suggesting that
we fix it a different way. The most compelling suggestion is to have the
existing landingpad instruction produce an i8* selector instead of an i32
selector, and then we indirectbr on it. This would save the selector pattern
matching that we do today, but I don't particularly like it. Optimization
passes can do basic block commoning, tail merging, and hoist instructions
between the landingpad and the indirectbr, and I don't like dealing with
that.

I also don't like it because we'd need to teach the inliner about
indirectbr, which today it refuses to inline.

On Fri, Apr 10, 2015 at 3:45 PM, Kaylor, Andrew <andrew.kaylor at
intel.com<mailto:andrew.kaylor at intel.com>> wrote:
Hi Reid and David,

I just wanted to give you a heads up that I’m currently working on a problem
where the WinEHPrepare outlining code stumbles and asserts while cloning code
that references extracted landing pad values via PHI nodes that are in
intermediate blocks that aren’t being cloned.  The test I’m looking at fails
with an assertion claiming that llvm.eh.begincatch was called twice inside a
handler.

I have an idea for how to address this (giving the cloning director a chance to
replace operands before an instruction is remapped).  I’ll put something up for
review as soon as I have it working.  In the mean time I wanted to make sure you
weren’t working on the same problem.

I’m seeing it with some C++ EH tests I’m working on, but I think it may be
possible for it to happen with SEH cleanup.

If you’re curious, here’s a reproducer.

int main(void) {
  try {
    try {
      throw 'a';
    } catch (char c) {
      printf("%c\n", c);
    }
    throw 1;
  } catch(int x) {
    printf("%d\n", x);
  } catch(...) {
    printf("...\n");
  }

  try {
    try {
      throw 'b';
    } catch (char c) {
      printf("%c\n", c);
    }
    throw 2;
  } catch(int x) {
    printf("%d\n", x);
  } catch (char c) {
    printf("%c\n", c);
  } catch(...) {
    printf("...\n");
  }
  return 0;
}

-Andy


-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150410/2422f6d2/attachment.html>

Just as an off-the-cuff idea that I haven’t thought through at all, what would
you think of the possibility of replacing this:

  %12 = call i32 @llvm.eh.typeid.for(i8* bitcast (%eh.CatchHandlerType*
@llvm.eh.handlertype.H.0 to i8*))
  %matches7 = icmp eq i32 %sel6, %12

with this:

  %matches7 = call i32 @llvm.eh.cmp.selector(i32 %sel6, i8* bitcast
(%eh.CatchHandlerType* @llvm.eh.handlertype.H.0 to i8*))

If we did that, the outlining code could just assume %sel6 was the current
selector (because we don’t clone into nested landing pads) and we wouldn’t need
to track it at all.

-Andy

From: Reid Kleckner [mailto:rnk at google.com]
Sent: Friday, April 10, 2015 4:06 PM
To: Kaylor, Andrew
Cc: David Majnemer <david.majnemer at gmail.com> (david.majnemer at
gmail.com); LLVM Developers Mailing List
Subject: Re: [WinEH] Cloning blocks that reference non-cloned PHI nodes

Sounds reasonable. Honestly, we've been having some serious whiteboard
discussions about how to get out of doing all this selector pattern matching. I
think that's basically the most fragile part of winehprepare, and if we can
add some new constructs that wrap that up in something opaque to mid-level
passes, that would be great.

Unfortunately, I haven't really been able to get enough consensus to write
something down. If I can't convince my coworkers that we need a new
construct in person, there's not much point in writing up a detailed RFC. :(

Here's the gist of the new idea:
- Leave invoke alone, too many optimizations care about it
- Add a new 'landingswitch' (name under debate) instruction that fills
the landingpad role and is also a terminator
- landingswitch clauses have some fixed operands (RTTI, handler label) and some
variable number of operands as required by the EH personality (catch object,
adjectives)
- Allow the 'resume' instruction to have 0, 1, or 2 successors, one of
which can be a landingpad.
- Add a 'resume' clause to landingswitches to allow them to continue the
unwind process at another landingpad in the same function

I've tried to sell this idea, but I'm getting push back suggesting that
we fix it a different way. The most compelling suggestion is to have the
existing landingpad instruction produce an i8* selector instead of an i32
selector, and then we indirectbr on it. This would save the selector pattern
matching that we do today, but I don't particularly like it. Optimization
passes can do basic block commoning, tail merging, and hoist instructions
between the landingpad and the indirectbr, and I don't like dealing with
that.

I also don't like it because we'd need to teach the inliner about
indirectbr, which today it refuses to inline.

On Fri, Apr 10, 2015 at 3:45 PM, Kaylor, Andrew <andrew.kaylor at
intel.com<mailto:andrew.kaylor at intel.com>> wrote:
Hi Reid and David,

I just wanted to give you a heads up that I’m currently working on a problem
where the WinEHPrepare outlining code stumbles and asserts while cloning code
that references extracted landing pad values via PHI nodes that are in
intermediate blocks that aren’t being cloned.  The test I’m looking at fails
with an assertion claiming that llvm.eh.begincatch was called twice inside a
handler.

I have an idea for how to address this (giving the cloning director a chance to
replace operands before an instruction is remapped).  I’ll put something up for
review as soon as I have it working.  In the mean time I wanted to make sure you
weren’t working on the same problem.

I’m seeing it with some C++ EH tests I’m working on, but I think it may be
possible for it to happen with SEH cleanup.

If you’re curious, here’s a reproducer.

int main(void) {
  try {
    try {
      throw 'a';
    } catch (char c) {
      printf("%c\n", c);
    }
    throw 1;
  } catch(int x) {
    printf("%d\n", x);
  } catch(...) {
    printf("...\n");
  }

  try {
    try {
      throw 'b';
    } catch (char c) {
      printf("%c\n", c);
    }
    throw 2;
  } catch(int x) {
    printf("%d\n", x);
  } catch (char c) {
    printf("%c\n", c);
  } catch(...) {
    printf("...\n");
  }
  return 0;
}

-Andy


-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150410/623e5180/attachment.html>