Throw an exception when passing strings by-value as out parameters.

[jkoritzinsky]

Block users from adding an [Out] attribute on strings and modifying them in place to avoid causing crashes with unloadability and string interning. Instead of allowing it, we'll throw a MarshalDirectiveException. Fixes #21506.

Blocked on dotnet/corefx#34091

[janvorli]

@jkoritzinsky something strange happened to the tests/src/Interop/StringMarshalling/LPTSTR/LPTSTRTest.cs file, git now thinks it is binary file for some reason and doesn't show a diff.

[jkoritzinsky]

Probably an encoding issue. I'll go fix that.

[jkoritzinsky]

Fixed. It was encoded as UTF-16 LE which GitHub has a habit of thinking is binary. Changed it to UTF-8.

[janvorli]

GitHub still thinks it is binary. Ok, I've just fetched the contents of the file and diffed it locally.

[janvorli]

The linux-musl x64 build now fails:

15:34:33 /mnt/j/workspace/dotnet_coreclr/master/linux-musl-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/vm/ilmarshalers.h:1978:23: error: 'GetNativeType' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
15:34:33     virtual LocalDesc GetNativeType();
15:34:33                       ^
15:34:33 /mnt/j/workspace/dotnet_coreclr/master/linux-musl-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/vm/ilmarshalers.h:1293:23: note: overridden virtual function is here
15:34:33     virtual LocalDesc GetNativeType() = 0;

That's because you have added the SupportsArgumentMarshal method with override and so the whole class now requires usage of override (clang's idiosyncracy). Could you please remove the override and add virtual to it?

[jkoritzinsky]

Will do.

[jkoritzinsky]

@dotnet-bot test this please.

[jkoritzinsky]

@dotnet-bot test this please

[jkoritzinsky]

It looks like some portions of System.Management rely on the current behavior. I'll need to go update them before we can merge this in.

See: https://github.com/dotnet/corefx/blob/784bd1b648dd5a69e10d6c22067c42088aaca90b/src/System.Management/src/System/Management/ManagementPath.cs#L324-L325 and https://github.com/dotnet/corefx/blob/fcea0a593a8b198d7ae741dcb80345ea2b129fe0/src/System.Management/src/System/Management/InteropClasses/WMIInterop.cs#L966

[jkoritzinsky]

@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test

[jkoritzinsky]

@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

[janvorli]

@AaronRobinsonMSFT I've just found this PR from last December was not merged in yet. Since I am not familiar with the marshalling code, I don't feel confident enough to approve the change. Could you please do that?

[AaronRobinsonMSFT]

Please update the docs about P/Invokes and root cause the Linux failure, otherwise LGTM.

[jkoritzinsky]

@AaronRobinsonMSFT I've already updated the docs to advise to not do this, but I'll go an add an update to that saying that it is disallowed in .NET Core 3.0+.

[jkoritzinsky]

@dotnet-bot test this please

[janvorli]

@jkoritzinsky the Linux-musl has a build break due to the source of the test. It is strange that it doesn't fail for non-MUSL Linux, as it complains about the wcslen and wmemcmp argument mismatch (it says the functions expect wchar_t* while we are passing in char16_t *.
The weird thing is also that other pre-existing functions in the same file used the same string comparison and they don't fail.

[janvorli]

I am trying to run the build on my local Alpine to see if I can find the real culprit.

[janvorli]

It is really weird - I can build coreclr with your change on my local Alpine just fine and the affected file is really being compiled. I'll retry the leg here.

[janvorli]

@dotnet-bot test Linux-musl x64 Debug Build please

[janvorli]

@dotnet-bot test Linux-musl x64 Debug Build please