Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Array ctor integer widening and add Reflection tests #61347

Merged
merged 16 commits into from
Nov 11, 2021
Merged

Fix Array ctor integer widening and add Reflection tests #61347

merged 16 commits into from
Nov 11, 2021

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Nov 9, 2021 •
edited
Loading

The Array ctor path via Reflection had a bug in its integer widening, so converted that to the correct path.
Add tests for Reflection Invoke where the input object[] is changed during invocation.
Remove some coreclr code in the Invoke path.

@AaronRobinsonMSFT
coreclr/
5822ef7 
Remove always false IsStructRequiringStackAllocRetBuf().
Extract the conversion of array constructor arguments into managed.
Expose new FCall for Array ctor call.
@AaronRobinsonMSFT
libraries/
b5bdbfa 
Add special case for array construction during Invoke.
@AaronRobinsonMSFT
mono/
cb13ebc 
Create new icall for Array ctor call.
Create macro define for SPAN_T.
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 7.0.0 milestone Nov 9, 2021
@ghost
Copy link

ghost commented Nov 9, 2021

Tagging subscribers to this area: @buyaa-n
See info in area-owners.md if you want to be subscribed.

Issue Details

This pulls out some logic from coreclr and mono for special casing array construction. The primary improvement is the unboxing of objects to ints is now done in managed code and passed down to the runtime as a Span<int>.

Author: AaronRobinsonMSFT
Assignees: -
Labels:

area-System.Reflection
Milestone: 7.0.0

@AaronRobinsonMSFT
mono/
de61440 
Build issues.
@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review November 9, 2021 06:13
@vargaz
Copy link
Contributor

vargaz commented Nov 9, 2021

The mono changes look ok.

@AaronRobinsonMSFT
Copy link
Member Author

/cc @steveharter @buyaa-n @elinor-fung @jkotas

@AaronRobinsonMSFT
libraries/
2a41c9d 
Add tests for Reflection Binder type conversion support during Invoke.
@AaronRobinsonMSFT
coreclr/
f9ec4f4 
Make sure integral types respect sign extension during widen operation.
Fix InvokeUtil::CreatePrimitiveValue to write destination size.
@AaronRobinsonMSFT
libraries/
f8512a5 
Validate arg count prior to running static constructor.
@AaronRobinsonMSFT
coreclr/
27ae925 
Properly widen the input by respecting endianness.
Revert changes to InvokeUtil::CreatePrimitiveValue.
@AaronRobinsonMSFT
libraries/
220a9b1
jkotas
jkotas approved these changes Nov 10, 2021
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@AaronRobinsonMSFT
coreclr/
e988f81 
Revert some non-impactful changes
@AaronRobinsonMSFT
libraries/
ff14814 
Revert some non-impactful changes
@AaronRobinsonMSFT
mono/
74ec0ce 
Revert some non-impactful changes
@AaronRobinsonMSFT
mono/
c10c7c4 
Remove additional changes.
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title Move some Reflection call of Array ctor into managed Fix Array ctor integer widening and add Reflection tests Nov 10, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT merged commit ff82b6d into dotnet:main Nov 11, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the reflection_array branch November 11, 2021 00:07
This was referenced Nov 29, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas approved these changes

@lambdageek lambdageek Awaiting requested review from lambdageek

@marek-safar marek-safar Awaiting requested review from marek-safar

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

@vargaz vargaz Awaiting requested review from vargaz

Assignees
No one assigned
Labels
area-System.Reflection
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@AaronRobinsonMSFT @vargaz @jkotas