Skip to content

[NativeAOT] Fix stack trace iteration over exceptions #105877

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 15, 2024

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Aug 2, 2024

Fixes #101364

The test b08944b on x86 in Release causes the bomb() method to be inlined. The stack trace iterator then incorrectly adjusts the pointer to the faulting instruction by -1 which causes the try block not to be matched.

On x64 the inlined generated code for the bomb() method ended up being

00007ff7`3ecda705 33c9           xor     ecx, ecx
00007ff7`3ecda707 833900         cmp     dword ptr [rcx], 0

On x86 it is

004478ef 833d0000000000     cmp     dword ptr ds:[0], 0

Hence, for x64 the previous instruction to the faulting instruction was still in the try region. On x86 that's not the case.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 2, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 2, 2024
@filipnavara
Copy link
Member Author

filipnavara commented Aug 2, 2024

Alternatively I can change to code to remove the ApplyReturnAddressAdjustment flag in the code path that sets ExCollide. Let's wait for the CI to see what breaks...

That would not work. ApplyReturnAddressAdjustment applies to all frames in the iterator, ExCollide applies only for a single frame and it's reset on each iteration.

@MichalStrehovsky
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@filipnavara filipnavara marked this pull request as ready for review August 2, 2024 16:24
@filipnavara
Copy link
Member Author

filipnavara commented Aug 2, 2024

Hmm, the test failures do look suspicious. I'll check it in detail when all the pipelines finish running.

Cursory look at the source code suggests that we may need to remove this:

// In case m_ControlPC is pre-adjusted, counteract here, since the caller of this routine
// will apply the adjustment again once we return. If the m_ControlPC is not pre-adjusted,
// this is simply an no-op.
m_ControlPC = m_OriginalControlPC;

@tannergooding
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@filipnavara
Copy link
Member Author

filipnavara commented Aug 3, 2024

There are 6 test failures:

  • 3x JIT\Regression\JitBlue\Runtime_105821\Runtime_105821\Runtime_105821 which was meanwhile fixed by Fix Runtime_105821 ISA check #105889
  • AV in System.Formats.Asn1.Tests on linux-arm
  • SIGKILL in System.IO.Tests on linux-musl-x64
  • Timeout in System.Threading.Channels.Tests on linux-arm64

The AV in System.Formats.Asn1.Tests has interesting stack trace:

Running assembly:System.Formats.Asn1.Tests, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51
Process terminated. Access Violation: Attempted to read or write protected memory. This is often an indication that other memory is corrupt. The application will be terminated since this platform does not support throwing an AccessViolationException.
   at System.RuntimeExceptionHelpers.FailFast(String, Exception, String, RhFailFastReason, IntPtr, IntPtr) + 0x1fd
   at System.RuntimeExceptionHelpers.GetRuntimeException(ExceptionIDs) + 0x167
   at System.Runtime.EH.GetClasslibException(ExceptionIDs, IntPtr) + 0x3d
   at System.Runtime.EH.RhThrowHwEx(UInt32, EH.ExInfo&) + 0x69
   at Test.Cryptography.ByteUtils.ByteArrayToHex(ReadOnlySpan`1 bytes) + 0x5d
   at System.Formats.Asn1.Tests.Writer.WriteNamedBitList.VerifyWriteNamedBitList_KeyUsage_OneByte(AsnEncodingRules ruleSet) + 0x10d
   at System.Formats.Asn1!<BaseAddress>+0x96a10a

There's nothing in ByteArrayToHex that would obviously cause an AV exception, so it looks like something was collected by GC that shouldn't have been. That could possibly be triggered by a change in the stack walking logic but there's no exception on the given code path so I don't see how the change in this PR could have effect in this specific instance.

@jkotas
Copy link
Member

jkotas commented Aug 3, 2024

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @janvorli @VSadov Could you please review this one as well?

@JulieLeeMSFT
Copy link
Member

LGTM. @janvorli @VSadov Could you please review this one as well?

Ping @janvorli @VSadov for code review.
cc @dotnet/jit-contrib

@jakobbotsch jakobbotsch added area-NativeAOT-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Aug 14, 2024
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo a nit regarding the comment.

@janvorli janvorli merged commit 6c400b2 into dotnet:main Aug 15, 2024
92 of 94 checks passed
@filipnavara filipnavara deleted the naot-eh-iter branch August 15, 2024 17:02
@github-actions github-actions bot locked and limited conversation to collaborators Sep 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NativeAOT, win-x86] Bad EH info generated for nested trys
7 participants