[Crossgen2] Add DelayLoadMethodCallThunks table, fix stack walks containing thunks #39063

nattress · 2020-07-10T08:45:21Z

GC stress runs with Crossgen2 binaries frequently see AVs with stack-walk failures. This occurs when a delay load thunk is being executed as a stack-walk is performed. The runtime resolves any thunk address to the code info for the last managed method in the runtime functions table.

Fix the binary search algorithm in NativeUnwindInfoLookupTable::LookupUnwindInfoForMethod which doesn't correctly return -1 if the code address being searched for is larger than any present. This old bug was not hit because Crossgen emits stubs earlier in the image than managed code, whereas the two are reversed with Crossgen2.

Fix generation of the RuntimeFunctionsTable so that the terminating sentinel node is not treated as part of the table in the R2R header entry for the table. This was causing the sentinel node to be treated as its own runtime function by the runtime and could lead to reading beyond the table bounds in NativeUnwindInfoLookupTable::LookupUnwindInfoForMethod. The bug in the binary search of the table hid this bug until now.

Add the DelayLoadMethodCallThunks header entry. DelayLoadMethodCallThunkNodeRange is a placeholder node (not an ObjectNode) to provide a symbol to relocate against from the header. It represents the range of import thunks. Add support to the object writer for defining a range between two symbols in the same section. The relocation phase can then resolve the difference between the two symbols to determine the range size.

Fixes #38482

cc @dotnet/crossgen-contrib

…aining thunks GC stress runs with Crossgen2 binaries frequently see AVs with stack-walk failures. This occurs when a delay load thunk is being executed as a stack-walk is performed. The runtime resolves any thunk address to the code info for the last managed method in the runtime functions table. Fix the binary search algorithm in `NativeUnwindInfoLookupTable::LookupUnwindInfoForMethod` which doesn't correctly return -1 if the code address being searched for is larger than any present. This old bug was not hit because Crossgen emits stubs earlier in the image than managed code, whereas the two are reversed with Crossgen2. Fix generation of the RuntimeFunctionsTable so that the terminating sentinel node is not treated as part of the table in the R2R header entry for the table. This was causing the sentinel node to be treated as its own runtime function by the runtime and could lead to reading beyond the table bounds in `NativeUnwindInfoLookupTable::LookupUnwindInfoForMethod`. The bug in the binary search of the table hid this bug until now. Add the `DelayLoadMethodCallThunks` header entry. `DelayLoadMethodCallThunkNodeRange` is a placeholder node (not an `ObjectNode`) to provide a symbol to relocate against from the header. It represents the range of import thunks. Add support to the object writer for defining a range between two symbols in the same section. The relocation phase can then resolve the difference between the two symbols to determine the range size. Fixes dotnet#38482

davidwrighton

If you could add the additional check I suggested, I consider this a great change.

src/coreclr/src/tools/aot/ILCompiler.ReadyToRun/CodeGen/ReadyToRunObjectWriter.cs

trylek

Looks great modulo some nits and David's feedback, thanks so much Simon for making these fixes and improvements!

trylek · 2020-07-10T19:48:19Z

...ot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/RuntimeFunctionsTableNode.cs

            {
                Debug.Assert(_tableSize >= 0);
-                return _tableSize;
+                return _tableSize - 4;


Nit - it's kind of awkward that we need to duplicate this logic between this place and the section table emitter. Perhaps we might be able to shove some virtual method like EndSentinelSize or SizeDelta into HeaderTableNode and use it for these adjustments. Having said that, I know you're running out of time so please feel free to ignore it, it's no big deal and we can clean this up later if we decide it's worth it.

Good idea - I'll add a SentinelSize property so it's more descriptive.

Well, actually it's more awkward than I thought. Somewhere along the way, we stopped treating header table component nodes as HeaderTableNodes and in fact I further generalized it to DependencyNodeCode<NodeFactory> in this commit. For now I'll replace the numbers with a const in RuntimeFunctionsTableNode. HeaderTableNode currently just provides a base to factor default values for ObjectNode required overrides. We might want to just refactor it away altogether and use an interface on the header table component nodes to get the small amount of functionality needed.

Sounds good, thank you!

trylek · 2020-07-10T19:53:25Z

src/coreclr/src/vm/codeman.cpp

        {
            PTR_RUNTIME_FUNCTION pFunctionEntry = pRuntimeFunctionTable + i;
-            if (RelativePc >= pFunctionEntry->BeginAddress)
+            if (RelativePc >= pFunctionEntry->BeginAddress && RelativePc <= pFunctionEntry->EndAddress)


Is EndAddress actually a valid address? I would expect this to be the RIP after the "ret" instruction i.e. in the non-reachable filler between adjacent functions that's normally getting filled with the "INT 3" instruction (on Intel). Long story short, should this rather be "<" - especially in view of the fact that it may happen in some cases that there's no filler (i.e. the ret instruction has the three lowest-order RIP bits set to 111) in which case the <= sign could cause a spurious match?

Yes that should be < thanks!

* Pass image base address to `LookupUnwindInfoForMethod` and correct the runtime function EndAddress lookup to use the standard macro. The image base is needed on non-x64 platforms where end address isn't just part of the `RuntimeFunction` struct. Fortunately, all the callsites of `LookupUnwindInfoForMethod` had the image base easily available 🙌 * Assert if the ImportThunks are not emitted in a single run * Replace the RuntimeFunctionsTable sentinel adjustment with a const

davidwrighton · 2020-07-14T02:19:24Z

This needs a fix for Windows X86. I've taken a look and it looks practical to use the GCInfo to provide a function size. Does that sound reasonable to everyone else? As @nattress is currently unavailable, I could put that fix together.

mangod9 · 2020-07-14T04:56:18Z

both Simon and @trylek are out this week. If this can wait Tomas might be able make the changes then.

davidwrighton · 2020-07-14T05:14:59Z

We need to get this in for Preview 8, as we might get some usage on that, and the bug is very severe. I'll see if I can swing it tomorrow.

jkotas · 2020-07-14T17:45:57Z

Would it be better to handle the thunk problem by excluding the thunk range (ReadyToRunSectionType::DelayLoadMethodCallThunks) early?

Cracking the unwind info or gc info feels expensive.

davidwrighton · 2020-07-14T18:17:16Z

I like the @jkotas approach better. That's cheap, and avoids handling things specially for only 1 function. We can put the check in JitCodeToMethodInfo.

src/coreclr/src/vm/codeman.cpp

jkotas · 2020-07-14T23:29:38Z

src/coreclr/src/debug/daccess/nidump.cpp

            if (pNgenLayout->m_CodeSections[iRange].IsInRange((TADDR)addr))
            {
-                int MethodIndex = NativeUnwindInfoLookupTable::LookupUnwindInfoForMethod(rva, pNgenLayout->m_pRuntimeFunctions[iRange], 0, pNgenLayout->m_nRuntimeFunctions[iRange] - 1);
+                int MethodIndex = NativeUnwindInfoLookupTable::LookupUnwindInfoForMethod(PTR_TO_TADDR(m_decoder.GetBase()), rva, pNgenLayout->m_pRuntimeFunctions[iRange], 0, pNgenLayout->m_nRuntimeFunctions[iRange] - 1);


Revert this?

src/coreclr/src/vm/codeman.cpp

Dotnet-GitSync-Bot added the area-crossgen2-coreclr label Jul 10, 2020

nattress changed the title ~~[Crossgen2] Add DelayLoadMethodCallThunks table, fix stack walks cont…~~ [Crossgen2] Add DelayLoadMethodCallThunks table, fix stack walks containing thunks Jul 10, 2020

nattress requested review from janvorli and trylek July 10, 2020 08:46

nattress force-pushed the importthunk_stackwalking_noderange branch from 643da86 to c8361cc Compare July 10, 2020 09:26

davidwrighton reviewed Jul 10, 2020

View reviewed changes

src/coreclr/src/tools/aot/ILCompiler.ReadyToRun/CodeGen/ReadyToRunObjectWriter.cs Show resolved Hide resolved

trylek approved these changes Jul 10, 2020

View reviewed changes

nattress and others added 3 commits July 10, 2020 15:27

Sometimes there are no thunks..

6514ee5

Handle Windows X86 by using GCInfo to detect function size

c2ea1ad

mangod9 approved these changes Jul 14, 2020

View reviewed changes

davidwrighton added 2 commits July 14, 2020 11:57

Merge branch 'master' into importthunk_stackwalking_noderange

b7f444c

Check to see if IP is stub before querying runtime function structure

b68afec

janvorli reviewed Jul 14, 2020

View reviewed changes

src/coreclr/src/vm/codeman.cpp Outdated Show resolved Hide resolved

Fix location of break statement

3def4ea

davidwrighton self-assigned this Jul 14, 2020

jkotas reviewed Jul 14, 2020

View reviewed changes

src/coreclr/src/vm/codeman.cpp Show resolved Hide resolved

Address code review feedback

f39d18a

jkotas approved these changes Jul 15, 2020

View reviewed changes

davidwrighton merged commit cd9b662 into dotnet:master Jul 15, 2020

karelz added this to the 5.0.0 milestone Aug 18, 2020

janvorli mentioned this pull request Nov 27, 2020

Compile System.Private.CoreLib with Crossgen2 by default #44618

Merged

trylek mentioned this pull request Nov 27, 2020

System.Private.CoreLib size regression when compiled with Crossgen2 compared to native Crossgen #45286

Closed

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

[Crossgen2] Add DelayLoadMethodCallThunks table, fix stack walks containing thunks #39063

[Crossgen2] Add DelayLoadMethodCallThunks table, fix stack walks containing thunks #39063

Uh oh!

Conversation

nattress commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidwrighton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trylek left a comment

Choose a reason for hiding this comment

Uh oh!

trylek Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

nattress Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

nattress Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

trylek Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

trylek Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

nattress Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

davidwrighton commented Jul 14, 2020

Uh oh!

mangod9 commented Jul 14, 2020

Uh oh!

davidwrighton commented Jul 14, 2020

Uh oh!

jkotas commented Jul 14, 2020

Uh oh!

davidwrighton commented Jul 14, 2020

Uh oh!

Uh oh!

jkotas Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nattress commented Jul 10, 2020 •

edited

Loading