In D102817#3921362, @pengfei wrote:

One question: Is the comment still true with this patch? https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86AsmPrinter.cpp#L807C55-L808

update per Pengfei's review feedbacks.

update per review feedbacks

In D102817#3910700, @pengfei wrote:

Reverse ping @tentzen. Is there any block issues for it moving on? I don't see serious from a quick review, just some nits.

Per Eli Friedman's feedback, replace MBB->hasAddressTaken() with new MBB->isMachineBlockAddressTaken()

What is the current status of this fix?

This isn't really the distinction I'm trying making here. The distinction here is between LLVM IR uses of blockaddress (for indirectbr), and MachineFunction uses of MachineBasicBlock labels (for retpoline etc.). Jump tables would fall into the latter category, except that we have explicit code to update jump tables in the places where it would matter.

How about AddressTaken_Internal and AddressTaken_External?

the _Internal is used for blockAddress-constant, jump table, etc where it's targeted by IR internal user code domain.
the latter indicates that the address is taken and can be targeted externally by Runtime (EH case), OS or even HW (like the retpoline case).

Hey Eli, I agree that address-taken is not perfect, but I’m not sure I have any better ideas right now. Maybe @rnk and @JosephTremoulet have thoughts?’

In D102817#3350514, @gmh5225 wrote:

Code below will cause a crash.

 void
 parse()
{
    throw std::exception("throw--------");
    return;
}

 int
 main()
{
    try
    {
        parse();
    }
    catch (const std::exception &e)
    {
        printf("except=%s\n", e.what());
        // Crash when return in catch scope
        return -1;
    }

    system("pause");
    printf("end\n");
    return 0;
}

Seems that function 'EmitSehCppScopeEnd' in PopCleanupBlock can not find the insert point.
CGF.Builder.GetInsertBlock() or CGF.getInvokeDest() equals nullptr.

In D80344#3453565, @bowang wrote:

Hi, this patch is just part-1 of Windows SEH feature support. Part-2 work had been sitting in https://reviews.llvm.org/D102817/new/ since last May 2021. It's been reviewed, and feedbacks had been addressed. but nobody has approved it. To handle Hardware Exception in Clang & LLVM, we both patches in. thanks.

Thanks for the feedback. This feature is really useful.

We are attempting to switch from MSVC to Clang company-wise. We resolved most issues but lastly got stuck by Clang not supporting SEH. I believe we are not an unique case running into this issue. I'd be very happy to see this feature get merged in.

In D80344#3450940, @bowang wrote:

Any updates on this patch? Would like to see it moving forward.

I was trying to use Google Breakpad with Clang. Breakpad is a crash handling library that generates a minidump at crash. The exception handler on Windows relies on SEH.

Breakpad works fine with MSVC. But when it is built with Clang, the exception code returned in exinfo>ExceptionRecord->ExceptionCode is incorrect. In my test program, an access to nullptr should trigger EXCEPTION_ACCESS_VIOLATION but what's returned is EXCEPTION_BREAKPOINT. I suspect it is due to Hardware Exception Handling in Clang.

I tried to add flag -fasync-exceptions but the problem stays the same.

I tried to add flag -fseh-exceptions but received an error error: invalid exception model 'seh' for target 'x86_64-pc-windows-msvc19.0.24245'

I'm not fully sure whether this patch can address my issue, but would appreciate if anyone could shed some light on this. Thanks in advance.

Hi Joseph, all good points. see replies below.
thank you!

Joseph,
I greatly appreciate your thorough review and insightful feedbacks.
new revision is provided. also review it again.
thank you!

Update per Joseph Tremoulet's feedback.

Thank Joseph's in-depth code review and many great suggestions. will work on it and reply back later..

The previous did get lots of substantial and quality reviews (especially those from rjmccall).
Roman, you asked about LLVM design & implementation when reviewing part-1. Here you go; Do you have any feedback?
thanks,

Thank David.
Does anyone have any more feedbacks?
We would like to land this patch in shortly.
thanks,

since I cannot repro it locally, let's have this patch in to resolve -EHa -O2 crashes for now.
I will add more -O2 tests in following patches.

@zahiraam, are you removing all those CHECKs:

In D103664#2798732, @aganea wrote:

Thanks for the quick fix! Would you mind fixing the two failing tests please? (see above)

@aganea Oh, my mistake. did not mean to enable -fasync-exceptions under -EHa in this patch. will fix it shortly...

Hi, yes I can repro the crash with -fasync-exceptions plus -O2 option. Working on it now..
thank you for reporting this bug..
@aganea , this patch should be zero-impact without explicit option -fasync-exceptions. Are you also seeing a crash without this option?
thanks

Hi, Alexandre
Thank you for reporting this . From the callstack it does not seem related to this patch.
Could you share the repro case @__compile.rsp temp.i with me?
Thanks,

fixed clang-tidy/-format.
added RUN command in one test.

Passed many CodeGen related test suites over the weekend.
It landed in..

Hi, (Last call for review!!)
Is there any more comments? This review has lasted for more than a year now. I believe I had addressed or answered all questions and concerns. Thank all reviewers' precious feedbacks.
For late comers, again
(1) Original llvm-dev [RFC] discussions can be found in these two threads below:
https://lists.llvm.org/pipermail/llvm-dev/2020-March/140541.html
https://lists.llvm.org/pipermail/llvm-dev/2020-April/141338.html

fixed command option typos in two test cases

clang-format fixes.

hi, I believe I'd addressed all issues or concerns, and it's rebased to up-to-date source in new _main branch now. Does this look good to everyone? If I don't hear any objection in a couple of days, I will go ahead make this patch in. Again This is just part-1 of Windows SEH feature. Without new option specified, it's a zero-impact change.

rebase the patch

Removed some files (mostly for Part 2 of this feature) that were accidentally put in last revision.

changed option name and a couple of minor changes per John McCall's comments.

thank you for the thorough review again. My answer for each comment below:

Hi, John,
sorry for the delay. I'm still in the middle of something.
will context-switch a little bit latter. thanks,

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..