rjmccall added a comment.
I've made some brief comments about the code, but I think I have much larger
concerns here.
The first is whether LLVM really intends to satisfy the constraints necessary
to make these exceptions work, which I don't think you've gotten clear
consensus about at all. Unfortunately, llvm-dev is not an effective place to
gather this kind of consensus. For a change of this magnitude, I think you
need to be proactively gathering a group of interested parties to sort out
whether this is something we should support, not just expecting people to reply
to an RFC.
The second is that I think the concept of block-level tracking state / control
flow is potentially really problematic for LLVM, and the entire design here
seems contingent on it. Again, this is something we need to ensure consensus
on, not something we can really just sign off on in code review.
The third is that I'm not really convinced that the way we handle cleanups in
Clang today is sufficiently "atomic" in design to accommodate this. I need to
think about it.
================
Comment at: clang/include/clang/Basic/LangOptions.def:131
LANGOPT(CXXExceptions , 1, 0, "C++ exceptions")
+LANGOPT(EHAsynch , 1, 0, "C/C++ EH Asynch exceptions")
LANGOPT(DWARFExceptions , 1, 0, "dwarf exception handling")
----------------
This is usually shortened as "async", but I'm not sure why it's being shortened
at all. Also, hardware faults are synchronous. Also, it applies in all
language modes.
================
Comment at: clang/lib/CodeGen/CGCleanup.cpp:769
+ // Under -EHa, invoke eha_scope_end() to mark scope end before dtor
+ bool IsEHa = getLangOpts().EHAsynch && !Scope.isLifetimeMarker();
+ const EHPersonality &Personality = EHPersonality::get(*this);
----------------
Readers of this code shouldn't have to remember what cl.exe command-line flags
mean. Please call this something like `RequiresSEHScopeEnd`.
================
Comment at: clang/lib/CodeGen/CGCleanup.cpp:783
+ EmitSehTryScopeEnd();
+ }
----------------
Please put braces around the outer `if`, and please be consistent about braces
for the inner clauses.
================
Comment at: clang/lib/CodeGen/CGCleanup.cpp:1305
+static void EmitSehEHaScope(CodeGenFunction &CGF,
+ llvm::FunctionCallee &SehCppScope) {
+ llvm::BasicBlock *InvokeDest = CGF.getInvokeDest();
----------------
I think you should take the callee by value. And please rename this to not use
"EHa".
================
Comment at: clang/lib/CodeGen/CGDecl.cpp:2008
+ EmitSehCppScopeBegin();
+ }
+
----------------
This seems like a pretty random place to do this. Presumably this need to be
tied to basically any scope with EH protection?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80344/new/
https://reviews.llvm.org/D80344
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
