[x86/Linux] Fix unknown type name 'PTR_RUNTIME_FUNCTION'#8272
Conversation
|
@janvorli , @jkotas , @gkhanna79 , PTAL |
| DWORD UnwindData; | ||
| } RUNTIME_FUNCTION, *PRUNTIME_FUNCTION; | ||
|
|
||
| #if defined(_X86_) |
There was a problem hiding this comment.
These should be defined elsewhere to match the other architectures.
The PTR_RUNTIME_FUNCTION should rather be defined in daccess.h, extending the #ifdef for TARGET_X86 && FEATURE_PAL:
https://github.com/dotnet/coreclr/blob/master/src/inc/daccess.h#L2404
The RUNTIME_FUNCTION__BeginAddress should go to clrnt.h close to where it is defined for TARGET_AMD64.
As for the RUNTIME_FUNCTION_INDIRECT, I would put it into palrt.h, next to the definition for TARGET_AMD64
|
If it is ok, I'd like to move |
|
@seanshpark we need to have these in corcompile.h for Windows x86, since neither pal.h nor palrt.h are included for Windows builds. |
|
@janvorli , oh, it's not a good idea to change. OK then, could you review current changes? I hope these are what you meant. |
|
@seanshpark I cannot see any new changes here - maybe you've forgotten to push? |
|
@janvorli , um... isn't current change e943f06 has what you pointed in #8272 (comment) ? |
|
I've said in my next comment: |
Fix compile error for x86/Linux - wrap only RUNTIME_FUNCTION in corcompile.h with !FEATURE_PAL
|
@janvorli , OK, got it. I am sorry for my misunderstanding. I've dropped previous adding defines and changed to your guide. Now the code is better with smaller changes from what it was. If this is also going in a wrong direction, please point out again. |
janvorli
left a comment
janvorli
left a comment
There was a problem hiding this comment.
LGTM, that's what I've meant. Thank you!
|
@dotnet-bot test Windows_NT x86 legacy_backend Checked Build and Test please |
|
Please ignore the x86 compatjit and x86 legacy_backend job failures. They have been removed from future PRs. |
| #ifdef _TARGET_X86_ | ||
| #ifndef FEATURE_PAL | ||
| // | ||
| // x86 ABI does not define RUNTIME_FUNCTION. Define our own to allow unification between x86 and other platforms. |
There was a problem hiding this comment.
Make sure that the definition that we end up picking up from the PAL has the same two fields like this one - it does not appear to be the case.
There was a problem hiding this comment.
Sorry, I have difficulties understanding. Before the change it had only #ifdef _TARGET_X86_ so I rolled back what it was and added #ifndef FEATURE_PAL for not PAL.
There was a problem hiding this comment.
@jkotas I have asked @seanshpark to do it this way to use the three field version on all Unix platforms, assuming we can converge to the same structure. Do you think it is a problem?
There was a problem hiding this comment.
We are not converged to the three field version even on non-x86 platforms today:
The only platform that uses the three field version is amd64 (on both Windows and Unix).
arm and arm64 (on both Windows and Unix) use the two field version. Maybe that actually get compiled with the three field version on Unix, but the third field is just a dead weight. It is not used for anything. We should get rid of it.
There was a problem hiding this comment.
Ok, then we should also modify the definition in PAL to ifdef out the third field for other than AMD64.
|
@dotnet-bot test Windows_NT x86 legacy_backend Checked Build and Test please |
|
Can this be merged? or do I have anything else to do? |
|
@seanshpark sure, I am sorry, I have somehow confused myself into thinking that #8305 contained this chagne. |
|
Thank you :) |
…clr#8272) Fix compile error for x86/Linux - wrap only RUNTIME_FUNCTION in corcompile.h with !FEATURE_PAL Commit migrated from dotnet/coreclr@eedc7cb
6 participants
Fix compile error for x86/Linux