Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix finding entry point unwind record for a method with fragmented un… #22202

Merged
merged 3 commits into from
Jan 28, 2019

Conversation

AndyAyersMS
Copy link
Member

…wind

On Arm and Arm64 unwind records can only cover a limited range of code
(512K and 1MB respectively). So for methods larger than this the jit
will emit multiple "fragment" unwind records to cover the full method code
range. Only the first of these describes the behavior of the method prolog.

When mapping an offset back to a method's entry point unwind, make sure to
find this "root" unwind record instead of one of the internal fragments.

Fixes #19209.

…wind

On Arm and Arm64 unwind records can only cover a limited range of code
(512K and 1MB respectively). So for methods larger than this the jit
will emit multiple "fragment" unwind records to cover the full method code
range. Only the first of these describes the behavior of the method prolog.

When mapping an offset back to a method's entry point unwind, make sure to
find this "root" unwind record instead of one of the internal fragments.

Fixes #19209.
@AndyAyersMS
Copy link
Member Author

@janvorli PTAL
cc @BruceForstall

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM.

Might be useful to add a header comment to EEJitManager::LazyGetFunctionEntry documenting that it will return the "root" RUNTIME_FUNCTION entry (containing the prolog).

Also, I guess it's ok you removed the comment above the --pFunctionEntry line, but I think it would be useful to leave it, also.

@AndyAyersMS
Copy link
Member Author

@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc_jitstress1 Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc_jitstress2 Build and Test

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, thank you!

@janvorli
Copy link
Member

@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc_jitstress1 Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc_jitstress2 Build and Test

1 similar comment
@janvorli
Copy link
Member

@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc_jitstress1 Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc_jitstress2 Build and Test

src/vm/codeman.cpp Outdated Show resolved Hide resolved
@AndyAyersMS
Copy link
Member Author

Looking at GC stress failures

  • b163200 is some other issue, was failing before this fix (#19464, #19986)
  • generics/WaitCallback/thread23 seems like a new failure (perhaps the null issue Jan spotted -- will see if this repros once I address that)

@AndyAyersMS
Copy link
Member Author

Latest push should fix the null issue and adds (restores) some comments.

@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc_jitstress1 Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc_jitstress2 Build and Test

@AndyAyersMS
Copy link
Member Author

Sigh. Need to actually return the entry instead of always returning null. Will push a fix shortly.

@AndyAyersMS
Copy link
Member Author

@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc_jitstress1 Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc_jitstress2 Build and Test

@AndyAyersMS
Copy link
Member Author

Hmm, build issues for the GC stress legs... did not realize test legs can share a build leg like this.

12:24:14   bne_opt -> /mnt/j/workspace/dotnet_coreclr/master/arm64_cross_checked_ubuntu16.04_prtest/bin/tests/Linux.arm64.Checked/JIT/jit64/opt/regress/vswhidbey/223862/bne_opt/bne_opt.exe
12:24:15 Command execution failed with exit code 139.

@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc_jitstress1 Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked gcstress0xc_jitstress2 Build and Test

@AndyAyersMS
Copy link
Member Author

Remaining stress failures are all known issues

  • GetGenerationWR (#19211)
  • b163200 (#19464)

@AndyAyersMS
Copy link
Member Author

@janvorli want to take one more look?

@janvorli
Copy link
Member

@AndyAyersMS it looks good to me.

@AndyAyersMS AndyAyersMS merged commit 1b8df83 into dotnet:master Jan 28, 2019
@AndyAyersMS AndyAyersMS deleted the Arm64FunctionFragmentFix branch January 28, 2019 18:45
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
dotnet/coreclr#22202)

On Arm and Arm64 unwind records can only cover a limited range of code
(512K and 1MB respectively). So for methods larger than this the jit
will emit multiple "fragment" unwind records to cover the full method code
range. Only the first of these describes the behavior of the method prolog.

When mapping an offset back to a method's entry point unwind, make sure to
find this "root" unwind record instead of one of the internal fragments.

Fixes dotnet/coreclr#19209.


Commit migrated from dotnet/coreclr@1b8df83
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants