Skip to content

Fixup orphaned references to GetRawSzArrayData #1458

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.

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

Merged
merged 2 commits into from
Jan 11, 2020

Conversation

GrabYourPitchforks
Copy link
Member

Per @MichalStrehovsky's comment at #1036 (comment), we had some orphaned references that I missed in the original PR.

/cc @dotnet/crossgen-contrib

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

LGTM modulo the orphaned method. Thanks!

@@ -45,6 +45,11 @@ public static MetadataType GetMarshal(TypeSystemContext context)
return context.SystemModule.GetKnownType("System.Runtime.InteropServices", "Marshal");
}

public static MetadataType GetMemoryMarshal(TypeSystemContext context)
Copy link
Member

Choose a reason for hiding this comment

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

Could you delete GetRuntimeHelpers below?

@@ -1322,7 +1322,7 @@ protected override void AllocAndTransformManagedToNative(ILCodeStream codeStream
ILEmitter emitter = _ilCodeStreams.Emitter;
ILCodeLabel lNullArray = emitter.NewCodeLabel();

MethodDesc getRawSzArrayDataMethod = InteropTypes.GetRuntimeHelpers(Context).GetKnownMethod("GetRawSzArrayData", null);
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we have a test hole?

Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we have a test hole?

AFAIK proper crossgen2 testing is only getting stood up now - but I haven't been keeping track while on leave. I would expect this to be hit in one of the p/invoke interop tests with crossgen2 when large version bubble is enabled.

Cc @dotnet/crossgen-contrib

Copy link
Member

Choose a reason for hiding this comment

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

We currently have two Crossgen2 pipelines - Pri0 and Pri1. They however haven't yet been enabled to run automatically because they are not yet 100% clean (the last time I looked I saw about a dozen failures in the Pri1 tests). I believe that we should be able to weed out the last remaining issues and add at least a limited amount of Crossgen2 testing to the pre-checkin and outerloop pipeline in 1-2 weeks.

Copy link
Member

Choose a reason for hiding this comment

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

We only have one remaining failure in the Pri1 tests compiled with crossgen2 with GC stress stress enabled and one without (on Windows, I am in progress of fixing GC stress issues on Linux). The one failing without GC stress doesn't seem related, the one with GC stress might be related, as it fails due to an incorrect GC reporting of an array element as byref via pinvoke. But it works fine without GC stress, so maybe is it not related after all.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, syncing to the latest master, I now get failure in crossgen2 due to this.

@GrabYourPitchforks
Copy link
Member Author

@jkotas Beat me to making the change by mere seconds. :)

@jkotas jkotas merged commit 72081db into dotnet:master Jan 11, 2020
@GrabYourPitchforks GrabYourPitchforks deleted the getrawszarraydata branch April 15, 2020 02:28
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
MichalStrehovsky pushed a commit to MichalStrehovsky/runtime that referenced this pull request Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants