JIT: don't optimize calls to CORINFO_HELP_VIRTUAL_FUNC_PTR for interface class lookups

[AndyAyersMS]

The jit currently replaces calls to CORINFO_HELP_VIRTUAL_FUNC_PTR whose results are unused with null pointer checks.

But if we're looking up a method on an interface, this helper can now throw a variety of exceptions, so this optimization is incorrect.

Fixes #82127.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The jit currently replaces calls to CORINFO_HELP_VIRTUAL_FUNC_PTR whose results are unused with null pointer checks.

But this helper can now throw a variety of exceptions, so this optimization is incorrect.

Fixes #82127.

Author:	AndyAyersMS
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[AndyAyersMS]

@BruceForstall PTAL
cc @dotnet/jit-contrib

I could not figure out when or why the helper call behavior changed, but per SPMI this optimization only kicks in for this one test, so it's not that important.

[edit: initially thought the diff I saw was from the Main in reabstraction but realize now this is likely some other method. Tracking from SPMI method instances back to test cases is not always easy.]

[jkotas]

I have added this optimization on 2004/08/31 ... memories :-) It was based on customer feedback: Some (3rd party?) compiler used ldvirtftn + pop to implement null checks and the ask was to make it more efficient. Is this idiomatic pattern to implement null checks going to regress?

[AndyAyersMS]

Is this idiomatic pattern to implement null checks going to regress?

Yes, if it is heavily used by some class of IL generators, we'll end up with helper calls instead of simple null checks, and perhaps miss downstream inferences that after this point the ref can't be null (that part I'd need to investigate further).

It seems like the possibility of some other exception kinds being thrown by CORINFO_HELP_VIRTUAL_FUNC_PTR is a relatively new thing -- I have not yet been able to track down exactly when or how this changed.

[jkotas]

The interface method resolution can fail for number of different reasons. It has been the case since forever, we just did not notice. I think it should be still ok to do this optimization for non-interface methods.

[AndyAyersMS]

Odd and seemingly unrelated failure on musl arm/arm64:

  Discovering: System.Collections.Immutable.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Collections.Immutable.Tests (found 2901 of 2986 test cases)
  Starting:    System.Collections.Immutable.Tests (parallel test collections = on, max threads = 2)

Assert failure(PID 22 [0x00000016], Thread: 41 [0x0029]): pImportSection == pModule->GetImportSectionForRVA(rva)
    File: /__w/1/s/src/coreclr/vm/prestub.cpp Line: 2991
    Image: /root/helix/work/correlation/dotnet

May be related, there are some diffs in arm64 linux crossgen2 to investigate.

[AndyAyersMS]

I don't see those diffs building locally.

[AndyAyersMS]

Going to update to latest main and retry.

[BruceForstall]

Testing hit a lot of infra issues. Going to cycle it.

[BruceForstall]

/azp run runtime-jit-experimental

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[AndyAyersMS]

Tizen armel: dotnet build tool installation crashed

Welcome to .NET 8.0!
---------------------
SDK Version: 8.0.100-alpha.1.23061.8

----------------
Installed an ASP.NET Core HTTPS development certificate.
To trust the certificate run 'dotnet dev-certs https --trust' (Windows and macOS only).
Learn about HTTPS: https://aka.ms/dotnet-https
----------------
Write your first app: https://aka.ms/dotnet-hello-world
Find out what's new: https://aka.ms/dotnet-whats-new
Explore documentation: https://aka.ms/dotnet-docs
Report issues and find source on GitHub: https://github.com/dotnet/core
Use 'dotnet --help' to see available commands or visit: https://aka.ms/dotnet-cli
--------------------------------------------------------------------------------------
/__w/1/s/eng/common/tools.sh: line 469:   547 Segmentation fault      (core dumped) "$_InitializeBuildTool" "$@"

[AndyAyersMS]

This PR is jinxed.

[AndyAyersMS]

Failure to unzip build assets:

Searching for: **\libraries_bin_windows_x64_Release.zip under directory: D:\a\_work\1\s\__download__\libraries_bin_windows_x64_Release\
Found: 1 files to extract:
D:\a\_work\1\s\__download__\libraries_bin_windows_x64_Release\libraries_bin_windows_x64_Release.zip
Extracting file: D:\a\_work\1\s\__download__\libraries_bin_windows_x64_Release\libraries_bin_windows_x64_Release.zip
D:\a\_work\_tasks\ExtractFiles_5e1e3830-fbfb-11e5-aab1-090c92bc4988\1.213.0\7zip\7z.exe x -oD:\a\_work\1\s\artifacts D:\a\_work\1\s\__download__\libraries_bin_windows_x64_Release\libraries_bin_windows_x64_Release.zip
ERROR: D:\a\_work\1\s\__download__\libraries_bin_windows_x64_Release\libraries_bin_windows_x64_Release.zip
Can not open the file as archive

[AndyAyersMS]

Runtime failure looks like an instance of #25979.

[AndyAyersMS]

SPMI replay failure is #82397.