This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Cross-bitness in ZapRelocs #18665
Merged
Cross-bitness in ZapRelocs #18665
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…loc in src/zap/zaprelocs.cpp
…_PTR in src/zap/zapinfo.cpp
…_PTR in src/zap/zaprelocs.cpp
jkotas
reviewed
Jun 27, 2018
src/zap/zapimport.cpp
Outdated
| { | ||
| if (IsReadyToRunCompilation()) | ||
| { | ||
| SIZE_T value = 0; |
Member
There was a problem hiding this comment.
This should use type that is of the right target pointer size, not SIZE_T.
jkotas
reviewed
Jun 27, 2018
src/zap/zapinfo.cpp
Outdated
|
|
||
| case IMAGE_REL_BASED_PTR: | ||
| #ifdef _TARGET_64BIT_ | ||
| *(UNALIGNED TADDR *)location = (TADDR)targetOffset; |
Member
There was a problem hiding this comment.
This should use int64 instead TADDR now.
jkotas
reviewed
Jun 27, 2018
src/zap/zapimport.cpp
Outdated
| if (IsReadyToRunCompilation()) | ||
| { | ||
| SIZE_T value = 0; | ||
| target_size_t value = 0; |
Member
There was a problem hiding this comment.
It looks odd to use target_size_t for all these since none of them are actually sizes.
Would TARGET_POINTER or TARGET_POINTER_TYPE be a better name?
Author
There was a problem hiding this comment.
The only reason I had in mind when picked target_size_t is to be consistent with clrjit where we already have target_size_t typedef. I renamed this to TARGET_POINTER_TYPE in src/zap
jkotas
approved these changes
Jun 27, 2018
echesakov
changed the title
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
* Cast to UINT32 to avoid warnings on Windows in ZapBaseRelocs::WriteReloc in src/zap/zaprelocs.cpp * Replace TADDR with DWORD in ZapInfo::recordRelocation IMAGE_REL_BASED_PTR in src/zap/zapinfo.cpp * Replace sizeof(cell) with TARGET_POINTER_SIZE in src/zap/zapimport.cpp * Replace TADDR with DWORD in ZapBaseRelocs::WriteReloc IMAGE_REL_BASED_PTR in src/zap/zaprelocs.cpp * Define target_size_t type * Replace TADDR with target_size_t in ZapInfo::recordRelocation in src/zap/zapinfo.cpp * Replace SIZE_T PVOID with target_size_t in src/zap/zapimport.cpp * Replace TADDR with target_size_t in src/zap/zaprelocs.cpp * Rename target_size_t to TARGET_POINTER_TYPE Commit migrated from dotnet/coreclr@1a24a27
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TADDRis defined astypedef ULONG_PTR TADDRin VM and it seems to me that it's almost impossible to use target-specific typedef forTADDR. In this PR I identified small number of places in src/zap whereTADDRis used for writing relocation records to PE image and replaced those withDWORDkeeping the rest of CoreCLR untouched. As usually, I validated these changes on CoreLib and CoreFX assemblies (incl. test assemblies) by checking that output of x86_arm and x64_arm crossgens are identical.@jkotas Do you think this is a right approach?