Use EXCEPTION_HANDLER_IMPL/DECL macros for declaring exception handlers #113826
Conversation
ghost
added
the
dotnet-policy-service
bot
added
the
community-contribution
| } | ||
|
|
||
| EXTERN_C EXCEPTION_DISPOSITION | ||
| UMThunkStubUnwindFrameChainHandler( |
There was a problem hiding this comment.
This handler is no longer used.
|
This is extracted from the x86 funclet prototype (#113576). It's one of the changes that makes the diff quite unreadable and distracts from the imporant parts. |
filipnavara
force-pushed
the
exception_decl_impl
branch
from
c042745 to
fd2ea20
Compare
There was a problem hiding this comment.
Overall, I would prefer just making the exception handlers have the __cdecl on x86 and not make any other changes. The functions definitions by the macro without the arguments are more difficult to follow. I don't also see a problem with the pEstablisherFrame being PVOID, as it is just a pointer to a stack location. The fact that it is some struct pointer on x86 doesn't seem to be a good reason for changing to that type everywhere. You can easily cast it to the struct pointer for x86 if you need it.
| { | ||
| OBJECTREF oref = ExceptionTracker::CreateThrowable(pExceptionRecord, FALSE); | ||
| DispatchManagedException(oref, pContextRecord, pExceptionRecord); | ||
| DispatchManagedException(oref, pContext, pExceptionRecord); |
There was a problem hiding this comment.
A nit - can we please keep the name pContextRecord at places where you have renamed it? It would make this change much smaller and it would match the Windows naming (e.g. in RtlVirtualUnwind and other similar functions). It is also usually used in locations where the exception record so that the two names match.
There was a problem hiding this comment.
I renamed it to follow the existing macro. If I were to change the macro then an equal number of places elsewhere would have the be modified.
There was a problem hiding this comment.
I would prefer taking the non-x86 stuff as the standard. The x86 has always been a bit different at various places for historical reasons.
| // Perform unwinding of the current frame | ||
| disposition = ProcessCLRException(exceptionRecord, | ||
| (void*)establisherFrame, | ||
| (struct _EXCEPTION_REGISTRATION_RECORD *)establisherFrame, |
There was a problem hiding this comment.
This looks strange, the establisherFrame isn't a struct like this. Is is purely a stack pointer.
I don't have strong preference. The macros were already there so I tried to reuse the existing infrastructure instead of inventing something else.
It's |
I'll try to rework my other PR to use that and then likely close this and submit the other variant. |
The current declaration of the exception handlers are incorrect on x86. They are missing
__cdeclcalling convention and the second parameter is declared asPVOIDinstead ofstruct _EXCEPTION_REGISTRATION_RECORD *.We have two options - either we fix every single of these declarations to match the required prototype or we use the existing macros. The macros have the flexibility of chosing between
struct _EXCEPTION_REGISTRATION_RECORD *andPVOIDfor the second parameter if we ever need that distinction.I assume this won't break IntelliSense completions due to the use of
src/coreclr/cpp.hint.