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.

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

Setting COMPlus_EnableHWIntrinsic=0 causes System.PlatformNotSupportedException with optimized libraries code #60035

Closed
echesakov opened this issue Oct 5, 2021 · 18 comments · Fixed by #62420

Comments

@echesakov
Copy link
Contributor

echesakov commented Oct 5, 2021

While working on #38162 I observed behavior (that I believe is incorrect) when running with .NET 6 SDK RC1.

To reproduce the issue build and run the project using the following sequence of commands:

dotnet build -c Release

set COMPlus_EnableHWIntrinsic=0

dotnet run -c Release

Runtime_60035.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
  </PropertyGroup>

</Project>

Runtime_60035.cs

using System;
using System.Text;
using System.Text.Encodings.Web;

namespace Runtime_60035
{
    class Program
    {
        static void Main(string[] args)
        {
            byte[] inputBytes = Encoding.UTF8.GetBytes("https://github.com/dotnet/runtime");
            Console.WriteLine(UrlEncoder.Default.FindFirstCharacterToEncodeUtf8(inputBytes));
        }
    }
}

The result will be a failure with the following stack trace

Unhandled exception. System.PlatformNotSupportedException: Operation is not supported on this platform.
   at System.Runtime.Intrinsics.X86.Sse2.LoadVector128(Byte* address)
   at System.Text.Encodings.Web.OptimizedInboxTextEncoder.GetIndexOfFirstByteToEncodeSsse3(Byte* pData, UIntPtr lengthInBytes)
   at System.Text.Encodings.Web.OptimizedInboxTextEncoder.GetIndexOfFirstByteToEncode(ReadOnlySpan`1 data)
   at System.Text.Encodings.Web.DefaultUrlEncoder.FindFirstCharacterToEncodeUtf8(ReadOnlySpan`1 utf8Text)
   at Runtime_60035.Program.Main(String[] args) in D:\echesako\GitHub\Runtime_60035\Program.cs:line 12

If in addition to setting COMPlus_EnableHWIntrinsic=0 I set COMPlus_ReadyToRun=0 the issue goes away.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Oct 5, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@echesakov
Copy link
Contributor Author

@tannergooding I wonder why we take OptimizedInboxTextEncoder.GetIndexOfFirstByteToEncodeSsse3 here with COMPlus_EnableHWIntrinsic=0.

@davidwrighton Do we have special handling for Ssse3.IsSupported and friends in crossgen2 (as we used to have in crossgen) to treat them as regular calls and not as intrinsics? Or this is done via a different mechanism in crossgen2?

@tannergooding
Copy link
Member

I wonder why we take OptimizedInboxTextEncoder.GetIndexOfFirstByteToEncodeSsse3 here with COMPlus_EnableHWIntrinsic=0.

@echesakovMSFT There was a regression at one point where EnableHWIntrinsic=0 was no longer impacting all ISAs. It's possible that was reintroduced at some point and missed.

I don't think we have any tests that explicitly check that setting the environment variable correctly controls the IsSupported flags, but perhaps those should get added.

@danmoseley
Copy link
Member

I know we discussed this in the past but I don’t know where we ended up. Do we test anything but AVX paths in the libraries (on x64)? And where we have added intrinsic paths for Arm64, do we test the software fallback paths? Just wondering how large the test gap might be.

@tannergooding
Copy link
Member

Do we test anything but AVX paths in the libraries (on x64)? And where we have added intrinsic paths for Arm64, do we test the software fallback paths? Just wondering how large the test gap might be.

I know we have tests in the runtime side to test every ISA configuration (just not that the environment variables themselves are doing the right thing).

I don't know where we landed with the libraries jobs alsoo running with those switches....

@danmoseley
Copy link
Member

I don't know where we landed with the libraries jobs alsoo running with those switches....

I don't see relevant DOTNET_ or COMPLUS_ in our yml, so I guess we have a test hole: we are shipping code we never test? Should we drive plugging that somehow before 7.0 feature coding starts, do you think? cc @safern

@tannergooding
Copy link
Member

so I guess we have a test hole: we are shipping code we never test

It was tested in all the various configurations a couple times, I just don't know/believe we have the infra to do it automatically right now.

@danmoseley
Copy link
Member

You’re quite right: I should have said that we do not test in automation. We have other such areas eg DirectoryServices tests need Docker containers. But this one seems feasible as it’s susceptible to environment variables. Could we safely pairwise this into our matrix perhaps eg Server 2022 always has AVX paths disabled?

@tannergooding
Copy link
Member

tannergooding commented Oct 6, 2021

I think we should be able to yes, especially as an outerloop. We just need to define the jobs and ensure the right environment variables are set.

I can sync with Santi or someone else tomorrow and figure this out.

@ghost
Copy link

ghost commented Oct 6, 2021

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

Issue Details

While working on #38162 I observed behavior (that I believe is incorrect) when running with .NET 6 SDK RC1.

To reproduce the issue build and run the project using the following sequence of commands:

dotnet build -c Release

set COMPlus_EnableHWIntrinsic=0

dotnet run -c Release

Runtime_60035.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
  </PropertyGroup>

</Project>

Runtime_60035.cs

using System;
using System.Text;
using System.Text.Encodings.Web;

namespace Runtime_60035
{
    class Program
    {
        static void Main(string[] args)
        {
            byte[] inputBytes = Encoding.UTF8.GetBytes("https://github.com/dotnet/runtime");
            Console.WriteLine(UrlEncoder.Default.FindFirstCharacterToEncodeUtf8(inputBytes));
        }
    }
}

The result will be a failure with the following stack trace

Unhandled exception. System.PlatformNotSupportedException: Operation is not supported on this platform.
   at System.Runtime.Intrinsics.X86.Sse2.LoadVector128(Byte* address)
   at System.Text.Encodings.Web.OptimizedInboxTextEncoder.GetIndexOfFirstByteToEncodeSsse3(Byte* pData, UIntPtr lengthInBytes)
   at System.Text.Encodings.Web.OptimizedInboxTextEncoder.GetIndexOfFirstByteToEncode(ReadOnlySpan`1 data)
   at System.Text.Encodings.Web.DefaultUrlEncoder.FindFirstCharacterToEncodeUtf8(ReadOnlySpan`1 utf8Text)
   at Runtime_60035.Program.Main(String[] args) in D:\echesako\GitHub\Runtime_60035\Program.cs:line 12

If in addition to setting COMPlus_EnableHWIntrinsic=0 I set COMPlus_ReadyToRun=0 the issue goes away.

Author: echesakovMSFT
Assignees: -
Labels:

area-System.Runtime.Intrinsics, untriaged

Milestone: -

@echesakov
Copy link
Contributor Author

@davidwrighton I am confirming (via the test #60047 I am working on) that the behavior is interaction between a crossgen2-d image and the Enable{ISA} flags.

crossgen2 assumes that Sse2.IsSupported, Ssse3.IsSupported, Sse41.IsSupported and Sse42.IsSupported (i.e. for any baseline ISA) would always return true, so, for example, setting EnableSse42=0 would not prevent the code in the R2R image from taking paths guarded with Sse42.IsSupported. However, for non-baseline ISA (e.g. Avx) setting EnableAvx=0 would lead to discarding the method code during the running time (i.e. the code would be re-jitted).

@safern
Copy link
Member

safern commented Oct 6, 2021

I think we could request the dnceng team to setup a helix queue that has avx disabled or we already have support (this was added to run jit stress tests) to submit test runs with different COMPlus/DOTNET environment variables, so we could easliy hook up setting COMPlus_EnableHWIntrinsic=0 on a libraries test run leg either on PRs or Rolling CI or even scheduled builds. Let me know what you need and I can definitely work with you to have it setup.

@danmoseley
Copy link
Member

I fear we have hijacked this issue 😄 moving to #950

@jkotas
Copy link
Member

jkotas commented Oct 7, 2021

Here is how it should work:

  • SSE and SSE2 are baseline ISAs. If the baseline ISA is not present at runtime, the whole R2R image should be rejected and everything fallbacks to JIT.
  • SSE4.1, SSE4.2, ... and bunch of others that do not require VEX encoding are optimistic ISAs. If the optimistic ISA is not present at runtime, the specific method in R2R image that took dependency on that ISA is rejected and fallbacks to JIT. It is achieved by JIT reporting ISA uses via notifyInstructionSetUsage callback on JIT/EE interface.
  • CoreLib is special in the sense that failing to use an instruction set is not a reason to not support usage of it:
    private bool notifyInstructionSetUsage(InstructionSet instructionSet, bool supportEnabled)
    {
    if (supportEnabled)
    {
    _actualInstructionSetSupported.AddInstructionSet(instructionSet);
    }
    else
    {
    // By policy we code review all changes into corelib, such that failing to use an instruction
    // set is not a reason to not support usage of it.
    if (!isMethodDefinedInCoreLib())
    {
    _actualInstructionSetUnsupported.AddInstructionSet(instructionSet);
    }
    }
    return supportEnabled;
    }

@echesakovMSFT Where do you see this breaking?

@jkotas
Copy link
Member

jkotas commented Oct 7, 2021

The problem seems to be that the env variables like EnableHWIntrinsic are only respected by the JIT and not by the VM. It means that the crossgened code and JITed code have different assumptions about the supported hardware intrinsics, and mixing of crossgened and JITed code can lead to crashes like above.

We need to move reading of these env variables to the VM.

@echesakov
Copy link
Contributor Author

Where do you see this breaking?

@jkotas I found this accidentally when I forgot to unset EnableHWIntrinsic=0 and ran build of the coreclr with the latest .NET 6 SDK. IIRC, the issue was during execution of one of MSBuild tasks during dotnet restore.

Here is how it should work: ...

I am working on the test to make sure that this functionality is properly validated in the CI.

@jeffhandley jeffhandley added this to the 7.0.0 milestone Oct 9, 2021
@jeffhandley jeffhandley removed the untriaged New issue has not been triaged by the area owner label Oct 9, 2021
echesakov added a commit to echesakov/runtime that referenced this issue Oct 15, 2021
@brantburnett
Copy link
Contributor

brantburnett commented Nov 9, 2021

I'm not certain if this is the same issue, but I encountered something similar with COMPlus_EnableSSSE3=0 and the dotnet test command after upgrading to .NET SDK 6.

In simple terms, I'm using a matrix in GitHub actions to run my unit test suite against various hardware situations in order to ensure that my fallback logic when the intrinsic is not supported is accurate. It appears that when dotnet test attempts to run build the build is failing due to the intrinsics being disabled internally for MSBuild's use.

error MSB4018: The "GenerateDepsFile" task failed unexpectedly. [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018: System.PlatformNotSupportedException: Operation is not supported on this platform. [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj][/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at System.Runtime.Intrinsics.X86.Ssse3.Shuffle(Vector128`1 value, Vector128`1 mask)
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at System.Text.Encodings.Web.OptimizedInboxTextEncoder.GetIndexOfFirstCharToEncodeSsse3(Char* pData, UIntPtr lengthInChars) [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at System.Text.Encodings.Web.OptimizedInboxTextEncoder.GetIndexOfFirstCharToEncode(ReadOnlySpan`1 data) [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at System.Text.Encodings.Web.DefaultJavaScriptEncoder.FindFirstCharacterToEncode(Char* text, Int32 textLength) [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at System.Text.Json.JsonWriterHelper.NeedsEscaping(ReadOnlySpan`1 value, JavaScriptEncoder encoder) [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at System.Text.Json.Utf8JsonWriter.WriteStartEscape(ReadOnlySpan`1 propertyName, Byte token) [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at System.Text.Json.Utf8JsonWriter.WriteStartObject(ReadOnlySpan`1 propertyName) [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at System.Text.Json.Utf8JsonWriter.WriteStartObject(String propertyName) [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.Extensions.DependencyModel.DependencyContextWriter.WriteRuntimeTargetInfo(DependencyContext context, Utf8JsonWriter jsonWriter) [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.Extensions.DependencyModel.DependencyContextWriter.Write(DependencyContext context, Stream stream) [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.NET.Build.Tasks.GenerateDepsFile.WriteDepsFile(String depsFilePath) [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.NET.Build.Tasks.GenerateDepsFile.ExecuteCore() [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.NET.Build.Tasks.TaskBase.Execute() [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute() [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]
/home/runner/.dotnet/sdk/6.0.100/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask) [/home/runner/work/Snappier/Snappier/Snappier/Snappier.csproj]

This works fine with the .NET 5 SDK, so it seems like a regression to me. However, there is a pretty easy workaround for this specific scenario. For me, the workaround is to run dotnet build first without the environment variable, then run dotnet test --no-build to test with the environment variable.

- name: Install dependencies
  run: dotnet restore
- name: Build dependencies
  run: dotnet build --configuration Release --verbosity normal
- name: Test
  run: |
    export COMPlus_Enable${{ matrix.disable }}=0 && \
    dotnet test --no-build -f ${{ matrix.framework }} --configuration Release --verbosity normal --logger "trx;LogFileName=results.trx"

@echesakov
Copy link
Contributor Author

@brantburnett I believe this is a occurrence of the same issue.

cc @davidwrighton

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 5, 2021
tannergooding added a commit to tannergooding/runtime that referenced this issue Jan 7, 2022
tannergooding added a commit that referenced this issue Jan 12, 2022
…onfig switches (#62420)

* Adding a regression test for #60035

* Add the correct "ISA implications" for Vector64/128

* Moving the `COMPlus_Enable*` HWIntrinsic ISA switches to the VM

* Ensure Set64BitInstructionSetVariants is called before EnsureValidInstructionSetSupport

* Ensure opts.compSupportsISA isn't overwritten to 0

* Ensure that Lzcnt is dependent on X86Base and AvxVnni is dependent on Avx2

* Adding x64, x86, and Arm64 specific views to help with debugging CorInfoInstructionSet

* Keep the JIT in charge of "artificial" ISAs

* Update JitBlue/Runtime_34587 to also validate the xplat SIMD types
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants