Skip to content

Fixup orphaned references to GetRawSzArrayData - #1458

Merged
jkotas merged 2 commits into
dotnet:masterfrom
GrabYourPitchforks:getrawszarraydata
Jan 11, 2020
Merged

jkotas merged 2 commits into
dotnet:masterfrom
GrabYourPitchforks:getrawszarraydata

Conversation

@GrabYourPitchforks

Copy link
Copy Markdown
Member

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

/cc @dotnet/crossgen-contrib

@MichalStrehovsky MichalStrehovsky left a comment

Copy link
Copy Markdown
Member

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!

return context.SystemModule.GetKnownType("System.Runtime.InteropServices", "Marshal");
}

public static MetadataType GetMemoryMarshal(TypeSystemContext context)

Copy link
Copy Markdown
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?

ILEmitter emitter = _ilCodeStreams.Emitter;
ILCodeLabel lNullArray = emitter.NewCodeLabel();

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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