Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Fixed recordRelocation for 32-bit platforms#3433

Merged
jkotas merged 4 commits intodotnet:masterfrom
sergign60:arm_build
Apr 27, 2017
Merged

Fixed recordRelocation for 32-bit platforms#3433
jkotas merged 4 commits intodotnet:masterfrom
sergign60:arm_build

Conversation

@sergign60
Copy link
Contributor

@sergign60 sergign60 commented Apr 26, 2017

No description provided.

@sergign60
Copy link
Contributor Author

sergign60 commented Apr 26, 2017

CC: @Dmitri-Botcharnikov

@jkotas
Copy link
Member

jkotas commented Apr 26, 2017

This will produce completely broken binaries. The relocations has to be recorded and applied when saving into the image even on 32-bit platforms.

@sergign60
Copy link
Contributor Author

sergign60 commented Apr 26, 2017 •
edited
Loading

@jkotas
I just looked at corecrl/src/vm/jitinterface.cpp

void CEEJitInfo::recordRelocation(void * location,
                                  void * target,
                                  WORD   fRelocType,
                                  WORD   slot,
                                  INT32  addlDelta)
{
...
#else // _WIN64
    JIT_TO_EE_TRANSITION_LEAF();

    // Nothing to do on 32-bit

    EE_TO_JIT_TRANSITION_LEAF();
#endif // _WIN64
}

@jkotas
Copy link
Member

jkotas commented Apr 26, 2017

The implementation in jitinterface.cpp is for the JIT case. It needs to be like crossgen https://github.com/dotnet/coreclr/blob/master/src/zap/zapinfo.cpp#L2468 for the AOT compilation case.

@sergign60
Copy link
Contributor Author

sergign60 commented Apr 26, 2017

@jkotas
I got it, thanks!

jkotas
jkotas reviewed Apr 26, 2017
View reviewed changes
src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/Relocation.cs


IMAGE_REL_BASED_THUMB_BRANCH24 = 0x13, // Thumb2: based B, BL
IMAGE_REL_THUMB_BRANCH24 = 0x14, // Thumb2: B, BL
Copy link
Member

@jkotas jkotas Apr 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoyMS Why are we mixing IMAGE_REL_BASED_XXX and IMAGE_REL_THUMB_XXX here (it started with change 1628798 on 9/22/2016)? They are two different enums. It will results into having two different values for the same thing; and it will likely result into collisions pretty soon as well.

@sergign60 Could you please keep IMAGE_REL_THUMB_XXX together, and add TODO with a link to a issue to get it cleaned up? I expect that it will need to get cleaned up to make progress on the ARM support.

@jkotas
Copy link
Member

jkotas commented Apr 27, 2017

Thanks!

@jkotas jkotas merged commit b7bda6e into dotnet:master Apr 27, 2017
@sergign60 sergign60 deleted the arm_build branch April 27, 2017 12:19
@MichalStrehovsky MichalStrehovsky mentioned this pull request Aug 11, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jkotas jkotas jkotas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-already-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sergign60 @jkotas @dnfclas