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

[Ten Tzen via Phabricator via cfe-commits]

tentzen added a comment.

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?

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

>> I don't think this is the same as SJLJ story.  I did once look into SJLJ problem when I found every single test in MSVC Setjmp suite was broken.  it's because the implicit back edge from longjmp (or a callee with longjump) was not modeled properly.  actually I have a simple idea that can fix this SJLJ problem robustly, but I've been clogged in this SEH task.
>
> The issue I'm describing was somewhat different from the general problems of using `setjmp`.  The C `setjmp`/`longjmp` library functions don't guarantee that stores to non-`volatile` objects will be visible after the `longjmp`; that is, they require user cooperation in order to get consistency.  This creates a problem when trying to use `setjmp`/`longjmp` directly in IR to implement language exceptions: while you can relatively easily mark all the accesses within a lexical `try` as volatile in the frontend, non-`volatile` accesses can be introduced into the function (e.g. by inlining) that won't be `volatile`, and then they potentially get lost if an exception is thrown.  A simple example of this is something like:
>
>   void assign_and_throw(int *ip) {
>     *ip = 5;
>     throw some_exception();
>   }
>   
>   void test() {
>     int i;
>     try {
>       i = 0;
>       assign_and_throw(&i);
>     } catch (...) {
>       printf("%d\n", i);
>     }
>   }
>
> The frontend marks the assignment to `i` in `test` as volatile, so the compiler must emit it as a real store, and so it will be observed in the catch handler after the call to `longjmp` done within the `throw` operation.  And if no inlining is done, the compiler has no choice but to emit the assignment to `*ip` in `assign_and_throw` as a real store as well.  But if the compiler inlines the call to `assign_and_throw`, that store to `*ip` becomes a non-volatile store directly to `i`, and since there's no direct control-flow path from there to a location that can read it, that store appears to be dead and can be discarded.  That is allowed under the rules for `setjmp` — if the user wants to ensure that this store is visible, they would have to declare `i` as a `volatile int` and `ip` as a `volatile int *` — but of course not under the rules for `throw`/`catch`.

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

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

>>>>> [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.
BTW I have to point out that to pass the entire suite we need 2nd missing feature (Local-Unwind) in place too.  As a heads up, a proposed Local-Unwind design and implementation can be seen here (https://github.com/tentzen/llvm-project ).  We will bring it back to RFC once -EHa is accepted and landed.

>> For C++ case under -EHa, seh.try.begin(), an EhaScopeBegin() is injected at the beginning of the scope if Dtor() cleanup is required.
>
> Okay, I think I understand how this is supposed to work, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80344