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

switch to using CSTRMarshaller instead of AnsiBSTR #55032

Merged
merged 9 commits into from
Jul 13, 2021
Merged

switch to using CSTRMarshaller instead of AnsiBSTR #55032

merged 9 commits into from
Jul 13, 2021

Conversation

mangod9
Copy link
Member

@mangod9 mangod9 commented Jul 1, 2021

Fixes #54820.

cc @dotnet/crossgen-contrib @MichalStrehovsky

mangod9
mangod9 commented Jul 1, 2021
View reviewed changes
src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.cs

bool bPassByValueInOnly = In && !Out && !IsManagedByRef;
ILLocalVariable localBuffer = emitter.NewLocal(Context.GetWellKnownType(WellKnownType.IntPtr));
if (bPassByValueInOnly)
Copy link
Member Author

Choose a reason for hiding this comment

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

is there a reason why this logic cant move to System.StubHelpers.CSTRMarshaler ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it would be nice to have majority of the logic in C#, and minimize amount of the hand-emitted IL.

@mangod9
Copy link
Member Author

mangod9 commented Jul 2, 2021 •
edited
Loading

@AaronRobinsonMSFT as well.. perhaps we should just fix the out string case for now and refactor these helpers in 7.

@AaronRobinsonMSFT
Copy link
Member

/cc @jkoritzinsky @elinor-fung

@mangod9
Add CleanupManaged back
39f17b8 
Updated the cleanup logic to the one implemented in ILOptimizedAllocMarshaler::EmitClearNative
@mangod9
Copy link
Member Author

mangod9 commented Jul 2, 2021

crossgen2 outerloop failures are comparable to main runs.

@mangod9
Copy link
Member Author

mangod9 commented Jul 6, 2021

@VSadov since we saw this show up as part of this issue: #54759. Any comments / concerns with this change?

Wonder how we hadn't seen any previous issues here since libraries have been compiled using cg2 for a few previews now and a simple interop call with out string should hit this case.

@VSadov
Copy link
Member

VSadov commented Jul 6, 2021

@mangod9 - the fix look reasonable and the issue is very likely the same as one observed in #54759.

As for why we have not seen this sooner - maybe seeing a combination of "out" parameters in interop scenario, on Linux, with crossgen2 just took time to observe...

@mangod9 mangod9 changed the title [WIP] switch to using CSTRMarshaller instead of AnsiBSTR switch to using CSTRMarshaller instead of AnsiBSTR Jul 6, 2021
@MichalStrehovsky
Copy link
Member

Do we need a regression test?

@mangod9
Copy link
Member Author

mangod9 commented Jul 7, 2021

yeah I am hoping there are some interop tests already covering out string scenario which we could add to the r2r pipeline. @AaronRobinsonMSFT @jkoritzinsky @elinor-fung do you happen to know if any exist?

@jkotas
Copy link
Member

jkotas commented Jul 7, 2021

We have a lot of tests for this. The problem is that the interop stub generation in crossgen is currently only enabled for CoreLib, so it is impossible to test.

We would need to enable it for everything in CoreLib version bubble to make it testable by changing this to if (_versionBubbleModuleSet.Contains(method.Context.SystemModule)).

@mangod9
Copy link
Member Author

mangod9 commented Jul 7, 2021

which is being done in this PR: #54847? So a combination of the two PRs (without skipping AnsiStringMarshaller) would suffice?

@jkotas
Copy link
Member

jkotas commented Jul 7, 2021

Yes, assuming we have appropriate large version bubble test leg.

@mangod9 mangod9 mentioned this pull request Jul 8, 2021
Closed
@mangod9
Copy link
Member Author

mangod9 commented Jul 8, 2021

with the latest change I am hitting an issue similar to this: #51189. so more of these marshallers would need updates to work with R2R

@jkotas
Copy link
Member

jkotas commented Jul 8, 2021

I would recommend undoing the last change, and deal with the testing later.

We need a unified intentionally designed set of helpers for marshalling strings that are shared between runtime-generated, crossgen-generated and source-generated interop.

@mangod9
Copy link
Member Author

mangod9 commented Jul 8, 2021

I would recommend undoing the last change, and deal with the testing later.

yeah that seems like the best approach to get this merged. Will revert that change.

@mangod9
Copy link
Member Author

mangod9 commented Jul 9, 2021

ok, CI is good now. crossgen2-composite failures are at par with main.

@MichalStrehovsky
Copy link
Member

Btw, disabling the Utf8Marshaller would be as simple as deleting these two lines:

runtime/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs

Lines 28 to 29 in d431d6a

case MarshallerKind.UTF8String:
return new UTF8StringMarshaller();
. I don't think Utf8Marshaller as it is right now works with crossgen2 at all.

Looks like all of this slipped through the cracks because nobody realized it's pretty much not exercised by anything.

@mangod9
Copy link
Member Author

mangod9 commented Jul 9, 2021

yeah will do that as a separate change. Believe there are other ones (like UnicodeString) which need to excluded for R2R.

@mangod9
Copy link
Member Author

mangod9 commented Jul 9, 2021

Any other concerns with this?

@runfoapp runfoapp bot mentioned this pull request Jul 10, 2021
Closed
MichalStrehovsky
MichalStrehovsky approved these changes Jul 12, 2021
View reviewed changes
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.

Looks good otherwise.

src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.cs Outdated Show resolved Hide resolved
src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.cs Outdated Show resolved Hide resolved
src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.cs Outdated Show resolved Hide resolved
@mangod9 mangod9 merged commit c7ebb65 into dotnet:main Jul 13, 2021
@mangod9 mangod9 deleted the usecstr branch July 13, 2021 02:49
@ManickaP ManickaP mentioned this pull request Jul 20, 2021
Merged
@agocke agocke mentioned this pull request Aug 11, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas left review comments

@MichalStrehovsky MichalStrehovsky MichalStrehovsky approved these changes

Assignees
No one assigned
Labels
area-crossgen2-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"free(): invalid pointer" with AnsiStringMarshaller compiled with crossgen2
5 participants
@mangod9 @AaronRobinsonMSFT @VSadov @MichalStrehovsky @jkotas