dotnet / runtimelab

This repo is for experimentation and exploring new ideas that may or may not make it into the main dotnet/runtime repo.
MIT License
1.36k stars 188 forks source link

[NativeAOT-LLVM] Reject tailcalls in methods with pinned locals #2541

Closed SingleAccretion closed 3 months ago

SingleAccretion commented 3 months ago

Tailcalling risks losing the pin for the callee.

Some regressions, but this is a correctness fix:

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 3059418
Total bytes of diff: 3059909
Total bytes of delta: 491 (0.02% % of base)
Average relative delta: 2.19%
    diff is a regression
    average relative diff is a regression

Top method regressions (percentages):
           9 (11.39% of base) : 1058.dasm - System.Text.UTF32Encoding__GetByteCount_0
           9 (11.39% of base) : 1055.dasm - System.Text.UnicodeEncoding__GetByteCount_0
          13 ( 8.61% of base) : 1016.dasm - System.Globalization.CompareInfo__IcuStartsWith
          13 ( 8.61% of base) : 1026.dasm - System.Globalization.CompareInfo__IcuEndsWith
          12 ( 8.33% of base) : 1063.dasm - System.Text.UTF8Encoding__GetString
           6 ( 7.79% of base) : 1054.dasm - System.Text.Encoding__GetByteCount_4
           6 ( 6.74% of base) : 1071.dasm - System.Threading.ObjectHeader__GetSyncIndex
           3 ( 6.38% of base) : 1027.dasm - System.Text.Encoding__GetString_0
           6 ( 5.77% of base) : 1052.dasm - System.Text.Encoding__GetBytes_6
           6 ( 5.77% of base) : 1051.dasm - System.Text.Encoding__GetChars_3
          12 ( 5.66% of base) : 1056.dasm - System.Text.UTF32Encoding__GetString
          12 ( 5.66% of base) : 1050.dasm - System.Text.UnicodeEncoding__GetString
          18 ( 5.13% of base) : 1009.dasm - System.Text.DecoderNLS__GetChars_0
          18 ( 5.13% of base) : 1057.dasm - System.Text.UTF32Encoding__GetBytes
          18 ( 5.13% of base) : 1053.dasm - System.Text.UnicodeEncoding__GetBytes
           3 ( 4.55% of base) : 1013.dasm - System.Runtime.InteropServices.PInvokeMarshal__StringToAnsiString
          22 ( 4.48% of base) : 1017.dasm - System.Number__UInt64ToDecStr
           3 ( 4.48% of base) : 1012.dasm - LSSATests_IL_System_Runtime_JitTesting_LSSATestsIL__Optimized_ExposedLocal_Untracked
          15 ( 4.32% of base) : 1066.dasm - System.Text.UTF8Encoding__GetBytes
          15 ( 4.04% of base) : 1002.dasm - HelloWasm_Program__PrintLine

Top method improvements (percentages):
         -14 (-1.93% of base) : 1019.dasm - System.Number__NegativeInt32ToDecStr
          -9 (-1.05% of base) : 1086.dasm - System.Array__CopyImplValueTypeArrayWithInnerGcRefs
          -1 (-0.12% of base) : 1072.dasm - System.Threading.ObjectHeader__TryAcquireUncommon

93 total methods with Code Size differences (3 improved, 90 regressed)
SingleAccretion commented 3 months ago

@dotnet/nativeaot-llvm

SingleAccretion commented 3 months ago

If I understand this, it's a problem only if the pinned local is used by the callee (or filter)? I assume that is hard to determine, if so.

Correct. The filter case makes this tricky, as you can have code like:

static void* s_addr;

void Method() {
   pinned ref int pinnedLocal = ref ...;
   s_addr = Unsafe.AsPointer(ref pinnedLocal);
   Throw(); // Runs a filter that reads 's_addr'.
}

It's not completely hopeless as usually Roslyn zeroes out the pin after fixed is over, but it would require a bespoke analysis to prove the optimization safe.