Unify SEH wrapping for method calls in the interpexec.cpp #121311
Conversation
This change adds a templated way to wrap any method call in a SEH exception try / catch. Until now, we were adding a new function for each case that needed this wrapping.
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors exception handling for method calls in the interpreter by introducing a generic CallWithSEHWrapper template function to replace specialized wrapper functions (CallPreStub and CallGetMethodDescOfVirtualizedCode). This consolidates duplicate SEH exception handling logic into a single reusable template.
- Replaced two specialized SEH wrapper functions with a single generic template
- Updated all call sites to use the new generic wrapper
- Consolidated duplicate exception handling logic
src/coreclr/vm/interpexec.cpp
Outdated
| } | ||
|
|
||
| template<typename Obj, typename Method, typename... Args> | ||
| auto CallWithSEHWrapper(Obj* obj, Method method, Args... args) -> decltype((obj->*method)(args...)) |
There was a problem hiding this comment.
Could we instead use a no-parameter lambda here (and capture the args into the state object)? Would avoid pulling in tuple and that machinery.
There was a problem hiding this comment.
I wonder why would you prefer to avoid using the tuple?
Anyways, I've changed the implementation to use the lambda as you've suggested. The benefit is that we can easily use it for invoking non-member functions without any changes, which we may need to do.
There was a problem hiding this comment.
One of the reasons I prefer not using the tuple is it violates our C++ guidelines for the repo.
I also think this implementation looks much cleaner and easier to understand at a glance.
There was a problem hiding this comment.
I don't see anything about tuple violating our C++ guidelines. I assume you may be referring to https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/clr-code-guide.md#-2114-limit-usage-of-standard-template-types-in-shipping-executables. My understanding was that this section was more about stuff that would have parts of the code in the lib. The tuple, at least in this usage case, doesn't require any such code.
We had bad experience with performance of lambdas in the past (@davidwrighton tried to change some huge macros in GC to lambdas), so I was worried that the compiler would not be able to eliminate the calls in this case. Fortunately, it is not that case here.
There was a problem hiding this comment.
The concern @jkotas has (and the reason for the rules) is about any code that may exist in the headers or any statically-linked portions. Anything in the standard library that is loaded at runtime from the target machine's standard library implementation is fine.
There was a problem hiding this comment.
Right, that's my understanding as well. It would not have been the case here.
There was a problem hiding this comment.
tuple has code that runs that's defined in the headers, so it meets the definition AFAIK. In practice we'd never have an issue, but my understanding is that we'd rather be more strict here than trying to be more granular header-by-header.
|
Hmm, the new version breaks the interpreter test, I am investigating ... |
|
Ah, copy and paste error for the GetMethodDescOfVirtualizedCode |
Fix copy and paste error
Removed the CallGetMethodDescOfVirtualizedCode function, which is now a dead code.
Fix arguments
3 participants
This change adds a way to wrap any method call in a SEH exception try / catch. Until now, we were adding a new function for each case that needed this wrapping.
I've verified that the generated code in the release build is the same as before this change.