Repository navigation
Fix ReadyToRun generic parameter symbol collisions - #136191
jtschuster wants to merge 7 commits into
Conversation
Name runtime-determined types by generic parameter kind and ordinal instead of metadata display names, avoiding colliding ReadyToRun generic lookup import symbols. Cover Unicode anonymous properties and type/method generic parameters with normal and composite Crossgen2 compilation tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 21c1c9a0-a662-4b3d-947e-e8217da4bd10
Keep the regression test non-composite since the generic symbol collision is reproduced by the single-assembly build. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 21c1c9a0-a662-4b3d-947e-e8217da4bd10
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/crossgen-contrib |
Would it be better to do this in the name mangler? |
RuntimeDeterminedType could probably become a IPrefixMangledType to enable that. We might need to change the |
Represent runtime-determined types with IPrefixMangledType so the type's kind and generic parameter index become stable UTF-8 name prefixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 21c1c9a0-a662-4b3d-947e-e8217da4bd10
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 21c1c9a0-a662-4b3d-947e-e8217da4bd10
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 21c1c9a0-a662-4b3d-947e-e8217da4bd10
| int index = _runtimeDeterminedDetailsType.Index; | ||
| _utf8Prefix = new byte[1 + FormattingHelpers.CountDigits((uint)index)]; | ||
| _utf8Prefix[0] = (byte)(_runtimeDeterminedDetailsType.Kind == GenericParameterKind.Type ? 'T' : 'M'); | ||
| bool formatted = index.TryFormat(_utf8Prefix.AsSpan(1), out int bytesWritten); |
There was a problem hiding this comment.
This will come up with the same prefix for e.g. List<__Canon> and Dictionary<__Canon, __Canon>. The RuntimeDeterminedType for the first argument is rawCanonType == __Canon for both, but detailsType is T (of List<>) in one case and TKey (of Dictionary<,> in the other).
This would need the extension I mentioned to do properly.
Based on what Copilot says, this is not a problem for ReadyToRun because it has a custom comparer for RuntimeDeterminedTypes that ignores where the generic parameter comes from and ends up comparing these two equal even though they're not equal in type system sense.
It is also not a problem for native AOT because it doesn't use mangled names for these.
However, the implementation is still wrong.
We should either make it correct, or continue doing weird things on the ReadyToRun side and e.g. also do custom mangling for these that takes into consideration the equivalency rules.
Fixes #135779
ReadyToRun runtime-determined type names used generic parameter metadata names. Unicode generic parameter names (e.g.
List<T😊>andList<T_>) could therefore sanitize to the same symbol name, causing collidingDelayLoadHelperImportsymbols.Name synthetic runtime-determined types using the generic parameter's signature identity instead:
T0,T1, etc. for type parametersM0,M1, etc. for method parametersThis preserves the diagnostic name behavior while keeping kind and ordinal identity distinct.
Added ReadyToRun compilation tests covering Unicode and punctuation cases in normal and composite builds.
Note
This PR description was generated with GitHub Copilot.