Skip to content

[NativeAOT] macOS/iOS: Emit simple compact unwinding information - #88724

Merged
VSadov merged 4 commits into
dotnet:mainfrom
filipnavara:compact-unwind
Aug 3, 2023
Merged

[NativeAOT] macOS/iOS: Emit simple compact unwinding information#88724
VSadov merged 4 commits into
dotnet:mainfrom
filipnavara:compact-unwind

Conversation

@filipnavara

@filipnavarafilipnavara commented Jul 12, 2023

Copy link
Copy Markdown
Member

Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.

This is minimum viable prototype that implements the following:

  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.

This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.

Contributes to #76371, #88292

@ghostghost added area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member labels Jul 12, 2023
@filipnavara
filipnavara requested a review from VSadovJuly 12, 2023 10:19
@ghost

ghost commented Jul 12, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.

This is minimum viable prototype that implements the following:

  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.

This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.

Contributes to #76371, #88292

Author:filipnavara
Assignees:-
Labels:

area-NativeAOT-coreclr

Milestone:-

filipnavara
filipnavara commented Jul 12, 2023
View reviewed changes
Comment threadsrc/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectWriter.cs Outdated
@build-analysisbuild-analysisBot mentioned this pull request Jul 12, 2023
Closed
@filipnavara
filipnavara marked this pull request as ready for review July 14, 2023 06:26
@filipnavara
filipnavara requested a review from jkotasJuly 17, 2023 19:36
@jkotas

jkotas commented Jul 18, 2023

Copy link
Copy Markdown
Member

The general idea looks good to me. The size savings are very nice.

The only wrinkle is encoding of the prolog sizes. It looks odd that the prolog sizes are encoded explicitly, and the epilog sizes are inferred from the code. What would it take to infer the prolog size from the compact unwind info or by inspecting the code?

@jkotas

jkotas commented Jul 18, 2023

Copy link
Copy Markdown
Member

What would it take to infer the prolog size from the compact unwind info or by inspecting the code?

It should be only needed during GetReturnAddressHijackInfo. It should not be needed during regular unwind. We should never call general unwind in prolog or epilog.

@filipnavara

filipnavara commented Jul 18, 2023

Copy link
Copy Markdown
MemberAuthor

I originally inferred the prolog size from the instructions but it feels a bit fragile once you start doing complex prologs (not included in the PR). The compact unwind code by itself is not sufficient. It describes the stack layout but not which instructions were used to produce it. Due to various ways how ARM64 code can fold pointer adjustments into single instruction it would be fragile for the general case (again, not issue for this single unwind code that's produced in this PR).

If you feel strongly about inferring the prolog size from code I will do it for this simple case and we can extend it later to cover the hard cases.

@jkotas

jkotas commented Jul 18, 2023

Copy link
Copy Markdown
Member

We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.

@filipnavara

filipnavara commented Aug 2, 2023

Copy link
Copy Markdown
MemberAuthor

We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.

Done. Let's see if the CI passes... (but it seemed to work locally and crossgen2 compiled the framework just fine)

@dotnetdotnet deleted a comment from azure-pipelinesBotAug 2, 2023
@dotnetdotnet deleted a comment from azure-pipelinesBotAug 2, 2023
@jkotas

jkotas commented Aug 2, 2023

Copy link
Copy Markdown
Member

/azp run runtime-extra-platforms

@azure-pipelines

azure-pipelinesBot commented Aug 2, 2023

Copy link
Copy Markdown

No commit pushedDate could be found for PR 88724 in repo dotnet/runtime

@jkotas

jkotas commented Aug 2, 2023

Copy link
Copy Markdown
Member

/azp run runtime-extra-platforms

@azure-pipelines

azure-pipelinesBot commented Aug 2, 2023

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

jkotas
jkotas approved these changes Aug 2, 2023
View reviewed changes

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. @VSadov Could you please take a look as well?

@VSadov

VSadov commented Aug 2, 2023

Copy link
Copy Markdown
Member

@VSadov Could you please take a look as well?

I will take a look.

VSadov
VSadov reviewed Aug 3, 2023
View reviewed changes
Comment threadsrc/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp Outdated
VSadov
VSadov approved these changes Aug 3, 2023
View reviewed changes

@VSadovVSadov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. (just a couple nits). Thanks!!!

@build-analysisbuild-analysisBot mentioned this pull request Aug 3, 2023
Closed
This was referenced Aug 3, 2023
Closed
Open
Closed
Closed
Closed
Closed
Closed
@VSadov

VSadov commented Aug 3, 2023

Copy link
Copy Markdown
Member

Thanks!

@VSadov
VSadov merged commit 5bf45ce into dotnet:mainAug 3, 2023
@filipnavara
filipnavara deleted the compact-unwind branch August 3, 2023 15:01
@jkotas

jkotas commented Aug 4, 2023

Copy link
Copy Markdown
Member

This is causing a lot of intermittent crashes on Linux arm64 . For example: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-fdbcd47e91a0464083/System.Collections.Concurrent.Tests/1/console.07e57ce1.log?helixlogtype=result

In checked builds, the crash is typically assertion failure in UnixNativeCodeManager::GetReturnAddressHijackInfo:

* thread #1, name = 'Microsoft.Exten', stop reason = signal SIGABRT
* frame #0: 0x0000ff5c2f85deac
frame #1: 0x0000ff5c2f84aaa0
frame #2: 0x0000aad8f9112abc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Assert(expr=<unavailable>, file=<unavailable>, line_num=<unavailable>, message=<unavailable>)
frame #3: 0x0000aad8f9170dd0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`UnixNativeCodeManager::GetReturnAddressHijackInfo(this=0x0000000000000001, pMethodInfo=0x0000ff5c2f99ac00, pRegisterSet=0x0000ff1b91ffb0d0, ppvRetAddrLocation=0x0000ff5c2f99a000, pRetValueKind=0x0000ff1b91ff78bc) at P:810:5
frame #4: 0x0000aad8f911dfb0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackReturnAddressWorker(this=0x0000ff1b91ffb7f8, frameIterator=0x0000ff1b91ff7918, pfnHijackFunction=(Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`RhpGcProbeHijack))())
frame #5: 0x0000aad8f911dbdc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(UNIX_CONTEXT*, void*) [inlined] Thread::HijackReturnAddress(this=0x0000ff1b91ffb7f8, pSuspendCtx=0x0000ff1b91ff7ca0)())
frame #6: 0x0000aad8f911db6c Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(pThreadContext=<unavailable>, pThreadToHijack=<unavailable>)
frame #7: 0x0000aad8f9167810 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`ActivationHandler(code=34, siginfo=0x0000ff1b91ff7c20, context=0x0000ff1b91ff7ca0)
frame #8: 0x0000ff5c2fabe5b0 linux-vdso.so.1
frame #9: 0x0000aad8f9508434 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`S_P_CoreLib_System_Collections_Generic_List_1<System___Canon>__Sort_2(this=<unavailable>, comparison=<unavailable>) at List.cs:1091
frame #10: 0x0000aad8f943c130 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Microsoft_Extensions_Configuration_Microsoft_Extensions_Configuration_ConfigurationProvider__GetChildKeys(this=<unavailable>, earlierKeys=<unavailable>, parentPath=<unavailable>) at ConfigurationProvider.cs:91

jkotas added a commit that referenced this pull request Aug 4, 2023
@jkotas
Revert "[NativeAOT] macOS/iOS: Emit simple compact unwinding informat…
549af5b
…ion (#88724)"
This reverts commit 5bf45ce.
@jkotasjkotas mentioned this pull request Aug 4, 2023
Closed
@filipnavara

filipnavara commented Aug 4, 2023

Copy link
Copy Markdown
MemberAuthor

This is causing a lot of intermittent crashes on Linux arm64 .

Thanks for alert, I will check. I must have messed up some condition because it should have had no effect on Linux.

@filipnavara

filipnavara commented Aug 4, 2023

Copy link
Copy Markdown
MemberAuthor

I see the mistake now, will submit a fix shortly...

@filipnavarafilipnavara mentioned this pull request Aug 4, 2023
Merged
@ghostghost locked as resolved and limited conversation to collaborators Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas approved these changes
@VSadovVSadovVSadov approved these changes
@ivanpovazanivanpovazanAwaiting requested review from ivanpovazan
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees

No one assigned

Labels

area-NativeAOT-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@filipnavara@jkotas@VSadov
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
[NativeAOT] macOS/iOS: Emit simple compact unwinding information by filipnavara · Pull Request #88724 · dotnet/runtime · GitHub
Skip to content
[NativeAOT] macOS/iOS: Emit simple compact unwinding information - #88724
Merged
VSadov merged 4 commits into
dotnet:mainfrom 
filipnavara:compact-unwind
Aug 3, 2023
Merged
[NativeAOT] macOS/iOS: Emit simple compact unwinding information#88724
VSadov merged 4 commits into
dotnet:mainfrom 
filipnavara:compact-unwind
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jul 12, 2023

Copy link


Copy Markdown

Member
Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.
This is minimum viable prototype that implements the following:
  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.
This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.
Contributes to #76371, #88292
@ghostghost
added

area-NativeAOT-coreclr

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 12, 2023
@filipnavara
filipnavara
requested a review
from VSadovJuly 12, 2023 10:19
@ghost
ghost
commented
Jul 12, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas

See info in area-owners.md if you want to be subscribed.
Issue Details
Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.
This is minimum viable prototype that implements the following:
  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.
This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.
Contributes to #76371, #88292
Author:filipnavara
Assignees:-
Labels:
area-NativeAOT-coreclr
Milestone:-
filipnavara
filipnavara
commented
Jul 12, 2023
View reviewed changes
Comment threadsrc/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectWriter.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 12, 2023

Closed

@filipnavara
filipnavara
marked this pull request as ready for review
July 14, 2023 06:26
@filipnavara
filipnavara
requested a review
from jkotasJuly 17, 2023 19:36
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
The general idea looks good to me. The size savings are very nice.
The only wrinkle is encoding of the prolog sizes. It looks odd that the prolog sizes are encoded explicitly, and the epilog sizes are inferred from the code. What would it take to infer the prolog size from the compact unwind info or by inspecting the code?
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
What would it take to infer the prolog size from the compact unwind info or by inspecting the code?
It should be only needed during GetReturnAddressHijackInfo. It should not be needed during regular unwind. We should never call general unwind in prolog or epilog.
@filipnavara
filipnavara
commented
Jul 18, 2023

Copy link


Copy Markdown

MemberAuthor
I originally inferred the prolog size from the instructions but it feels a bit fragile once you start doing complex prologs (not included in the PR). The compact unwind code by itself is not sufficient. It describes the stack layout but not which instructions were used to produce it. Due to various ways how ARM64 code can fold pointer adjustments into single instruction it would be fragile for the general case (again, not issue for this single unwind code that's produced in this PR).
If you feel strongly about inferring the prolog size from code I will do it for this simple case and we can extend it later to cover the hard cases.
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.
@filipnavara
filipnavara
commented
Aug 2, 2023

Copy link


Copy Markdown

MemberAuthor
We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.
Done. Let's see if the CI passes... (but it seemed to work locally and crossgen2 compiled the framework just fine)
@dotnetdotnet
deleted a comment from azure-pipelinesBotAug 2, 2023
@dotnetdotnet
deleted a comment from azure-pipelinesBotAug 2, 2023
@jkotas
jkotas
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 2, 2023

Copy link


Copy Markdown

No commit pushedDate could be found for PR 88724 in repo dotnet/runtime
@jkotas
jkotas
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
jkotas
jkotas
approved these changes
Aug 2, 2023
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. @VSadov Could you please take a look as well?
@VSadov
VSadov
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
@VSadov Could you please take a look as well?
I will take a look.
VSadov
VSadov
reviewed
Aug 3, 2023
View reviewed changes
Comment threadsrc/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp
Outdated

VSadov
VSadov
approved these changes
Aug 3, 2023
View reviewed changes
@VSadovVSadov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. (just a couple nits). Thanks!!!
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 3, 2023

Closed


This was referenced Aug 3, 2023

Closed


Open


Closed


Closed


Closed


Closed


Closed

@VSadov
VSadov
commented
Aug 3, 2023

Copy link


Copy Markdown

Member
Thanks!
@VSadov
VSadov
merged commit 5bf45ce
into
dotnet:mainAug 3, 2023
@filipnavara
filipnavara
deleted the

compact-unwind

branch
August 3, 2023 15:01
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This is causing a lot of intermittent crashes on Linux arm64 . For example: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-fdbcd47e91a0464083/System.Collections.Concurrent.Tests/1/console.07e57ce1.log?helixlogtype=result
In checked builds, the crash is typically assertion failure in UnixNativeCodeManager::GetReturnAddressHijackInfo:
* thread #1, name = 'Microsoft.Exten', stop reason = signal SIGABRT
* frame #0: 0x0000ff5c2f85deac
frame #1: 0x0000ff5c2f84aaa0
frame #2: 0x0000aad8f9112abc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Assert(expr=<unavailable>, file=<unavailable>, line_num=<unavailable>, message=<unavailable>)
frame #3: 0x0000aad8f9170dd0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`UnixNativeCodeManager::GetReturnAddressHijackInfo(this=0x0000000000000001, pMethodInfo=0x0000ff5c2f99ac00, pRegisterSet=0x0000ff1b91ffb0d0, ppvRetAddrLocation=0x0000ff5c2f99a000, pRetValueKind=0x0000ff1b91ff78bc) at P:810:5
frame #4: 0x0000aad8f911dfb0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackReturnAddressWorker(this=0x0000ff1b91ffb7f8, frameIterator=0x0000ff1b91ff7918, pfnHijackFunction=(Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`RhpGcProbeHijack))())
frame #5: 0x0000aad8f911dbdc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(UNIX_CONTEXT*, void*) [inlined] Thread::HijackReturnAddress(this=0x0000ff1b91ffb7f8, pSuspendCtx=0x0000ff1b91ff7ca0)())
frame #6: 0x0000aad8f911db6c Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(pThreadContext=<unavailable>, pThreadToHijack=<unavailable>)
frame #7: 0x0000aad8f9167810 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`ActivationHandler(code=34, siginfo=0x0000ff1b91ff7c20, context=0x0000ff1b91ff7ca0)
frame #8: 0x0000ff5c2fabe5b0 linux-vdso.so.1
frame #9: 0x0000aad8f9508434 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`S_P_CoreLib_System_Collections_Generic_List_1<System___Canon>__Sort_2(this=<unavailable>, comparison=<unavailable>) at List.cs:1091
frame #10: 0x0000aad8f943c130 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Microsoft_Extensions_Configuration_Microsoft_Extensions_Configuration_ConfigurationProvider__GetChildKeys(this=<unavailable>, earlierKeys=<unavailable>, parentPath=<unavailable>) at ConfigurationProvider.cs:91

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "[NativeAOT] macOS/iOS: Emit simple compact unwinding informat…
549af5b
…ion (#88724)"
This reverts commit 5bf45ce.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Closed

@filipnavara
filipnavara
commented
Aug 4, 2023

Copy link


Copy Markdown

MemberAuthor
This is causing a lot of intermittent crashes on Linux arm64 .
Thanks for alert, I will check. I must have messed up some condition because it should have had no effect on Linux.
@filipnavara
filipnavara
commented
Aug 4, 2023

Copy link


Copy Markdown

MemberAuthor
I see the mistake now, will submit a fix shortly...
@filipnavarafilipnavara
mentioned this pull request
Aug 4, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@VSadovVSadovVSadov approved these changes
@ivanpovazanivanpovazanAwaiting requested review from ivanpovazan
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees


No one assigned


Labels

area-NativeAOT-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@filipnavara@jkotas@VSadov

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
[NativeAOT] macOS/iOS: Emit simple compact unwinding information by filipnavara · Pull Request #88724 · dotnet/runtime · GitHub
Skip to content
[NativeAOT] macOS/iOS: Emit simple compact unwinding information - #88724
Merged
VSadov merged 4 commits into
dotnet:mainfrom 
filipnavara:compact-unwind
Aug 3, 2023
Merged
[NativeAOT] macOS/iOS: Emit simple compact unwinding information#88724
VSadov merged 4 commits into
dotnet:mainfrom 
filipnavara:compact-unwind
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jul 12, 2023

Copy link


Copy Markdown

Member
Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.
This is minimum viable prototype that implements the following:
  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.
This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.
Contributes to #76371, #88292
@ghostghost
added

area-NativeAOT-coreclr

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 12, 2023
@filipnavara
filipnavara
requested a review
from VSadovJuly 12, 2023 10:19
@ghost
ghost
commented
Jul 12, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas

See info in area-owners.md if you want to be subscribed.
Issue Details
Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.
This is minimum viable prototype that implements the following:
  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.
This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.
Contributes to #76371, #88292
Author:filipnavara
Assignees:-
Labels:
area-NativeAOT-coreclr
Milestone:-
filipnavara
filipnavara
commented
Jul 12, 2023
View reviewed changes
Comment threadsrc/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectWriter.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 12, 2023

Closed

@filipnavara
filipnavara
marked this pull request as ready for review
July 14, 2023 06:26
@filipnavara
filipnavara
requested a review
from jkotasJuly 17, 2023 19:36
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
The general idea looks good to me. The size savings are very nice.
The only wrinkle is encoding of the prolog sizes. It looks odd that the prolog sizes are encoded explicitly, and the epilog sizes are inferred from the code. What would it take to infer the prolog size from the compact unwind info or by inspecting the code?
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
What would it take to infer the prolog size from the compact unwind info or by inspecting the code?
It should be only needed during GetReturnAddressHijackInfo. It should not be needed during regular unwind. We should never call general unwind in prolog or epilog.
@filipnavara
filipnavara
commented
Jul 18, 2023

Copy link


Copy Markdown

MemberAuthor
I originally inferred the prolog size from the instructions but it feels a bit fragile once you start doing complex prologs (not included in the PR). The compact unwind code by itself is not sufficient. It describes the stack layout but not which instructions were used to produce it. Due to various ways how ARM64 code can fold pointer adjustments into single instruction it would be fragile for the general case (again, not issue for this single unwind code that's produced in this PR).
If you feel strongly about inferring the prolog size from code I will do it for this simple case and we can extend it later to cover the hard cases.
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.
@filipnavara
filipnavara
commented
Aug 2, 2023

Copy link


Copy Markdown

MemberAuthor
We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.
Done. Let's see if the CI passes... (but it seemed to work locally and crossgen2 compiled the framework just fine)
@dotnetdotnet
deleted a comment from azure-pipelinesBotAug 2, 2023
@dotnetdotnet
deleted a comment from azure-pipelinesBotAug 2, 2023
@jkotas
jkotas
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 2, 2023

Copy link


Copy Markdown

No commit pushedDate could be found for PR 88724 in repo dotnet/runtime
@jkotas
jkotas
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
jkotas
jkotas
approved these changes
Aug 2, 2023
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. @VSadov Could you please take a look as well?
@VSadov
VSadov
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
@VSadov Could you please take a look as well?
I will take a look.
VSadov
VSadov
reviewed
Aug 3, 2023
View reviewed changes
Comment threadsrc/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp
Outdated

VSadov
VSadov
approved these changes
Aug 3, 2023
View reviewed changes
@VSadovVSadov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. (just a couple nits). Thanks!!!
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 3, 2023

Closed


This was referenced Aug 3, 2023

Closed


Open


Closed


Closed


Closed


Closed


Closed

@VSadov
VSadov
commented
Aug 3, 2023

Copy link


Copy Markdown

Member
Thanks!
@VSadov
VSadov
merged commit 5bf45ce
into
dotnet:mainAug 3, 2023
@filipnavara
filipnavara
deleted the

compact-unwind

branch
August 3, 2023 15:01
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This is causing a lot of intermittent crashes on Linux arm64 . For example: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-fdbcd47e91a0464083/System.Collections.Concurrent.Tests/1/console.07e57ce1.log?helixlogtype=result
In checked builds, the crash is typically assertion failure in UnixNativeCodeManager::GetReturnAddressHijackInfo:
* thread #1, name = 'Microsoft.Exten', stop reason = signal SIGABRT
* frame #0: 0x0000ff5c2f85deac
frame #1: 0x0000ff5c2f84aaa0
frame #2: 0x0000aad8f9112abc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Assert(expr=<unavailable>, file=<unavailable>, line_num=<unavailable>, message=<unavailable>)
frame #3: 0x0000aad8f9170dd0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`UnixNativeCodeManager::GetReturnAddressHijackInfo(this=0x0000000000000001, pMethodInfo=0x0000ff5c2f99ac00, pRegisterSet=0x0000ff1b91ffb0d0, ppvRetAddrLocation=0x0000ff5c2f99a000, pRetValueKind=0x0000ff1b91ff78bc) at P:810:5
frame #4: 0x0000aad8f911dfb0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackReturnAddressWorker(this=0x0000ff1b91ffb7f8, frameIterator=0x0000ff1b91ff7918, pfnHijackFunction=(Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`RhpGcProbeHijack))())
frame #5: 0x0000aad8f911dbdc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(UNIX_CONTEXT*, void*) [inlined] Thread::HijackReturnAddress(this=0x0000ff1b91ffb7f8, pSuspendCtx=0x0000ff1b91ff7ca0)())
frame #6: 0x0000aad8f911db6c Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(pThreadContext=<unavailable>, pThreadToHijack=<unavailable>)
frame #7: 0x0000aad8f9167810 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`ActivationHandler(code=34, siginfo=0x0000ff1b91ff7c20, context=0x0000ff1b91ff7ca0)
frame #8: 0x0000ff5c2fabe5b0 linux-vdso.so.1
frame #9: 0x0000aad8f9508434 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`S_P_CoreLib_System_Collections_Generic_List_1<System___Canon>__Sort_2(this=<unavailable>, comparison=<unavailable>) at List.cs:1091
frame #10: 0x0000aad8f943c130 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Microsoft_Extensions_Configuration_Microsoft_Extensions_Configuration_ConfigurationProvider__GetChildKeys(this=<unavailable>, earlierKeys=<unavailable>, parentPath=<unavailable>) at ConfigurationProvider.cs:91

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "[NativeAOT] macOS/iOS: Emit simple compact unwinding informat…
549af5b
…ion (#88724)"
This reverts commit 5bf45ce.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Closed

@filipnavara
filipnavara
commented
Aug 4, 2023

Copy link


Copy Markdown

MemberAuthor
This is causing a lot of intermittent crashes on Linux arm64 .
Thanks for alert, I will check. I must have messed up some condition because it should have had no effect on Linux.
@filipnavara
filipnavara
commented
Aug 4, 2023

Copy link


Copy Markdown

MemberAuthor
I see the mistake now, will submit a fix shortly...
@filipnavarafilipnavara
mentioned this pull request
Aug 4, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@VSadovVSadovVSadov approved these changes
@ivanpovazanivanpovazanAwaiting requested review from ivanpovazan
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees


No one assigned


Labels

area-NativeAOT-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@filipnavara@jkotas@VSadov

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
[NativeAOT] macOS/iOS: Emit simple compact unwinding information by filipnavara · Pull Request #88724 · dotnet/runtime · GitHub
Skip to content
[NativeAOT] macOS/iOS: Emit simple compact unwinding information - #88724
Merged
VSadov merged 4 commits into
dotnet:mainfrom 
filipnavara:compact-unwind
Aug 3, 2023
Merged
[NativeAOT] macOS/iOS: Emit simple compact unwinding information#88724
VSadov merged 4 commits into
dotnet:mainfrom 
filipnavara:compact-unwind
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jul 12, 2023

Copy link


Copy Markdown

Member
Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.
This is minimum viable prototype that implements the following:
  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.
This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.
Contributes to #76371, #88292
@ghostghost
added

area-NativeAOT-coreclr

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 12, 2023
@filipnavara
filipnavara
requested a review
from VSadovJuly 12, 2023 10:19
@ghost
ghost
commented
Jul 12, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas

See info in area-owners.md if you want to be subscribed.
Issue Details
Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.
This is minimum viable prototype that implements the following:
  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.
This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.
Contributes to #76371, #88292
Author:filipnavara
Assignees:-
Labels:
area-NativeAOT-coreclr
Milestone:-
filipnavara
filipnavara
commented
Jul 12, 2023
View reviewed changes
Comment threadsrc/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectWriter.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 12, 2023

Closed

@filipnavara
filipnavara
marked this pull request as ready for review
July 14, 2023 06:26
@filipnavara
filipnavara
requested a review
from jkotasJuly 17, 2023 19:36
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
The general idea looks good to me. The size savings are very nice.
The only wrinkle is encoding of the prolog sizes. It looks odd that the prolog sizes are encoded explicitly, and the epilog sizes are inferred from the code. What would it take to infer the prolog size from the compact unwind info or by inspecting the code?
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
What would it take to infer the prolog size from the compact unwind info or by inspecting the code?
It should be only needed during GetReturnAddressHijackInfo. It should not be needed during regular unwind. We should never call general unwind in prolog or epilog.
@filipnavara
filipnavara
commented
Jul 18, 2023

Copy link


Copy Markdown

MemberAuthor
I originally inferred the prolog size from the instructions but it feels a bit fragile once you start doing complex prologs (not included in the PR). The compact unwind code by itself is not sufficient. It describes the stack layout but not which instructions were used to produce it. Due to various ways how ARM64 code can fold pointer adjustments into single instruction it would be fragile for the general case (again, not issue for this single unwind code that's produced in this PR).
If you feel strongly about inferring the prolog size from code I will do it for this simple case and we can extend it later to cover the hard cases.
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.
@filipnavara
filipnavara
commented
Aug 2, 2023

Copy link


Copy Markdown

MemberAuthor
We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.
Done. Let's see if the CI passes... (but it seemed to work locally and crossgen2 compiled the framework just fine)
@dotnetdotnet
deleted a comment from azure-pipelinesBotAug 2, 2023
@dotnetdotnet
deleted a comment from azure-pipelinesBotAug 2, 2023
@jkotas
jkotas
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 2, 2023

Copy link


Copy Markdown

No commit pushedDate could be found for PR 88724 in repo dotnet/runtime
@jkotas
jkotas
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
jkotas
jkotas
approved these changes
Aug 2, 2023
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. @VSadov Could you please take a look as well?
@VSadov
VSadov
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
@VSadov Could you please take a look as well?
I will take a look.
VSadov
VSadov
reviewed
Aug 3, 2023
View reviewed changes
Comment threadsrc/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp
Outdated

VSadov
VSadov
approved these changes
Aug 3, 2023
View reviewed changes
@VSadovVSadov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. (just a couple nits). Thanks!!!
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 3, 2023

Closed


This was referenced Aug 3, 2023

Closed


Open


Closed


Closed


Closed


Closed


Closed

@VSadov
VSadov
commented
Aug 3, 2023

Copy link


Copy Markdown

Member
Thanks!
@VSadov
VSadov
merged commit 5bf45ce
into
dotnet:mainAug 3, 2023
@filipnavara
filipnavara
deleted the

compact-unwind

branch
August 3, 2023 15:01
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This is causing a lot of intermittent crashes on Linux arm64 . For example: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-fdbcd47e91a0464083/System.Collections.Concurrent.Tests/1/console.07e57ce1.log?helixlogtype=result
In checked builds, the crash is typically assertion failure in UnixNativeCodeManager::GetReturnAddressHijackInfo:
* thread #1, name = 'Microsoft.Exten', stop reason = signal SIGABRT
* frame #0: 0x0000ff5c2f85deac
frame #1: 0x0000ff5c2f84aaa0
frame #2: 0x0000aad8f9112abc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Assert(expr=<unavailable>, file=<unavailable>, line_num=<unavailable>, message=<unavailable>)
frame #3: 0x0000aad8f9170dd0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`UnixNativeCodeManager::GetReturnAddressHijackInfo(this=0x0000000000000001, pMethodInfo=0x0000ff5c2f99ac00, pRegisterSet=0x0000ff1b91ffb0d0, ppvRetAddrLocation=0x0000ff5c2f99a000, pRetValueKind=0x0000ff1b91ff78bc) at P:810:5
frame #4: 0x0000aad8f911dfb0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackReturnAddressWorker(this=0x0000ff1b91ffb7f8, frameIterator=0x0000ff1b91ff7918, pfnHijackFunction=(Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`RhpGcProbeHijack))())
frame #5: 0x0000aad8f911dbdc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(UNIX_CONTEXT*, void*) [inlined] Thread::HijackReturnAddress(this=0x0000ff1b91ffb7f8, pSuspendCtx=0x0000ff1b91ff7ca0)())
frame #6: 0x0000aad8f911db6c Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(pThreadContext=<unavailable>, pThreadToHijack=<unavailable>)
frame #7: 0x0000aad8f9167810 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`ActivationHandler(code=34, siginfo=0x0000ff1b91ff7c20, context=0x0000ff1b91ff7ca0)
frame #8: 0x0000ff5c2fabe5b0 linux-vdso.so.1
frame #9: 0x0000aad8f9508434 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`S_P_CoreLib_System_Collections_Generic_List_1<System___Canon>__Sort_2(this=<unavailable>, comparison=<unavailable>) at List.cs:1091
frame #10: 0x0000aad8f943c130 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Microsoft_Extensions_Configuration_Microsoft_Extensions_Configuration_ConfigurationProvider__GetChildKeys(this=<unavailable>, earlierKeys=<unavailable>, parentPath=<unavailable>) at ConfigurationProvider.cs:91

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "[NativeAOT] macOS/iOS: Emit simple compact unwinding informat…
549af5b
…ion (#88724)"
This reverts commit 5bf45ce.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Closed

@filipnavara
filipnavara
commented
Aug 4, 2023

Copy link


Copy Markdown

MemberAuthor
This is causing a lot of intermittent crashes on Linux arm64 .
Thanks for alert, I will check. I must have messed up some condition because it should have had no effect on Linux.
@filipnavara
filipnavara
commented
Aug 4, 2023

Copy link


Copy Markdown

MemberAuthor
I see the mistake now, will submit a fix shortly...
@filipnavarafilipnavara
mentioned this pull request
Aug 4, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@VSadovVSadovVSadov approved these changes
@ivanpovazanivanpovazanAwaiting requested review from ivanpovazan
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees


No one assigned


Labels

area-NativeAOT-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@filipnavara@jkotas@VSadov

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
[NativeAOT] macOS/iOS: Emit simple compact unwinding information by filipnavara · Pull Request #88724 · dotnet/runtime · GitHub
Skip to content
[NativeAOT] macOS/iOS: Emit simple compact unwinding information - #88724
Merged
VSadov merged 4 commits into
dotnet:mainfrom 
filipnavara:compact-unwind
Aug 3, 2023
Merged
[NativeAOT] macOS/iOS: Emit simple compact unwinding information#88724
VSadov merged 4 commits into
dotnet:mainfrom 
filipnavara:compact-unwind
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jul 12, 2023

Copy link


Copy Markdown

Member
Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.
This is minimum viable prototype that implements the following:
  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.
This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.
Contributes to #76371, #88292
@ghostghost
added

area-NativeAOT-coreclr

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 12, 2023
@filipnavara
filipnavara
requested a review
from VSadovJuly 12, 2023 10:19
@ghost
ghost
commented
Jul 12, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas

See info in area-owners.md if you want to be subscribed.
Issue Details
Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.
This is minimum viable prototype that implements the following:
  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.
This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.
Contributes to #76371, #88292
Author:filipnavara
Assignees:-
Labels:
area-NativeAOT-coreclr
Milestone:-
filipnavara
filipnavara
commented
Jul 12, 2023
View reviewed changes
Comment threadsrc/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectWriter.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 12, 2023

Closed

@filipnavara
filipnavara
marked this pull request as ready for review
July 14, 2023 06:26
@filipnavara
filipnavara
requested a review
from jkotasJuly 17, 2023 19:36
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
The general idea looks good to me. The size savings are very nice.
The only wrinkle is encoding of the prolog sizes. It looks odd that the prolog sizes are encoded explicitly, and the epilog sizes are inferred from the code. What would it take to infer the prolog size from the compact unwind info or by inspecting the code?
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
What would it take to infer the prolog size from the compact unwind info or by inspecting the code?
It should be only needed during GetReturnAddressHijackInfo. It should not be needed during regular unwind. We should never call general unwind in prolog or epilog.
@filipnavara
filipnavara
commented
Jul 18, 2023

Copy link


Copy Markdown

MemberAuthor
I originally inferred the prolog size from the instructions but it feels a bit fragile once you start doing complex prologs (not included in the PR). The compact unwind code by itself is not sufficient. It describes the stack layout but not which instructions were used to produce it. Due to various ways how ARM64 code can fold pointer adjustments into single instruction it would be fragile for the general case (again, not issue for this single unwind code that's produced in this PR).
If you feel strongly about inferring the prolog size from code I will do it for this simple case and we can extend it later to cover the hard cases.
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.
@filipnavara
filipnavara
commented
Aug 2, 2023

Copy link


Copy Markdown

MemberAuthor
We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.
Done. Let's see if the CI passes... (but it seemed to work locally and crossgen2 compiled the framework just fine)
@dotnetdotnet
deleted a comment from azure-pipelinesBotAug 2, 2023
@dotnetdotnet
deleted a comment from azure-pipelinesBotAug 2, 2023
@jkotas
jkotas
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 2, 2023

Copy link


Copy Markdown

No commit pushedDate could be found for PR 88724 in repo dotnet/runtime
@jkotas
jkotas
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
jkotas
jkotas
approved these changes
Aug 2, 2023
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. @VSadov Could you please take a look as well?
@VSadov
VSadov
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
@VSadov Could you please take a look as well?
I will take a look.
VSadov
VSadov
reviewed
Aug 3, 2023
View reviewed changes
Comment threadsrc/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp
Outdated

VSadov
VSadov
approved these changes
Aug 3, 2023
View reviewed changes
@VSadovVSadov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. (just a couple nits). Thanks!!!
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 3, 2023

Closed


This was referenced Aug 3, 2023

Closed


Open


Closed


Closed


Closed


Closed


Closed

@VSadov
VSadov
commented
Aug 3, 2023

Copy link


Copy Markdown

Member
Thanks!
@VSadov
VSadov
merged commit 5bf45ce
into
dotnet:mainAug 3, 2023
@filipnavara
filipnavara
deleted the

compact-unwind

branch
August 3, 2023 15:01
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This is causing a lot of intermittent crashes on Linux arm64 . For example: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-fdbcd47e91a0464083/System.Collections.Concurrent.Tests/1/console.07e57ce1.log?helixlogtype=result
In checked builds, the crash is typically assertion failure in UnixNativeCodeManager::GetReturnAddressHijackInfo:
* thread #1, name = 'Microsoft.Exten', stop reason = signal SIGABRT
* frame #0: 0x0000ff5c2f85deac
frame #1: 0x0000ff5c2f84aaa0
frame #2: 0x0000aad8f9112abc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Assert(expr=<unavailable>, file=<unavailable>, line_num=<unavailable>, message=<unavailable>)
frame #3: 0x0000aad8f9170dd0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`UnixNativeCodeManager::GetReturnAddressHijackInfo(this=0x0000000000000001, pMethodInfo=0x0000ff5c2f99ac00, pRegisterSet=0x0000ff1b91ffb0d0, ppvRetAddrLocation=0x0000ff5c2f99a000, pRetValueKind=0x0000ff1b91ff78bc) at P:810:5
frame #4: 0x0000aad8f911dfb0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackReturnAddressWorker(this=0x0000ff1b91ffb7f8, frameIterator=0x0000ff1b91ff7918, pfnHijackFunction=(Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`RhpGcProbeHijack))())
frame #5: 0x0000aad8f911dbdc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(UNIX_CONTEXT*, void*) [inlined] Thread::HijackReturnAddress(this=0x0000ff1b91ffb7f8, pSuspendCtx=0x0000ff1b91ff7ca0)())
frame #6: 0x0000aad8f911db6c Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(pThreadContext=<unavailable>, pThreadToHijack=<unavailable>)
frame #7: 0x0000aad8f9167810 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`ActivationHandler(code=34, siginfo=0x0000ff1b91ff7c20, context=0x0000ff1b91ff7ca0)
frame #8: 0x0000ff5c2fabe5b0 linux-vdso.so.1
frame #9: 0x0000aad8f9508434 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`S_P_CoreLib_System_Collections_Generic_List_1<System___Canon>__Sort_2(this=<unavailable>, comparison=<unavailable>) at List.cs:1091
frame #10: 0x0000aad8f943c130 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Microsoft_Extensions_Configuration_Microsoft_Extensions_Configuration_ConfigurationProvider__GetChildKeys(this=<unavailable>, earlierKeys=<unavailable>, parentPath=<unavailable>) at ConfigurationProvider.cs:91

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "[NativeAOT] macOS/iOS: Emit simple compact unwinding informat…
549af5b
…ion (#88724)"
This reverts commit 5bf45ce.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Closed

@filipnavara
filipnavara
commented
Aug 4, 2023

Copy link


Copy Markdown

MemberAuthor
This is causing a lot of intermittent crashes on Linux arm64 .
Thanks for alert, I will check. I must have messed up some condition because it should have had no effect on Linux.
@filipnavara
filipnavara
commented
Aug 4, 2023

Copy link


Copy Markdown

MemberAuthor
I see the mistake now, will submit a fix shortly...
@filipnavarafilipnavara
mentioned this pull request
Aug 4, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@VSadovVSadovVSadov approved these changes
@ivanpovazanivanpovazanAwaiting requested review from ivanpovazan
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees


No one assigned


Labels

area-NativeAOT-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@filipnavara@jkotas@VSadov

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
[NativeAOT] macOS/iOS: Emit simple compact unwinding information by filipnavara · Pull Request #88724 · dotnet/runtime · GitHub
Skip to content
[NativeAOT] macOS/iOS: Emit simple compact unwinding information - #88724
Merged
VSadov merged 4 commits into
dotnet:mainfrom 
filipnavara:compact-unwind
Aug 3, 2023
Merged
[NativeAOT] macOS/iOS: Emit simple compact unwinding information#88724
VSadov merged 4 commits into
dotnet:mainfrom 
filipnavara:compact-unwind
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jul 12, 2023

Copy link


Copy Markdown

Member
Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.
This is minimum viable prototype that implements the following:
  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.
This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.
Contributes to #76371, #88292
@ghostghost
added

area-NativeAOT-coreclr

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 12, 2023
@filipnavara
filipnavara
requested a review
from VSadovJuly 12, 2023 10:19
@ghost
ghost
commented
Jul 12, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas

See info in area-owners.md if you want to be subscribed.
Issue Details
Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.
This is minimum viable prototype that implements the following:
  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.
This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.
Contributes to #76371, #88292
Author:filipnavara
Assignees:-
Labels:
area-NativeAOT-coreclr
Milestone:-
filipnavara
filipnavara
commented
Jul 12, 2023
View reviewed changes
Comment threadsrc/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectWriter.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 12, 2023

Closed

@filipnavara
filipnavara
marked this pull request as ready for review
July 14, 2023 06:26
@filipnavara
filipnavara
requested a review
from jkotasJuly 17, 2023 19:36
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
The general idea looks good to me. The size savings are very nice.
The only wrinkle is encoding of the prolog sizes. It looks odd that the prolog sizes are encoded explicitly, and the epilog sizes are inferred from the code. What would it take to infer the prolog size from the compact unwind info or by inspecting the code?
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
What would it take to infer the prolog size from the compact unwind info or by inspecting the code?
It should be only needed during GetReturnAddressHijackInfo. It should not be needed during regular unwind. We should never call general unwind in prolog or epilog.
@filipnavara
filipnavara
commented
Jul 18, 2023

Copy link


Copy Markdown

MemberAuthor
I originally inferred the prolog size from the instructions but it feels a bit fragile once you start doing complex prologs (not included in the PR). The compact unwind code by itself is not sufficient. It describes the stack layout but not which instructions were used to produce it. Due to various ways how ARM64 code can fold pointer adjustments into single instruction it would be fragile for the general case (again, not issue for this single unwind code that's produced in this PR).
If you feel strongly about inferring the prolog size from code I will do it for this simple case and we can extend it later to cover the hard cases.
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.
@filipnavara
filipnavara
commented
Aug 2, 2023

Copy link


Copy Markdown

MemberAuthor
We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.
Done. Let's see if the CI passes... (but it seemed to work locally and crossgen2 compiled the framework just fine)
@dotnetdotnet
deleted a comment from azure-pipelinesBotAug 2, 2023
@dotnetdotnet
deleted a comment from azure-pipelinesBotAug 2, 2023
@jkotas
jkotas
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 2, 2023

Copy link


Copy Markdown

No commit pushedDate could be found for PR 88724 in repo dotnet/runtime
@jkotas
jkotas
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
jkotas
jkotas
approved these changes
Aug 2, 2023
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. @VSadov Could you please take a look as well?
@VSadov
VSadov
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
@VSadov Could you please take a look as well?
I will take a look.
VSadov
VSadov
reviewed
Aug 3, 2023
View reviewed changes
Comment threadsrc/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp
Outdated

VSadov
VSadov
approved these changes
Aug 3, 2023
View reviewed changes
@VSadovVSadov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. (just a couple nits). Thanks!!!
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 3, 2023

Closed


This was referenced Aug 3, 2023

Closed


Open


Closed


Closed


Closed


Closed


Closed

@VSadov
VSadov
commented
Aug 3, 2023

Copy link


Copy Markdown

Member
Thanks!
@VSadov
VSadov
merged commit 5bf45ce
into
dotnet:mainAug 3, 2023
@filipnavara
filipnavara
deleted the

compact-unwind

branch
August 3, 2023 15:01
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This is causing a lot of intermittent crashes on Linux arm64 . For example: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-fdbcd47e91a0464083/System.Collections.Concurrent.Tests/1/console.07e57ce1.log?helixlogtype=result
In checked builds, the crash is typically assertion failure in UnixNativeCodeManager::GetReturnAddressHijackInfo:
* thread #1, name = 'Microsoft.Exten', stop reason = signal SIGABRT
* frame #0: 0x0000ff5c2f85deac
frame #1: 0x0000ff5c2f84aaa0
frame #2: 0x0000aad8f9112abc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Assert(expr=<unavailable>, file=<unavailable>, line_num=<unavailable>, message=<unavailable>)
frame #3: 0x0000aad8f9170dd0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`UnixNativeCodeManager::GetReturnAddressHijackInfo(this=0x0000000000000001, pMethodInfo=0x0000ff5c2f99ac00, pRegisterSet=0x0000ff1b91ffb0d0, ppvRetAddrLocation=0x0000ff5c2f99a000, pRetValueKind=0x0000ff1b91ff78bc) at P:810:5
frame #4: 0x0000aad8f911dfb0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackReturnAddressWorker(this=0x0000ff1b91ffb7f8, frameIterator=0x0000ff1b91ff7918, pfnHijackFunction=(Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`RhpGcProbeHijack))())
frame #5: 0x0000aad8f911dbdc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(UNIX_CONTEXT*, void*) [inlined] Thread::HijackReturnAddress(this=0x0000ff1b91ffb7f8, pSuspendCtx=0x0000ff1b91ff7ca0)())
frame #6: 0x0000aad8f911db6c Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(pThreadContext=<unavailable>, pThreadToHijack=<unavailable>)
frame #7: 0x0000aad8f9167810 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`ActivationHandler(code=34, siginfo=0x0000ff1b91ff7c20, context=0x0000ff1b91ff7ca0)
frame #8: 0x0000ff5c2fabe5b0 linux-vdso.so.1
frame #9: 0x0000aad8f9508434 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`S_P_CoreLib_System_Collections_Generic_List_1<System___Canon>__Sort_2(this=<unavailable>, comparison=<unavailable>) at List.cs:1091
frame #10: 0x0000aad8f943c130 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Microsoft_Extensions_Configuration_Microsoft_Extensions_Configuration_ConfigurationProvider__GetChildKeys(this=<unavailable>, earlierKeys=<unavailable>, parentPath=<unavailable>) at ConfigurationProvider.cs:91

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "[NativeAOT] macOS/iOS: Emit simple compact unwinding informat…
549af5b
…ion (#88724)"
This reverts commit 5bf45ce.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Closed

@filipnavara
filipnavara
commented
Aug 4, 2023

Copy link


Copy Markdown

MemberAuthor
This is causing a lot of intermittent crashes on Linux arm64 .
Thanks for alert, I will check. I must have messed up some condition because it should have had no effect on Linux.
@filipnavara
filipnavara
commented
Aug 4, 2023

Copy link


Copy Markdown

MemberAuthor
I see the mistake now, will submit a fix shortly...
@filipnavarafilipnavara
mentioned this pull request
Aug 4, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@VSadovVSadovVSadov approved these changes
@ivanpovazanivanpovazanAwaiting requested review from ivanpovazan
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees


No one assigned


Labels

area-NativeAOT-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@filipnavara@jkotas@VSadov

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
})();
[NativeAOT] macOS/iOS: Emit simple compact unwinding information by filipnavara · Pull Request #88724 · dotnet/runtime · GitHub
Skip to content
[NativeAOT] macOS/iOS: Emit simple compact unwinding information - #88724
Merged
VSadov merged 4 commits into
dotnet:mainfrom 
filipnavara:compact-unwind
Aug 3, 2023
Merged
[NativeAOT] macOS/iOS: Emit simple compact unwinding information#88724
VSadov merged 4 commits into
dotnet:mainfrom 
filipnavara:compact-unwind
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jul 12, 2023

Copy link


Copy Markdown

Member
Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.
This is minimum viable prototype that implements the following:
  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.
This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.
Contributes to #76371, #88292
@ghostghost
added

area-NativeAOT-coreclr

community-contribution
Indicates that the PR has been added by a community member
labels
Jul 12, 2023
@filipnavara
filipnavara
requested a review
from VSadovJuly 12, 2023 10:19
@ghost
ghost
commented
Jul 12, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas

See info in area-owners.md if you want to be subscribed.
Issue Details
Apple platforms can represent certain unwinding sequences more efficiently using a single 32-bit "compact unwinding" code instead of a DWARF EH frame. This format cannot accurately unwind prologs and epilogs. However, we already handle epilogs manually since JIT doesn't generate the DWARF code for it, and prologs could be handled in a similar manner.
This is minimum viable prototype that implements the following:
  1. Emits a single specific ARM64 unwind code for an empty frame prolog. Any other prolog would need changes in JIT to produce compatible prologs (eg. point frame pointer to the top of the frame, not bottom, store registers in correct order, always store registers in pairs).
  2. Emit a prolog length into LSDA block if DWARF information is omitted. This allows the runtime to quickly identify if we are in the prolog or not.
This empty frame prolog accounts for about 30% of the DWARF code / methods in a typical executable, so it has significant impact even without handing the more complex cases.
Contributes to #76371, #88292
Author:filipnavara
Assignees:-
Labels:
area-NativeAOT-coreclr
Milestone:-
filipnavara
filipnavara
commented
Jul 12, 2023
View reviewed changes
Comment threadsrc/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectWriter.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 12, 2023

Closed

@filipnavara
filipnavara
marked this pull request as ready for review
July 14, 2023 06:26
@filipnavara
filipnavara
requested a review
from jkotasJuly 17, 2023 19:36
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
The general idea looks good to me. The size savings are very nice.
The only wrinkle is encoding of the prolog sizes. It looks odd that the prolog sizes are encoded explicitly, and the epilog sizes are inferred from the code. What would it take to infer the prolog size from the compact unwind info or by inspecting the code?
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
What would it take to infer the prolog size from the compact unwind info or by inspecting the code?
It should be only needed during GetReturnAddressHijackInfo. It should not be needed during regular unwind. We should never call general unwind in prolog or epilog.
@filipnavara
filipnavara
commented
Jul 18, 2023

Copy link


Copy Markdown

MemberAuthor
I originally inferred the prolog size from the instructions but it feels a bit fragile once you start doing complex prologs (not included in the PR). The compact unwind code by itself is not sufficient. It describes the stack layout but not which instructions were used to produce it. Due to various ways how ARM64 code can fold pointer adjustments into single instruction it would be fragile for the general case (again, not issue for this single unwind code that's produced in this PR).
If you feel strongly about inferring the prolog size from code I will do it for this simple case and we can extend it later to cover the hard cases.
@jkotas
jkotas
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.
@filipnavara
filipnavara
commented
Aug 2, 2023

Copy link


Copy Markdown

MemberAuthor
We took on this fragility in epilogs, so I think it would be fine to take on it in prologs too and save a bit more size.
Done. Let's see if the CI passes... (but it seemed to work locally and crossgen2 compiled the framework just fine)
@dotnetdotnet
deleted a comment from azure-pipelinesBotAug 2, 2023
@dotnetdotnet
deleted a comment from azure-pipelinesBotAug 2, 2023
@jkotas
jkotas
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 2, 2023

Copy link


Copy Markdown

No commit pushedDate could be found for PR 88724 in repo dotnet/runtime
@jkotas
jkotas
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 2, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
jkotas
jkotas
approved these changes
Aug 2, 2023
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. @VSadov Could you please take a look as well?
@VSadov
VSadov
commented
Aug 2, 2023

Copy link


Copy Markdown

Member
@VSadov Could you please take a look as well?
I will take a look.
VSadov
VSadov
reviewed
Aug 3, 2023
View reviewed changes
Comment threadsrc/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp
Outdated

VSadov
VSadov
approved these changes
Aug 3, 2023
View reviewed changes
@VSadovVSadov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. (just a couple nits). Thanks!!!
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 3, 2023

Closed


This was referenced Aug 3, 2023

Closed


Open


Closed


Closed


Closed


Closed


Closed

@VSadov
VSadov
commented
Aug 3, 2023

Copy link


Copy Markdown

Member
Thanks!
@VSadov
VSadov
merged commit 5bf45ce
into
dotnet:mainAug 3, 2023
@filipnavara
filipnavara
deleted the

compact-unwind

branch
August 3, 2023 15:01
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This is causing a lot of intermittent crashes on Linux arm64 . For example: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-fdbcd47e91a0464083/System.Collections.Concurrent.Tests/1/console.07e57ce1.log?helixlogtype=result
In checked builds, the crash is typically assertion failure in UnixNativeCodeManager::GetReturnAddressHijackInfo:
* thread #1, name = 'Microsoft.Exten', stop reason = signal SIGABRT
* frame #0: 0x0000ff5c2f85deac
frame #1: 0x0000ff5c2f84aaa0
frame #2: 0x0000aad8f9112abc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Assert(expr=<unavailable>, file=<unavailable>, line_num=<unavailable>, message=<unavailable>)
frame #3: 0x0000aad8f9170dd0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`UnixNativeCodeManager::GetReturnAddressHijackInfo(this=0x0000000000000001, pMethodInfo=0x0000ff5c2f99ac00, pRegisterSet=0x0000ff1b91ffb0d0, ppvRetAddrLocation=0x0000ff5c2f99a000, pRetValueKind=0x0000ff1b91ff78bc) at P:810:5
frame #4: 0x0000aad8f911dfb0 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackReturnAddressWorker(this=0x0000ff1b91ffb7f8, frameIterator=0x0000ff1b91ff7918, pfnHijackFunction=(Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`RhpGcProbeHijack))())
frame #5: 0x0000aad8f911dbdc Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(UNIX_CONTEXT*, void*) [inlined] Thread::HijackReturnAddress(this=0x0000ff1b91ffb7f8, pSuspendCtx=0x0000ff1b91ff7ca0)())
frame #6: 0x0000aad8f911db6c Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Thread::HijackCallback(pThreadContext=<unavailable>, pThreadToHijack=<unavailable>)
frame #7: 0x0000aad8f9167810 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`ActivationHandler(code=34, siginfo=0x0000ff1b91ff7c20, context=0x0000ff1b91ff7ca0)
frame #8: 0x0000ff5c2fabe5b0 linux-vdso.so.1
frame #9: 0x0000aad8f9508434 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`S_P_CoreLib_System_Collections_Generic_List_1<System___Canon>__Sort_2(this=<unavailable>, comparison=<unavailable>) at List.cs:1091
frame #10: 0x0000aad8f943c130 Microsoft.Extensions.Configuration.EnvironmentVariables.Tests`Microsoft_Extensions_Configuration_Microsoft_Extensions_Configuration_ConfigurationProvider__GetChildKeys(this=<unavailable>, earlierKeys=<unavailable>, parentPath=<unavailable>) at ConfigurationProvider.cs:91

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "[NativeAOT] macOS/iOS: Emit simple compact unwinding informat…
549af5b
…ion (#88724)"
This reverts commit 5bf45ce.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Closed

@filipnavara
filipnavara
commented
Aug 4, 2023

Copy link


Copy Markdown

MemberAuthor
This is causing a lot of intermittent crashes on Linux arm64 .
Thanks for alert, I will check. I must have messed up some condition because it should have had no effect on Linux.
@filipnavara
filipnavara
commented
Aug 4, 2023

Copy link


Copy Markdown

MemberAuthor
I see the mistake now, will submit a fix shortly...
@filipnavarafilipnavara
mentioned this pull request
Aug 4, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@VSadovVSadovVSadov approved these changes
@ivanpovazanivanpovazanAwaiting requested review from ivanpovazan
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees


No one assigned


Labels

area-NativeAOT-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@filipnavara@jkotas@VSadov