Encode calling convention in mangled function pointer representation #81122

trylek · 2023-01-24T20:41:28Z

This fixes the recently introduced issue #81106 tracking Crossgen2 crash due to not being able to distinguish two function pointers differing only by calling convention (one of the pointers is Static and the other is Static | UnmanagedCallingConvention).

Thanks

Tomas

Fixes: #81106

/cc @dotnet/crossgen-contrib

trylek · 2023-01-24T20:42:01Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2023-01-24T20:42:16Z

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky · 2023-01-25T02:10:18Z

src/coreclr/tools/Common/Compiler/NativeAotNameMangler.cs

                    var fnPtrType = (FunctionPointerType)type;
-                    mangledName = "__FnPtr" + EnterNameScopeSequence;
+                    mangledName = "__FnPtr_" + ((int)fnPtrType.Signature.Flags).ToString("X2") + EnterNameScopeSequence;


@steveharter what was the decision about function pointer type identity within the runtime? Do we only consider the value of the legacy "calling convention flags" or do we also include calling conventions from modopts, or do we consider all modopts everywhere in the signature?

(I admit I dropped the ball on following the discussion. I'll need to read up on it to actually implement this in nativeaot but can't spend time on it right now.)

This fix looks good if we only consider the legacy calling convention flags.

Are runtime tests running with NativeAOT? It seems that there is a gap in NativeAOT as well. I don't recall seeing the function pointer tests failing for NativeAOT.

@fanyang-mono I defer to Michal to elaborate in more detail, according to the running times and the logs I'm seeing for the NativeAOT legs in the runtime (PR / CI) pipeline I'm guessing that right now just a small subset of Pri0 tests gets run on a regular basis in NativeAOT mode.

This codepath is only exercised in obscure cases in NativeAOT - #71749. Most of the time function pointers are treated as IntPtr right now. It will have to change for .NET 8.

And yes, we don't run regular Pri0/Pri1 CoreCLR test suite runs right now. Another thing getting addressed for 8.0.

@steveharter what was the decision about function pointer type identity within the runtime?

@steveharter is on vacation.

In current main, the function pointer runtime type identity includes calling convention from signature. Modopts are not included in the function pointer runtime type identity.

After #81006, the function pointer runtime type identity is only going to include managed vs. unmanaged. The full calling convention details and modopts are not going to be part of function pointer runtime type identity.

Thanks Jan and Michal for your feedback. I have deleted the superfluous comment per Michal's suggestion in the 2nd commit.

trylek · 2023-01-27T19:42:12Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2023-01-27T19:42:24Z

Azure Pipelines successfully started running 1 pipeline(s).

trylek · 2023-01-30T01:32:32Z

@steveharter - could you please respond to @MichalStrehovsky's question when you have a chance? We're trying to stay on top of customer bug escalations and I'm having an active Teams thread with Mono folks who need the fix getting backported to .NET 7 so I'm trying to figure out how to make it at least to main.

src/coreclr/tools/Common/Compiler/NativeAotNameMangler.cs

This fixes the recently introduced issue dotnet#81106 tracking Crossgen2 crash due to not being able to distinguish two function pointers differing only by calling convention (one of the pointers is Static and the other is Static | UnmanagedCallingConvention). Thanks Tomas

trylek · 2023-01-30T23:06:48Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2023-01-30T23:07:14Z

Azure Pipelines successfully started running 1 pipeline(s).

trylek · 2023-01-31T17:14:23Z

/azp runtime-coreclr outerloop

azure-pipelines · 2023-01-31T17:14:29Z

Command 'runtime-coreclr' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

trylek · 2023-01-31T17:14:42Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2023-01-31T17:15:00Z

Azure Pipelines successfully started running 1 pipeline(s).

trylek · 2023-01-31T20:21:02Z

The one failure in the DictionaryExpansion test on Windows x86 is known,

#75244

I'll probably take the bug as it's currently assigned to @davidwrighton who's expected to be OOF for a prolonged period of time. I'm going to merge this in assuming no other untracked issues pop up in the remaining outerloop jobs.

trylek · 2023-01-31T20:51:01Z

OK, there's one more bug but that's also known, #81109, I'm actually investigating it right now, that's obviously unrelated to this change, merging in.

trylek · 2023-01-31T20:52:59Z

/backport to release/7.0

github-actions · 2023-01-31T20:53:11Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4058243565

github-actions · 2023-01-31T20:55:13Z

@trylek backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Encode calling convention in mangled function pointer representation
Applying: Delete superfluous comment per Michal's PR feedback
Applying: Remove issues.targets exclusion for the function pointer test
Using index info to reconstruct a base tree...
M	src/tests/issues.targets
Falling back to patching base and 3-way merge...
Auto-merging src/tests/issues.targets
CONFLICT (content): Merge conflict in src/tests/issues.targets
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 Remove issues.targets exclusion for the function pointer test
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-01-31T20:55:15Z

@trylek an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

Based on tactical discussion with Fan and Sam I have backported the PR #81122 including the removal of the issues.targets entry for function pointer tests as an extra commit on top of the backport of Fan's PR #80855. Thanks Tomas

* Fix function pointer check * Make is_monomorphic_array return false when the element is function pointer * Move test to a better location * Disable function pointer test for crossgen due to an existing issue * Backport of PR #81122 to .NET 7.0, revert change to issues.targets Based on tactical discussion with Fan and Sam I have backported the PR #81122 including the removal of the issues.targets entry for function pointer tests as an extra commit on top of the backport of Fan's PR #80855. Thanks Tomas --------- Co-authored-by: Fan Yang <yangfan@microsoft.com> Co-authored-by: Fan Yang <52458914+fanyang-mono@users.noreply.github.com> Co-authored-by: Tomas <trylek@microsoft.com>

trylek added the area-crossgen2-coreclr label Jan 24, 2023

trylek added this to the 8.0.0 milestone Jan 24, 2023

trylek requested a review from jkotas January 24, 2023 20:41

trylek requested a review from MichalStrehovsky as a code owner January 24, 2023 20:41

ghost assigned trylek Jan 24, 2023

runfoapp bot mentioned this pull request Jan 24, 2023

Infra improvements for Helix #68176

Closed

build-analysis bot mentioned this pull request Jan 24, 2023

tracing/eventpipe/eventsourceerror/eventsourceerror/eventsourceerror failure #80666

Closed

MichalStrehovsky reviewed Jan 25, 2023

View reviewed changes

SamMonoRT mentioned this pull request Jan 25, 2023

[release/7.0] [Mono] Fix function pointer check #80927

Merged

trylek closed this Jan 27, 2023

trylek reopened this Jan 27, 2023

trylek mentioned this pull request Jan 27, 2023

Disable function pointer test for crossgen due to an existing issue #81286

Merged

MichalStrehovsky approved these changes Jan 30, 2023

View reviewed changes

src/coreclr/tools/Common/Compiler/NativeAotNameMangler.cs Outdated Show resolved Hide resolved

trylek added 3 commits January 31, 2023 00:05

Delete superfluous comment per Michal's PR feedback

e293180

Remove issues.targets exclusion for the function pointer test

273ead6

trylek force-pushed the 81106 branch from a16bde6 to 273ead6 Compare January 30, 2023 23:06

trylek closed this Jan 31, 2023

trylek reopened this Jan 31, 2023

trylek merged commit f70d8a9 into dotnet:main Jan 31, 2023

trylek deleted the 81106 branch January 31, 2023 20:52

build-analysis bot mentioned this pull request Jan 31, 2023

Test failure Loader\\classloader\\DictionaryExpansion\\DictionaryExpansion\\DictionaryExpansion.cmd #75244

Closed

ghost locked as resolved and limited conversation to collaborators Mar 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode calling convention in mangled function pointer representation #81122

Encode calling convention in mangled function pointer representation #81122

trylek commented Jan 24, 2023

trylek commented Jan 24, 2023

azure-pipelines bot commented Jan 24, 2023

MichalStrehovsky Jan 25, 2023

fanyang-mono Jan 25, 2023

trylek Jan 26, 2023

MichalStrehovsky Jan 26, 2023

fanyang-mono Jan 26, 2023

jkotas Jan 30, 2023 •

edited

Loading

trylek Jan 30, 2023

trylek commented Jan 27, 2023

azure-pipelines bot commented Jan 27, 2023

trylek commented Jan 30, 2023

trylek commented Jan 30, 2023

azure-pipelines bot commented Jan 30, 2023

trylek commented Jan 31, 2023

azure-pipelines bot commented Jan 31, 2023

trylek commented Jan 31, 2023

azure-pipelines bot commented Jan 31, 2023

trylek commented Jan 31, 2023

trylek commented Jan 31, 2023

trylek commented Jan 31, 2023

github-actions bot commented Jan 31, 2023

github-actions bot commented Jan 31, 2023

github-actions bot commented Jan 31, 2023

Encode calling convention in mangled function pointer representation #81122

Encode calling convention in mangled function pointer representation #81122

Conversation

trylek commented Jan 24, 2023

trylek commented Jan 24, 2023

azure-pipelines bot commented Jan 24, 2023

MichalStrehovsky Jan 25, 2023

Choose a reason for hiding this comment

fanyang-mono Jan 25, 2023

Choose a reason for hiding this comment

trylek Jan 26, 2023

Choose a reason for hiding this comment

MichalStrehovsky Jan 26, 2023

Choose a reason for hiding this comment

fanyang-mono Jan 26, 2023

Choose a reason for hiding this comment

jkotas Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

trylek Jan 30, 2023

Choose a reason for hiding this comment

trylek commented Jan 27, 2023

azure-pipelines bot commented Jan 27, 2023

trylek commented Jan 30, 2023

trylek commented Jan 30, 2023

azure-pipelines bot commented Jan 30, 2023

trylek commented Jan 31, 2023

azure-pipelines bot commented Jan 31, 2023

trylek commented Jan 31, 2023

azure-pipelines bot commented Jan 31, 2023

trylek commented Jan 31, 2023

trylek commented Jan 31, 2023

trylek commented Jan 31, 2023

github-actions bot commented Jan 31, 2023

github-actions bot commented Jan 31, 2023

github-actions bot commented Jan 31, 2023

jkotas Jan 30, 2023 •

edited

Loading