Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix RVA field placement order#24047

Merged
fadimounir merged 4 commits into
dotnet:masterfrom
fadimounir:Fix23872_rva
Apr 18, 2019
Merged

Fix RVA field placement order#24047
fadimounir merged 4 commits into
dotnet:masterfrom
fadimounir:Fix23872_rva

Conversation

@fadimounir
Copy link
Copy Markdown

Fix for #23872

RVA fields should be placed in the R2R image after a correct qsort operation, and not as part of ZapNode relocs placement during compilation. This fixes the Managed C++ binaries scenario in R2R.

Updating the qsort callback to sort by descending size for fields of equal RVA: we do this for fragile NIs

@fadimounir fadimounir requested a review from jkotas April 17, 2019 00:18
Comment thread src/zap/zapmetadata.cpp
Comment thread src/zap/zapmetadata.cpp Outdated
@MichalStrehovsky
Copy link
Copy Markdown
Member

Can we add a test for this so that there's something to test this with for CPAOT? (Plus, regression tests never hurt.)

@fadimounir
Copy link
Copy Markdown
Author

@MichalStrehovsky Test added

Comment thread tests/src/JIT/Directed/rvastatics/RVAOrderingTest.il
@CarolEidt
Copy link
Copy Markdown

@fadimounir - this test is also failing in CheckProjects here: https://ci.dot.net/job/dotnet_coreclr/job/master/job/arm64_cross_debug_ubuntu16.04_tst/172/

@fadimounir
Copy link
Copy Markdown
Author

@CarolEidt I just merged a fix #24119

@CarolEidt
Copy link
Copy Markdown

Thanks @fadimounir !

@fadimounir fadimounir deleted the Fix23872_rva branch May 9, 2019 17:51
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Static RVA fields should be placed sequentially after the qsort operation to not break Managed C++ binaries in R2R

* Add regression test


Commit migrated from dotnet/coreclr@1a2bc6c
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants