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

Thu Sep 24 00:40:15 PDT 2020

tentzen added a comment.

In D80344#2291688 <https://reviews.llvm.org/D80344#2291688>, @rjmccall wrote:

> In D80344#2291456 <https://reviews.llvm.org/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!

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

Oh, right, I take it back.  volatile-ness is used to assure 2nd (reordering) and 3rd (global-state) rules for C-code as described in head summary of this patch.  I was thinking about abnormal exits (like https://reviews.llvm.org/D77936#change-6bDmAmTUlS0o) in _try-finally case where dataflow is guaranteed to reach _finally.

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

Yes, that is the plan after -EHa Part-II is done.  but again, it won't be the entire suite without the support of Local-Unwind.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80344