[PATCH] D150340: [SEH]:Fix assertion when try is used inside catch(...) block with /EHa

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added a comment.

In D150340#4343915 <https://reviews.llvm.org/D150340#4343915>, @efriedma wrote:

> LGTM... but I don't think the IR we're generating is really correct overall; see https://github.com/llvm/llvm-project/issues/62723
>
> On a side-note, other open issues related to -EHa/__try:
>
> https://github.com/llvm/llvm-project/issues/62606
> D124642 <https://reviews.llvm.org/D124642>

Thanks Eli for the code review!!  I will take look about missing seh.try.end in c++ try/catch part later.  In term of D124642 <https://reviews.llvm.org/D124642>, I'd really want to look at IR for return inside __try/__finally, but for some reason, I an not build compiler with your patch: no member named 'setHasAddressTaken' in 'llvm::MachineBasicBlock'; did you mean 'hasAddressTaken'.  I may missing some  thing in my environment.



================
Comment at: clang/lib/CodeGen/CGException.cpp:650
+        llvm::FunctionCallee SehCppScope = getSehTryBeginFn(CGM);
+        EmitSehScope(SehCppScope);
+      }
----------------
efriedma wrote:
> jyu2 wrote:
> > efriedma wrote:
> > > Do we need to make the same change in EmitSEHTryStmt/ExitSEHTryStmt?
> > > 
> > > Is there some reason not to just call EmitSehTryScopeBegin here?
> > EmitSehTryScopeBegin:  it is emit seh.scope.begin
> > 
> > In here we want to emit seh.try.begin.  call EmitSehSCope with different function.
> > 
> > For EmitSEHTryStmt/ExitSEHTryStmt if is for __try /__except/__finally and it is for C code.  I thought about that.  And tried some test(BTW, try and __try can not be in same construct), I don't see the problem.  So I did not add that. 
> Not sure I understand what you're saying about EmitSehTryScopeBegin.  CGException.cpp:45 refers to "llvm.seh.try.begin".  CGCleanup.cpp:1370 also refers to "llvm.seh.try.begin"
> 
> For EmitSEHTryStmt/ExitSEHTryStmt, I guess __try codegen can't generate a construct quite like the given testcase, so thta's fine.
:-(  Sorry, You are right!!  It is seems EmitSehTryScopeBegin is not called anywhere.
  
Changed.  Thanks for the review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150340