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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
