[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

Wed Sep 23 21:20:47 PDT 2020

rjmccall added a comment.

In D80344#2291456 <https://reviews.llvm.org/D80344#2291456>, @tentzen wrote:

> In D80344#2291156 <https://reviews.llvm.org/D80344#2291156>, @rjmccall wrote:
>
>> In D80344#2288898 <https://reviews.llvm.org/D80344#2288898>, @tentzen wrote:
>>
>>> In D80344#2286838 <https://reviews.llvm.org/D80344#2286838>, @rjmccall wrote:
>>>
>>>> In D80344#2286666 <https://reviews.llvm.org/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?
>
>>> 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.
>>
>> You also need to make sure that potentially-faulting instructions aren't moved *into* the `_try` region.  I don't think that just making accesses with the `_try` `volatile` achieves that.  Probably the easiest way to do this would be to outline the `_try` region through most of the LLVM pipeline and only inline it very late.
>
> The intrinsic is set with unknown memory access.  Plus `returns_twice` you just suggested, is not it sufficient to block potentially-faulting instructions from being moved across?

I've changed my mind; you've mostly got edges in the place you need, and you don't need `returns_twice`.  But see the comment below about unreachable ends of scopes; I don't know how to handle that well.

>>>>> For LLVM transformations/optimizations, the only constrain added is **volatile** attribute on memory operations within a SEH region.
>>>>
>>>> When is this attribute applied?  Because you can't just apply it at a single point; you also need to mark operations when they're copied/inlined into the region.  We once had a similar attempt at handling exceptions with a setjmp-based ABI (not the "SJLJ" exceptions supported by the backends, something frontend-centric) that had persistent problems because of poor modeling in the IR, which it feels like this is doomed to repeat.
>>>
>>> This is applied only once in FE. Consider it's like from the source code. So NO there is no other place we need to change. Per SEH semantic, the inline code is immune from volatile constraint.
>>
>> The SEH semantics say that faulting instructions not lexically within the `_try` can be re-ordered with each other, but it's not clear that it's okay to re-order them with instructions within the `_try`.  If I had to guess, I would say that the expectation is that inlining is disabled within a `_try` scope.
>
> 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).

> Volatile-ness here is primary for reordering constraint, not for data-flow. Per LLVM EH framework, a data-defined in _try will flow through explicit EH edge into the Handler.  In this SEH design, there will be a seh.try.end() at every exit of SEH region that ensures data defined in _try scope flow to the use in Handler.

Where is the "explicit EH edge into the handler" from a faulting instruction?  It seems like there's an EH edge to the innermost cleanup from the last cleanup scope you entered, but that's not the same as from a faulting instruction, and the next site that does have a potential EH edge is the end of the scope, which you have no guarantee is actually reachable via the ordinary CFG.  So I think you actually *are* relying on `volatile` for its data-flow guarantees.

>> This is why frontends cannot naively lower things like EH to `setjmp` without any influence on the inliner.
>
> why do you want to do that?  LLVM EH offers a robust data-flow representation while SJLJ data-flow modeling is incomplete.

We don't, it's a bad representation for EH.  But the problems seem very analogous to what you're dealing with here.

>>>>>> [rjmccall] Does it pass under all combinations of optimization and code-generation settings? How do LLVM contributors ensure that this test suite continues to pass?
>>>>>
>>>>> Yes. this is just the first patch.  there will be second patch to land LLVM code-gen part in, then 3rd patch to add some test cases (Ex, xcpt4u.c from MSVC). Without option -EHa, the patch is a zero-impact change for now.
>>>>
>>>> Do you expect to maintain a CI bot that runs the full test suite continuously as changes go into LLVM?  Because this sort of thing is extremely easy to break.
>>>
>>> Yes, as long as it's a bug/flaw in the design/implementation.  New opt/analysis/tools that violates original basic volatile/cleanup/EH-framework exposed by SEH test cases is not included.  Isn't this the policy of Clang/LLVM community?
>>
>> I think you're missing what I'm asking.  If LLVM accepts this feature, it will become our collective responsibility as a project to keep it working.  You have a large external correctness test suite for this feature.  It does not sound like you intend to open-source that test suite; instead, you intend to extract a small number of unit tests from it and add those to the LLVM test suite.  So I'm asking if you're at least going to have an external CI bot which will run the full test suite for this feature to ensure it hasn't been broken by the last day of commits.  It does not seem reasonable to expect that the few unit tests you extract will themselves be sufficient to keep the feature tested.
>
> As Aaron replied, yes MS SEH (and EH) test suite are (or will be) open sourced.

Okay, great.  We just need a firm commitment that those tests will be run continually on the main branches and during release qualification.  If they're open-source, you may be able to add them to the extended LLVM test suite (not just the checked-in regression tests), which I think those things already happen for.  Other people should be able to help you with the right process for that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80344