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

Partially allow marshalling ILSTUB compilation for ndirect methods using crossgen2 if SPC.dll is in the same bubble #54847

Closed
wants to merge 1 commit into from
Closed

Partially allow marshalling ILSTUB compilation for ndirect methods using crossgen2 if SPC.dll is in the same bubble #54847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,10 +346,8 @@ public sealed override bool GeneratesPInvoke(MethodDesc method)
{
// PInvokes depend on details of the core library, so for now only compile them if:
// 1) We're compiling the core library module, or
// 2) We're compiling any module, and no marshalling is needed
//
// TODO Future: consider compiling PInvokes with complex marshalling in version bubble
// mode when the core library is included in the bubble.
// 2) We're compiling any module, and no marshalling is needed, or
// 3) We're compiling any module, and core library module is in the same bubble, and marhaller supports compilation

Debug.Assert(method is EcmaMethod);

Expand All @@ -361,6 +359,9 @@ public sealed override bool GeneratesPInvoke(MethodDesc method)
if (((EcmaMethod)method).Module.Equals(method.Context.SystemModule))
return true;

Comment on lines 359 to 361
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (((EcmaMethod)method).Module.Equals(method.Context.SystemModule))
return true;

This condition is redundant

if (_versionBubbleModuleSet.Contains(method.Context.SystemModule))
return !Marshaller.IsMarshallingNotSupported(method);
Copy link
Member

Choose a reason for hiding this comment

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

I believe we have an existing mechanism that rejects unsupported IL stubs already. Is this one redundant?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @gbalykov, Based on Jan's comments the only change required here is to check whether s.p.corelib.dll is part of the bubble. I will include that in #55032, so we could validate with crossgen2 composite tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mangod9 I had some problems when GeneratesPInvoke returned true for methods which used NotSupportedMarshaller. This resulted in them not being compiled into ni.dll at all for some reason. I'll check with #55032

Copy link
Member

Choose a reason for hiding this comment

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

So enabling stub generation for broader scenarios uncovered other issues. So plan is to merge the fix for BStr issue, and remove other Marshallers not implemented for R2R as a separate change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mangod9 Ok, I'll update my PR when other marshallers are removed. This marshallers removal will be included in 6.0, right? Do you have milestone for correct marshallers inplementation for crossgen2? Will it be 6.0 too or 7.0?

Copy link
Member

Choose a reason for hiding this comment

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

yeah hoping removal will be done in the next week. Correct implementation for cg2 most likely will be in 7

Copy link
Member

Choose a reason for hiding this comment

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

@gbalykov the UTF8 and Unicode marshallers have been fixed. We will evaluate how to properly generate stubs for these cases in 7

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll update this PR then


return !Marshaller.IsMarshallingRequired(method);
}

Expand Down
17 changes: 17 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,5 +141,22 @@ public static bool IsMarshallingRequired(MethodSignature methodSig, ParameterMet

return false;
}

public static bool IsMarshallingNotSupported(MethodDesc targetMethod)
{
Debug.Assert(targetMethod.IsPInvoke);

var marshallers = GetMarshallersForMethod(targetMethod);
for (int i = 0; i < marshallers.Length; i++)
{
if (marshallers[i].GetType() == typeof(NotSupportedMarshaller)
// TODO: AnsiStringMarshaller can be allowed when it's logic is fixed,
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to fix this.

// currently it leads to free(): invalid pointer
|| marshallers[i].GetType() == typeof(AnsiStringMarshaller))
return true;
}

return false;
}
}
}