Hi, Is there any more concerns, feedbacks or suggestions?
thanks,

In D80344#2410417, @pengfei wrote:

In D80344#2407305, @tentzen wrote:

In D80344#2407250, @pengfei wrote:

Do we need to consider FP exceptions in _try block?

Yes, FP exception is handled as long as FP exceptions are not disabled (Ex via _controlfp() runtime) and FP exception code is filtered & handled via ___except() statement (Ex, ___except(GetExceptionCode()==EXCEPTION_FLT_INEXACT_RESULT)).

I see. If this is the case, you may need to assign FPE_Strict to _try block to preserve FP instructions' order.

In D80344#2407309, @belliash wrote:

Does it really work for x86_64 architecture only? I have tried using this with x86 build and produced binaries looks like this should work on this platform as well, but I have not tried running produced executables to see the results yet tbh.

In D80344#2407250, @pengfei wrote:

Do we need to consider FP exceptions in _try block?

Per Joseph's feedback, further clarify the semantic of llvm.seh.try.begin and llvm.seh.try.end.

Fixed the clang fault when built without '-feh-asynch' option.
EHStack's CGF field must be initialized even without -feh-asynch.

per review feedback from John McCall and others, this update includes:

OK, great.
Summarize our discussion above, I will follow up with:

In D80344#2291926, @rjmccall wrote:

Okay. I think we're on the same page about representation now. If you can come up with a good replacement for "eha" in the intrinsic names, I think this is pretty much ready to go.

In D80344#2291688, @rjmccall wrote:

In D80344#2291456, @tentzen wrote:

Reordering is NOT okay for instructions 'directly' within a _try. 'directly' here means it's the code from source code originally, i.e., inlined code or the code in callees is not included. Disabling inline in general is good, but not necessary.
As said above, SEH semantic only applies to code "directly" under SEH scope for few obvious reasons (Ex, indirect callees or callees not in the same module wont be visible). if faulty instructions are allowed to be reordered inside a callee, those instructions are allowed to do so even after they are inlined into _try.

I feel almost certain that this is wrong. Let me try to explain. It would be helpful to have a more precise statement of semantics than I can find online, but we can start from the basics.

"X and Y can be reordered" is how optimizer folks talk, but it's not generally how semantics are specified. As a general rule (ignoring sequence points and concurrency for simplicity), C and C++ require operations to be executed in a particular order on the abstract machine, and there are no rules explicitly sanctioning reordering. Instead, the "as if" rule gives the implementation broad leeway to do things differently from how they're specified on the abstract machine, as long as the difference cannot be observed by a valid program. Faults generally correspond to conditions that have undefined behavior under the language, and so the implementation may assume they do not happen in a valid program, and so the consequences of them happening cannot be observed by a valid program, and so faults can be ignored when deciding whether to reorder operations, which allows a lot of reordering.

_try should be understood as fully defining the behavior of faulting operations written within its scope so that they have the defined semantics of resuming execution in the _except clause. Because faults have the defined behavior of ending execution of the _try block, whether the fault occurred is observable, so the "as if" leeway stops applying. Thus, other operations with side-effects cannot be reordered with potentially faulty operations written within the _try, no more than they could be reordered with a break statement. That applies whether those other operations are written within the _try or not; it's just that potentially-faulty operations written within the _try must always be treated as having side-effects.

-EHa more generally should be understand as partially defining the behavior of faulting operations even if they are not written in a _try: if the operation faults, the behavior is defined so that scopes are unwound and control resumes at an _except clause if one is dynamically active, but this may be observed at an indeterminate point. It is hard to describe these semantics formally, but the intended rules for the implementation are pretty clear: potentially faulty operations outside of a _try can be reordered around each other or even moved to different scopes, as per normal optimization rules, but whenever those operations are executed, if they fault they must trigger an unwind and cause execution to resume at an _except clause if one is active.

So I think you cannot allow operations inlined from a call made within a _try to be reordered with operations written within the _try, or to happen outside the _try. Since LLVM doesn't promise not to reorder volatile and non-volatile stores to provably non-aliased locations, you cannot safely inline non-volatile stores within a _try. You can either disable inlining or mark the calls in some way that will cause the inliner to make any inlined stores volatile (and whatever else is necessary to ensure they will not be reordered).

Oh, yes you are right. I was talking about reordering between inlined faulty instructions is allowed. Reordering inlined instruction with any 'direct' volatile instruction should be prohibited. I overlooked LLVM doesn't promise not to reorder volatile and non-volatile. I will simply disable inlining into a _try scope. thanks!

In D80344#2291156, @rjmccall wrote:

In D80344#2288898, @tentzen wrote:

In D80344#2286838, @rjmccall wrote:

In D80344#2286666, @tentzen wrote:

There is absolutely NO explicit edge added in this design.

I didn't say there was. I said that there was an *implicit* edge: from the memory access to the handler. It's precisely because that edge is implicit that it's problematic for analysis and optimization.

Oh, sorry I misread it.
For HW exceptions the 'implicit' edge is unavoidable unless an explicit approach (like previous iload/istore) is employed. iload approach was extensively discussed and evaluated when it's proposed. As far as I know, the concerns were that it could create many Invokes, complicate flow graph and possibly result in negative performance impact for downstream optimization and code generation. Making all optimizations be aware of the new semantic is also substantial.

Hmm. I suppose it's not that different from the general problem with setjmp/longjmp. I think you'll still have representation problems here, but I'll address them below; I concede that in principle marking regions with an intrinsic that instructions can't necessary be moved over is workable, and so you don't need to turn every faulting instruction into an invoke. You may need to mark the initial seh.try.begin with something like returns_twice or otherwise communicate that there's non-obvious control flow within that function.

OK, good to know this. thank you!
Other than blocking some opts, does this returns_twice attribute have some other implications?

In D80344#2286838, @rjmccall wrote:

In D80344#2286666, @tentzen wrote:

Thank you for prompt reply again.

[rjmccall] And I agree with him that the potential benefits are substantial. I'm just saying that one barely-trafficked RFC thread is not evidence of a community consensus.

OK, thanks. it's good to know you are also supportive in adding this feature.

[rjmccall] As I understand it, you are creating implicit edges within the function to the active SEH handler (potentially part of the same function) from arbitrary places within covered basic blocks, turning basic blocks from a single-entry single-(internal-)exit representation to a single-entry multiple-exit representation. That is a huge change. How do you expect LLVM transformations to preserve or merge this information across blocks? How do you expect transformations to not move code between blocks in problematic ways, or reorder code within blocks in ways that will suddenly be visible to the optimizer? These are the standard objections to supporting features like this, that they have basically unspecified behavior and appear to work by good intentions and happy thoughts.

There is absolutely NO explicit edge added in this design.

I didn't say there was. I said that there was an *implicit* edge: from the memory access to the handler. It's precisely because that edge is implicit that it's problematic for analysis and optimization.

Oh, sorry I misread it.
For HW exceptions the 'implicit' edge is unavoidable unless an explicit approach (like previous iload/istore) is employed. iload approach was extensively discussed and evaluated when it's proposed. As far as I know, the concerns were that it could create many Invokes, complicate flow graph and possibly result in negative performance impact for downstream optimization and code generation. Making all optimizations be aware of the new semantic is also substantial.
So this design applies an IMPLICT approach. I respectfully disagree that it's problematic because as long as volatile attribute is honored it's robust. please see the C & C++ SEH rule stated in patch description section.

Thank you for prompt reply again.

Hi, John, thank you for reviewing this patch and providing feedback.
regarding your comments:

this patch has lasted for a couple of months. a bug in this area is hard and time-consuming to diagnose.
it's better to get this fix in sooner than later. could someone review and approve it?
thanks,

does it look good to you? or any more comment?
thanks

update a couple of changes per David Majnemer's suggestion.

thank you David. will update and submit a new patch shortly.

Is there any feedback? thanks,

Hi, does this look good? or is there any other concern?
thanks,

update LangRef.rst for new intrinsics

fixed tidy warnings

It may be helpful (even for the reviewers) to first specify their behavior,
instead of writing that after-the-fact "backwardly" based on the implementation.

Per Roman Lebedev's feedback, divide the patch into Clang and LLVM.

In D80344#2051127, @lebedev.ri wrote:

This should likely be at least 3 patches: llvm middle-end, llvm codegen, clang.
Langref changes missing for new intrinsics.
Please post all patches with full context (-U99999)

avoid lint warnings and lint hang on Windows

fixed formats

Hi, Is there more concern?
To re-iterate the implementation strategy of this change:

right, I noticed that too and looked into it further..

Per discussion in https://reviews.llvm.org/D79760 , x86 test case there failed.

I meant did you debug into the crash of your test case in https://bugs.llvm.org/show_bug.cgi?id=42266 ?

Hi, Pengfei,
Did you get to the root of the bug and check the correctness of ctor&dtor ordering on your test cases?
Or you just tried to copy MSVC?

Hi, Pengfei,
Is this the same bug as this patch, https://reviews.llvm.org/D79474/new/ , right?

thank you David, good eyes!
Is there any more concern?

fixed a comment.

Hi, Reid,
the existing test case is fixed. I think there is no need to add one more unit test for this patch.
thanks,

fix a typo in test case

Hi, Anton,
I found you are the Code Owner of "Exception handling, Windows CodeGen, ARM EABI".
could you please provide a quick review here? thanks,

any more concern or comment?
thanks,

right, thank you Reid. I see it now :

remade a patch after re-sync

It can be greater than 2 because this Map includes Decls of User locals from parent.
see CodeGenFunction::EmitCapturedLocals() (the same place of this fix).

..
auto I = ParentCGF.LocalDeclMap.find(VD);
if (I == ParentCGF.LocalDeclMap.end())
  continue;

In D77920#1984956, @rnk wrote:

Sounds good, and the fix looks simple, but please add a test.

I'm surprised this change doesn't break an existing test, I'd expect us to have a test for the LSDA of a try-in-catch:

try { }
catch () {
  try { }
  catch () { }
}

As I understand it, the outer TBME must appear before the inner one, and the current behavior puts them in the other way around, innermost first.

Replace F_HasSehAbnormalExits with F_HasExitSwitch

The fix there deals with SEH filter with SEH _finally parent where its prototype is FIXED (2 implicit parameters). It will never change.

In D77982#1981505, @efriedma wrote:

Searching LocalDeclMap is less problematic, I guess... but still, it should be possible to something more straightforward. Maybe make startOutlinedSEHHelper store the actual ImplicitParamDecl, or something like that.

In D77982#1978105, @efriedma wrote:

Again, using the name isn't reliable. Among other things, in release builds, IR values don't have names at all.

Do not use name comparison to locate parent's alloca of frame-pointer-addr.
search parent's LocalDeclMap instead.

Remove hard-code name "frame-pointer".
get the name from 2nd Arg of the _finally().

Per Eli's suggestion,
Use icmp (and zext) to check the JumpDest index, instead of directly passing the Index to _finally().

In D77936#1976984, @efriedma wrote:

I'm not concerned about the performance implications of whatever approach we take here. In the worst case, an "icmp+zext" corresponds to two extra arithmetic instructions; that's not enough to matter. And I expect usually it'll get optimized away.

I'd prefer to avoid exposing our cleanup numbering to user code, if possible. The Microsoft documentation says it returns 0 or 1.

Fixed the format at comment lines

In D77936#1976938, @efriedma wrote:

Instead of asserting there are less than 256 cleanup destinations, can you emit an icmp against zero, or something like that?

Add option -fms-extensions in test case

Per Eli's feedbacks:
(1) a test case (windows-seh-abnormal-exit.c) is added under clang\test\CodeGen directory.
(2) an assert is added to catch the extremely rare case that the number of JumpDests in a function is greater than 255. The max. number of JumpDests is 1 (return) + 2*M + N, where M is the number of for/while-loops and N is the number of Goto targets.