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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

(Please mark this "closed" to reflect the fact that it was merged.)



================
Comment at: clang/lib/Sema/JumpDiagnostics.cpp:935
+    LabelStmt *Label = cast<LabelStmt>(To);
+    Label->setSideEntry(true);
   }
----------------
tentzen wrote:
> rjmccall wrote:
> > This doesn't seem like a reasonable assertion in the abstract.  Even if we really only currently emit warnings with jumps to labels, that doesn't seem like something we should write code that relies on.  And I'm sure this problem can come up with switch cases, unless that's structurally outlawed in some other way.
> > 
> > Also, you're making the correct setting of this flag dependent on whether we're emitting a warning vs. an error.  Seems like we should be setting it regardless.
> > 
> > What conditions exactly do you want this flag set on?  I would naturally assume it's specifically branches from a block outside the `try`, and you don't care about branches within the `try`?  If the label is used in multiple places, do you need to be careful to only set the flag on those branches that come from outside the `try`?
> A _try scope must be a (SEME) Single-Entry-Multi-Exits region.  Branching into a _try is not allowed; it triggers an error. Clang handles it properly.
> What we want to flag here is an branch (either initiated by a go-to/break/continue) from inner _try to outer _try. 
> This flag is set in this If-statement because that is exactly the place issue the warning we want to catch. 
I'm trying to follow this, and not really understanding.  Take the following:

```
void f(int a);
void f(int ex, int lu, int lu2, int lu3) {
  __try {
    {
    int a = 1;
    foo:
      f(a);
    }
    __try {
        f(2);
        goto foo;
    } __except (ex){
    }
  } __except (ex){
  }
}
```

We emit an @llvm.seh.scope.begin() which... doesn't appear to do anything?  I guess maybe that's just a bug, and you only intended to catch jumps into scopes involving a non-trivial destructor?  Okay, let's try that:

```
struct A { ~A() noexcept; };
void f(int a);
void f(int ex, int lu, int lu2, int lu3) {
  __try {
    {
    A a;
    foo:
      f(1);
    }
    __try {
        f(2);
        goto foo;
    } __except (ex){
    }
  } __except (ex){
  }
}
```

Here we get... two @llvm.seh.scope.begin, and no @llvm.seh.scope.end?

Okay, maybe this is actually only meant to work with "try", not "_try".  Let's see what happens:

```
struct A { ~A() noexcept; };
void f(int a);
void f(int ex, int lu, int lu2, int lu3) {
  try {
    f(0);
    {
    A a;
    foo:
      f(1);
    }
        f(2);
        goto foo;
  } catch (...){
  }
}
```

I get two llvm.seh.scope.begin in a row, followed by one llvm.seh.scope.end.  Which still seems wrong.

I'm really not sure what you're hoping to accomplish here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80344