Skip to content

Fix x86 EH for UnmanagedCallersOnly methods - #58796

Merged
janvorli merged 2 commits into
dotnet:mainfrom
janvorli:fix-x86-eh-unmanagedcallers
Sep 9, 2021
Merged

Fix x86 EH for UnmanagedCallersOnly methods#58796
janvorli merged 2 commits into
dotnet:mainfrom
janvorli:fix-x86-eh-unmanagedcallers

Conversation

@janvorli

@janvorlijanvorli commented Sep 8, 2021•
edited by AaronRobinsonMSFT
Loading

Copy link
Copy Markdown
Member

There is a bug in the exception handling on x86 when exception is
propagated from a method marked with UnmanagedCallersOnly attribute to
an immediate caller that's also managed (and calls the method via a
native delegate). In this case, there is no native frame in between and
the stack walker accidentally skips the InlinedCallFrame processing
during first pass of exception handling. The InlinedCallFrame is expected
to be processed before the related PInvoke IL stub and it doesn't happen
in this case, as a native frame is required to make the stack frame iterator
move to the explicit InlinedCallFrame. The fact that it is not processed
results in ignoring the upper limit on the stack walk and walking the stack
further than expected.

This change fixes it by detecting the direct transition from
UnmanagedCallersOnly to managed caller and forces the stack frame
iterator to process the InlinedCallFrame.

Fixes#57915

@janvorli
Fix x86 EH for UnmanagedCallersOnly methods
a943ad5
There is a bug in the exception handling on x86 when exception is
propagated from a method marked with UnmanagedCallersOnly attribute to
an immediate caller that's also managed (and calls the method via a
native delegate). In this case, there is no native frame in between and
the stack walker accidentally skips the InlinedCallFrame processing
during first pass of exception handling. The InlinedCallFrame is expected
to be processed before the related PInvoke IL stub and it doesn't happen
in this case, as a native frame is required to make the stack frame iterator
move to the explicit InlinedCallFrame. The fact that it is not processed
results in ignoring the upper limit on the stack walk and walking the stack
further than expected.
This change fixes it by detecting the direct transition from
UnmanagedCallersOnly to managed caller and forces the stack frame
iterator to process the InlinedCallFrame.
@ghost

ghost commented Sep 8, 2021

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@janvorli
janvorli requested a review from jkotasSeptember 8, 2021 12:04
@janvorlijanvorli self-assigned this Sep 8, 2021
@janvorlijanvorli added the area-ExceptionHandling-coreclr only use for closed issues label Sep 8, 2021
@janvorlijanvorli added this to the 7.0.0 milestone Sep 8, 2021
@janvorli

janvorli commented Sep 8, 2021

Copy link
Copy Markdown
MemberAuthor

cc: @AaronRobinsonMSFT

jkotas
jkotas reviewed Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/methodtablebuilder.cpp Outdated
AaronRobinsonMSFT
AaronRobinsonMSFT reviewed Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/ilstubcache.cpp Outdated
@janvorli
janvorliforce-pushed the fix-x86-eh-unmanagedcallers branch from 62f65f1 to 36861eeCompareSeptember 8, 2021 19:37
jkotas
jkotas reviewed Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/stackwalk.cpp

#ifdef TARGET_X86
hdrInfo gcHdrInfo;
DecodeGCHdrInfo(m_crawl.codeInfo.GetGCInfoToken(), 0, &gcHdrInfo);

@jkotasjkotasSep 8, 2021

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.

All other places that decode GCInfo in this file go through m_crawl.GetCodeManager(). Is it worth following the pattern?

@janvorlijanvorliSep 8, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Good idea, but let me do that as a separate cleanup so that this change is just a fix for the bug. I will need to add a new method to the EECodeManager and then I'd use it in the exceptionhandling.cpp too where we currently also have this raw decoding.

jkotas
jkotas approved these changes Sep 8, 2021
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

@janvorli
janvorli merged commit 7754114 into dotnet:mainSep 9, 2021
@janvorli
janvorli deleted the fix-x86-eh-unmanagedcallers branch September 9, 2021 08:54
@ghostghost locked as resolved and limited conversation to collaborators Oct 9, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas approved these changes
@AaronRobinsonMSFTAaronRobinsonMSFTAaronRobinsonMSFT approved these changes

Assignees

@janvorlijanvorli

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

[JitStressWithRandomNumbers] UnmanagedCallersOnlyTest fails.

3 participants

@janvorli@jkotas@AaronRobinsonMSFT
, '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" + '
Fix x86 EH for UnmanagedCallersOnly methods by janvorli · Pull Request #58796 · dotnet/runtime · GitHub
Skip to content
Fix x86 EH for UnmanagedCallersOnly methods - #58796
Merged
janvorli merged 2 commits into
dotnet:mainfrom 
janvorli:fix-x86-eh-unmanagedcallers
Sep 9, 2021
Merged
Fix x86 EH for UnmanagedCallersOnly methods#58796
janvorli merged 2 commits into
dotnet:mainfrom 
janvorli:fix-x86-eh-unmanagedcallers
Conversation
@janvorli
@janvorlijanvorli
commented
Sep 8, 2021•

edited by AaronRobinsonMSFT

Loading

Copy link


Copy Markdown

Member
There is a bug in the exception handling on x86 when exception is

propagated from a method marked with UnmanagedCallersOnly attribute to

an immediate caller that's also managed (and calls the method via a

native delegate). In this case, there is no native frame in between and

the stack walker accidentally skips the InlinedCallFrame processing

during first pass of exception handling. The InlinedCallFrame is expected

to be processed before the related PInvoke IL stub and it doesn't happen

in this case, as a native frame is required to make the stack frame iterator

move to the explicit InlinedCallFrame. The fact that it is not processed

results in ignoring the upper limit on the stack walk and walking the stack

further than expected.
This change fixes it by detecting the direct transition from

UnmanagedCallersOnly to managed caller and forces the stack frame

iterator to process the InlinedCallFrame.
Fixes#57915
@janvorli
Fix x86 EH for UnmanagedCallersOnly methods
a943ad5
There is a bug in the exception handling on x86 when exception is
propagated from a method marked with UnmanagedCallersOnly attribute to
an immediate caller that's also managed (and calls the method via a
native delegate). In this case, there is no native frame in between and
the stack walker accidentally skips the InlinedCallFrame processing
during first pass of exception handling. The InlinedCallFrame is expected
to be processed before the related PInvoke IL stub and it doesn't happen
in this case, as a native frame is required to make the stack frame iterator
move to the explicit InlinedCallFrame. The fact that it is not processed
results in ignoring the upper limit on the stack walk and walking the stack
further than expected.
This change fixes it by detecting the direct transition from
UnmanagedCallersOnly to managed caller and forces the stack frame
iterator to process the InlinedCallFrame.
@ghost
ghost
commented
Sep 8, 2021

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@janvorli
janvorli
requested a review
from jkotasSeptember 8, 2021 12:04
@janvorlijanvorli
self-assigned this
Sep 8, 2021
@janvorlijanvorli
added
the 
area-ExceptionHandling-coreclr
only use for closed issues
label
Sep 8, 2021
@janvorlijanvorli
added this to the 7.0.0 milestone
Sep 8, 2021
@janvorli
janvorli
commented
Sep 8, 2021

Copy link


Copy Markdown

MemberAuthor
cc: @AaronRobinsonMSFT
jkotas
jkotas
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/methodtablebuilder.cpp
Outdated

AaronRobinsonMSFT
AaronRobinsonMSFT
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/ilstubcache.cpp
Outdated

@janvorli
janvorliforce-pushed
the
fix-x86-eh-unmanagedcallers
branch
from
62f65f1 to
36861eeCompareSeptember 8, 2021 19:37
jkotas
jkotas
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/stackwalk.cpp

#ifdef TARGET_X86
 hdrInfo gcHdrInfo;
 DecodeGCHdrInfo(m_crawl.codeInfo.GetGCInfoToken(), 0, &gcHdrInfo);
@jkotasjkotasSep 8, 2021

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.

All other places that decode GCInfo in this file go through m_crawl.GetCodeManager(). Is it worth following the pattern?
@janvorlijanvorliSep 8, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good idea, but let me do that as a separate cleanup so that this change is just a fix for the bug. I will need to add a new method to the EECodeManager and then I'd use it in the exceptionhandling.cpp too where we currently also have this raw decoding.
jkotas
jkotas
approved these changes
Sep 8, 2021
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
@janvorli
janvorli
merged commit 7754114
into
dotnet:mainSep 9, 2021
@janvorli
janvorli
deleted the

fix-x86-eh-unmanagedcallers

branch
September 9, 2021 08:54
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 9, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@AaronRobinsonMSFTAaronRobinsonMSFTAaronRobinsonMSFT approved these changes

Assignees

@janvorlijanvorli

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

[JitStressWithRandomNumbers] UnmanagedCallersOnlyTest fails.


3 participants

@janvorli@jkotas@AaronRobinsonMSFT

, '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('^' + ".*" + '
Fix x86 EH for UnmanagedCallersOnly methods by janvorli · Pull Request #58796 · dotnet/runtime · GitHub
Skip to content
Fix x86 EH for UnmanagedCallersOnly methods - #58796
Merged
janvorli merged 2 commits into
dotnet:mainfrom 
janvorli:fix-x86-eh-unmanagedcallers
Sep 9, 2021
Merged
Fix x86 EH for UnmanagedCallersOnly methods#58796
janvorli merged 2 commits into
dotnet:mainfrom 
janvorli:fix-x86-eh-unmanagedcallers
Conversation
@janvorli
@janvorlijanvorli
commented
Sep 8, 2021•

edited by AaronRobinsonMSFT

Loading

Copy link


Copy Markdown

Member
There is a bug in the exception handling on x86 when exception is

propagated from a method marked with UnmanagedCallersOnly attribute to

an immediate caller that's also managed (and calls the method via a

native delegate). In this case, there is no native frame in between and

the stack walker accidentally skips the InlinedCallFrame processing

during first pass of exception handling. The InlinedCallFrame is expected

to be processed before the related PInvoke IL stub and it doesn't happen

in this case, as a native frame is required to make the stack frame iterator

move to the explicit InlinedCallFrame. The fact that it is not processed

results in ignoring the upper limit on the stack walk and walking the stack

further than expected.
This change fixes it by detecting the direct transition from

UnmanagedCallersOnly to managed caller and forces the stack frame

iterator to process the InlinedCallFrame.
Fixes#57915
@janvorli
Fix x86 EH for UnmanagedCallersOnly methods
a943ad5
There is a bug in the exception handling on x86 when exception is
propagated from a method marked with UnmanagedCallersOnly attribute to
an immediate caller that's also managed (and calls the method via a
native delegate). In this case, there is no native frame in between and
the stack walker accidentally skips the InlinedCallFrame processing
during first pass of exception handling. The InlinedCallFrame is expected
to be processed before the related PInvoke IL stub and it doesn't happen
in this case, as a native frame is required to make the stack frame iterator
move to the explicit InlinedCallFrame. The fact that it is not processed
results in ignoring the upper limit on the stack walk and walking the stack
further than expected.
This change fixes it by detecting the direct transition from
UnmanagedCallersOnly to managed caller and forces the stack frame
iterator to process the InlinedCallFrame.
@ghost
ghost
commented
Sep 8, 2021

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@janvorli
janvorli
requested a review
from jkotasSeptember 8, 2021 12:04
@janvorlijanvorli
self-assigned this
Sep 8, 2021
@janvorlijanvorli
added
the 
area-ExceptionHandling-coreclr
only use for closed issues
label
Sep 8, 2021
@janvorlijanvorli
added this to the 7.0.0 milestone
Sep 8, 2021
@janvorli
janvorli
commented
Sep 8, 2021

Copy link


Copy Markdown

MemberAuthor
cc: @AaronRobinsonMSFT
jkotas
jkotas
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/methodtablebuilder.cpp
Outdated

AaronRobinsonMSFT
AaronRobinsonMSFT
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/ilstubcache.cpp
Outdated

@janvorli
janvorliforce-pushed
the
fix-x86-eh-unmanagedcallers
branch
from
62f65f1 to
36861eeCompareSeptember 8, 2021 19:37
jkotas
jkotas
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/stackwalk.cpp

#ifdef TARGET_X86
 hdrInfo gcHdrInfo;
 DecodeGCHdrInfo(m_crawl.codeInfo.GetGCInfoToken(), 0, &gcHdrInfo);
@jkotasjkotasSep 8, 2021

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.

All other places that decode GCInfo in this file go through m_crawl.GetCodeManager(). Is it worth following the pattern?
@janvorlijanvorliSep 8, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good idea, but let me do that as a separate cleanup so that this change is just a fix for the bug. I will need to add a new method to the EECodeManager and then I'd use it in the exceptionhandling.cpp too where we currently also have this raw decoding.
jkotas
jkotas
approved these changes
Sep 8, 2021
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
@janvorli
janvorli
merged commit 7754114
into
dotnet:mainSep 9, 2021
@janvorli
janvorli
deleted the

fix-x86-eh-unmanagedcallers

branch
September 9, 2021 08:54
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 9, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@AaronRobinsonMSFTAaronRobinsonMSFTAaronRobinsonMSFT approved these changes

Assignees

@janvorlijanvorli

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

[JitStressWithRandomNumbers] UnmanagedCallersOnlyTest fails.


3 participants

@janvorli@jkotas@AaronRobinsonMSFT

, '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('^' + ".*" + '
Fix x86 EH for UnmanagedCallersOnly methods by janvorli · Pull Request #58796 · dotnet/runtime · GitHub
Skip to content
Fix x86 EH for UnmanagedCallersOnly methods - #58796
Merged
janvorli merged 2 commits into
dotnet:mainfrom 
janvorli:fix-x86-eh-unmanagedcallers
Sep 9, 2021
Merged
Fix x86 EH for UnmanagedCallersOnly methods#58796
janvorli merged 2 commits into
dotnet:mainfrom 
janvorli:fix-x86-eh-unmanagedcallers
Conversation
@janvorli
@janvorlijanvorli
commented
Sep 8, 2021•

edited by AaronRobinsonMSFT

Loading

Copy link


Copy Markdown

Member
There is a bug in the exception handling on x86 when exception is

propagated from a method marked with UnmanagedCallersOnly attribute to

an immediate caller that's also managed (and calls the method via a

native delegate). In this case, there is no native frame in between and

the stack walker accidentally skips the InlinedCallFrame processing

during first pass of exception handling. The InlinedCallFrame is expected

to be processed before the related PInvoke IL stub and it doesn't happen

in this case, as a native frame is required to make the stack frame iterator

move to the explicit InlinedCallFrame. The fact that it is not processed

results in ignoring the upper limit on the stack walk and walking the stack

further than expected.
This change fixes it by detecting the direct transition from

UnmanagedCallersOnly to managed caller and forces the stack frame

iterator to process the InlinedCallFrame.
Fixes#57915
@janvorli
Fix x86 EH for UnmanagedCallersOnly methods
a943ad5
There is a bug in the exception handling on x86 when exception is
propagated from a method marked with UnmanagedCallersOnly attribute to
an immediate caller that's also managed (and calls the method via a
native delegate). In this case, there is no native frame in between and
the stack walker accidentally skips the InlinedCallFrame processing
during first pass of exception handling. The InlinedCallFrame is expected
to be processed before the related PInvoke IL stub and it doesn't happen
in this case, as a native frame is required to make the stack frame iterator
move to the explicit InlinedCallFrame. The fact that it is not processed
results in ignoring the upper limit on the stack walk and walking the stack
further than expected.
This change fixes it by detecting the direct transition from
UnmanagedCallersOnly to managed caller and forces the stack frame
iterator to process the InlinedCallFrame.
@ghost
ghost
commented
Sep 8, 2021

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@janvorli
janvorli
requested a review
from jkotasSeptember 8, 2021 12:04
@janvorlijanvorli
self-assigned this
Sep 8, 2021
@janvorlijanvorli
added
the 
area-ExceptionHandling-coreclr
only use for closed issues
label
Sep 8, 2021
@janvorlijanvorli
added this to the 7.0.0 milestone
Sep 8, 2021
@janvorli
janvorli
commented
Sep 8, 2021

Copy link


Copy Markdown

MemberAuthor
cc: @AaronRobinsonMSFT
jkotas
jkotas
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/methodtablebuilder.cpp
Outdated

AaronRobinsonMSFT
AaronRobinsonMSFT
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/ilstubcache.cpp
Outdated

@janvorli
janvorliforce-pushed
the
fix-x86-eh-unmanagedcallers
branch
from
62f65f1 to
36861eeCompareSeptember 8, 2021 19:37
jkotas
jkotas
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/stackwalk.cpp

#ifdef TARGET_X86
 hdrInfo gcHdrInfo;
 DecodeGCHdrInfo(m_crawl.codeInfo.GetGCInfoToken(), 0, &gcHdrInfo);
@jkotasjkotasSep 8, 2021

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.

All other places that decode GCInfo in this file go through m_crawl.GetCodeManager(). Is it worth following the pattern?
@janvorlijanvorliSep 8, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good idea, but let me do that as a separate cleanup so that this change is just a fix for the bug. I will need to add a new method to the EECodeManager and then I'd use it in the exceptionhandling.cpp too where we currently also have this raw decoding.
jkotas
jkotas
approved these changes
Sep 8, 2021
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
@janvorli
janvorli
merged commit 7754114
into
dotnet:mainSep 9, 2021
@janvorli
janvorli
deleted the

fix-x86-eh-unmanagedcallers

branch
September 9, 2021 08:54
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 9, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@AaronRobinsonMSFTAaronRobinsonMSFTAaronRobinsonMSFT approved these changes

Assignees

@janvorlijanvorli

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

[JitStressWithRandomNumbers] UnmanagedCallersOnlyTest fails.


3 participants

@janvorli@jkotas@AaronRobinsonMSFT

, '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" + '
Fix x86 EH for UnmanagedCallersOnly methods by janvorli · Pull Request #58796 · dotnet/runtime · GitHub
Skip to content
Fix x86 EH for UnmanagedCallersOnly methods - #58796
Merged
janvorli merged 2 commits into
dotnet:mainfrom 
janvorli:fix-x86-eh-unmanagedcallers
Sep 9, 2021
Merged
Fix x86 EH for UnmanagedCallersOnly methods#58796
janvorli merged 2 commits into
dotnet:mainfrom 
janvorli:fix-x86-eh-unmanagedcallers
Conversation
@janvorli
@janvorlijanvorli
commented
Sep 8, 2021•

edited by AaronRobinsonMSFT

Loading

Copy link


Copy Markdown

Member
There is a bug in the exception handling on x86 when exception is

propagated from a method marked with UnmanagedCallersOnly attribute to

an immediate caller that's also managed (and calls the method via a

native delegate). In this case, there is no native frame in between and

the stack walker accidentally skips the InlinedCallFrame processing

during first pass of exception handling. The InlinedCallFrame is expected

to be processed before the related PInvoke IL stub and it doesn't happen

in this case, as a native frame is required to make the stack frame iterator

move to the explicit InlinedCallFrame. The fact that it is not processed

results in ignoring the upper limit on the stack walk and walking the stack

further than expected.
This change fixes it by detecting the direct transition from

UnmanagedCallersOnly to managed caller and forces the stack frame

iterator to process the InlinedCallFrame.
Fixes#57915
@janvorli
Fix x86 EH for UnmanagedCallersOnly methods
a943ad5
There is a bug in the exception handling on x86 when exception is
propagated from a method marked with UnmanagedCallersOnly attribute to
an immediate caller that's also managed (and calls the method via a
native delegate). In this case, there is no native frame in between and
the stack walker accidentally skips the InlinedCallFrame processing
during first pass of exception handling. The InlinedCallFrame is expected
to be processed before the related PInvoke IL stub and it doesn't happen
in this case, as a native frame is required to make the stack frame iterator
move to the explicit InlinedCallFrame. The fact that it is not processed
results in ignoring the upper limit on the stack walk and walking the stack
further than expected.
This change fixes it by detecting the direct transition from
UnmanagedCallersOnly to managed caller and forces the stack frame
iterator to process the InlinedCallFrame.
@ghost
ghost
commented
Sep 8, 2021

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@janvorli
janvorli
requested a review
from jkotasSeptember 8, 2021 12:04
@janvorlijanvorli
self-assigned this
Sep 8, 2021
@janvorlijanvorli
added
the 
area-ExceptionHandling-coreclr
only use for closed issues
label
Sep 8, 2021
@janvorlijanvorli
added this to the 7.0.0 milestone
Sep 8, 2021
@janvorli
janvorli
commented
Sep 8, 2021

Copy link


Copy Markdown

MemberAuthor
cc: @AaronRobinsonMSFT
jkotas
jkotas
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/methodtablebuilder.cpp
Outdated

AaronRobinsonMSFT
AaronRobinsonMSFT
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/ilstubcache.cpp
Outdated

@janvorli
janvorliforce-pushed
the
fix-x86-eh-unmanagedcallers
branch
from
62f65f1 to
36861eeCompareSeptember 8, 2021 19:37
jkotas
jkotas
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/stackwalk.cpp

#ifdef TARGET_X86
 hdrInfo gcHdrInfo;
 DecodeGCHdrInfo(m_crawl.codeInfo.GetGCInfoToken(), 0, &gcHdrInfo);
@jkotasjkotasSep 8, 2021

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.

All other places that decode GCInfo in this file go through m_crawl.GetCodeManager(). Is it worth following the pattern?
@janvorlijanvorliSep 8, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good idea, but let me do that as a separate cleanup so that this change is just a fix for the bug. I will need to add a new method to the EECodeManager and then I'd use it in the exceptionhandling.cpp too where we currently also have this raw decoding.
jkotas
jkotas
approved these changes
Sep 8, 2021
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
@janvorli
janvorli
merged commit 7754114
into
dotnet:mainSep 9, 2021
@janvorli
janvorli
deleted the

fix-x86-eh-unmanagedcallers

branch
September 9, 2021 08:54
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 9, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@AaronRobinsonMSFTAaronRobinsonMSFTAaronRobinsonMSFT approved these changes

Assignees

@janvorlijanvorli

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

[JitStressWithRandomNumbers] UnmanagedCallersOnlyTest fails.


3 participants

@janvorli@jkotas@AaronRobinsonMSFT

, '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('^' + ".*" + '
Fix x86 EH for UnmanagedCallersOnly methods by janvorli · Pull Request #58796 · dotnet/runtime · GitHub
Skip to content
Fix x86 EH for UnmanagedCallersOnly methods - #58796
Merged
janvorli merged 2 commits into
dotnet:mainfrom 
janvorli:fix-x86-eh-unmanagedcallers
Sep 9, 2021
Merged
Fix x86 EH for UnmanagedCallersOnly methods#58796
janvorli merged 2 commits into
dotnet:mainfrom 
janvorli:fix-x86-eh-unmanagedcallers
Conversation
@janvorli
@janvorlijanvorli
commented
Sep 8, 2021•

edited by AaronRobinsonMSFT

Loading

Copy link


Copy Markdown

Member
There is a bug in the exception handling on x86 when exception is

propagated from a method marked with UnmanagedCallersOnly attribute to

an immediate caller that's also managed (and calls the method via a

native delegate). In this case, there is no native frame in between and

the stack walker accidentally skips the InlinedCallFrame processing

during first pass of exception handling. The InlinedCallFrame is expected

to be processed before the related PInvoke IL stub and it doesn't happen

in this case, as a native frame is required to make the stack frame iterator

move to the explicit InlinedCallFrame. The fact that it is not processed

results in ignoring the upper limit on the stack walk and walking the stack

further than expected.
This change fixes it by detecting the direct transition from

UnmanagedCallersOnly to managed caller and forces the stack frame

iterator to process the InlinedCallFrame.
Fixes#57915
@janvorli
Fix x86 EH for UnmanagedCallersOnly methods
a943ad5
There is a bug in the exception handling on x86 when exception is
propagated from a method marked with UnmanagedCallersOnly attribute to
an immediate caller that's also managed (and calls the method via a
native delegate). In this case, there is no native frame in between and
the stack walker accidentally skips the InlinedCallFrame processing
during first pass of exception handling. The InlinedCallFrame is expected
to be processed before the related PInvoke IL stub and it doesn't happen
in this case, as a native frame is required to make the stack frame iterator
move to the explicit InlinedCallFrame. The fact that it is not processed
results in ignoring the upper limit on the stack walk and walking the stack
further than expected.
This change fixes it by detecting the direct transition from
UnmanagedCallersOnly to managed caller and forces the stack frame
iterator to process the InlinedCallFrame.
@ghost
ghost
commented
Sep 8, 2021

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@janvorli
janvorli
requested a review
from jkotasSeptember 8, 2021 12:04
@janvorlijanvorli
self-assigned this
Sep 8, 2021
@janvorlijanvorli
added
the 
area-ExceptionHandling-coreclr
only use for closed issues
label
Sep 8, 2021
@janvorlijanvorli
added this to the 7.0.0 milestone
Sep 8, 2021
@janvorli
janvorli
commented
Sep 8, 2021

Copy link


Copy Markdown

MemberAuthor
cc: @AaronRobinsonMSFT
jkotas
jkotas
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/methodtablebuilder.cpp
Outdated

AaronRobinsonMSFT
AaronRobinsonMSFT
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/ilstubcache.cpp
Outdated

@janvorli
janvorliforce-pushed
the
fix-x86-eh-unmanagedcallers
branch
from
62f65f1 to
36861eeCompareSeptember 8, 2021 19:37
jkotas
jkotas
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/stackwalk.cpp

#ifdef TARGET_X86
 hdrInfo gcHdrInfo;
 DecodeGCHdrInfo(m_crawl.codeInfo.GetGCInfoToken(), 0, &gcHdrInfo);
@jkotasjkotasSep 8, 2021

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.

All other places that decode GCInfo in this file go through m_crawl.GetCodeManager(). Is it worth following the pattern?
@janvorlijanvorliSep 8, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good idea, but let me do that as a separate cleanup so that this change is just a fix for the bug. I will need to add a new method to the EECodeManager and then I'd use it in the exceptionhandling.cpp too where we currently also have this raw decoding.
jkotas
jkotas
approved these changes
Sep 8, 2021
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
@janvorli
janvorli
merged commit 7754114
into
dotnet:mainSep 9, 2021
@janvorli
janvorli
deleted the

fix-x86-eh-unmanagedcallers

branch
September 9, 2021 08:54
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 9, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@AaronRobinsonMSFTAaronRobinsonMSFTAaronRobinsonMSFT approved these changes

Assignees

@janvorlijanvorli

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

[JitStressWithRandomNumbers] UnmanagedCallersOnlyTest fails.


3 participants

@janvorli@jkotas@AaronRobinsonMSFT

, '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); }
})();
})();
Fix x86 EH for UnmanagedCallersOnly methods by janvorli · Pull Request #58796 · dotnet/runtime · GitHub
Skip to content
Fix x86 EH for UnmanagedCallersOnly methods - #58796
Merged
janvorli merged 2 commits into
dotnet:mainfrom 
janvorli:fix-x86-eh-unmanagedcallers
Sep 9, 2021
Merged
Fix x86 EH for UnmanagedCallersOnly methods#58796
janvorli merged 2 commits into
dotnet:mainfrom 
janvorli:fix-x86-eh-unmanagedcallers
Conversation
@janvorli
@janvorlijanvorli
commented
Sep 8, 2021•

edited by AaronRobinsonMSFT

Loading

Copy link


Copy Markdown

Member
There is a bug in the exception handling on x86 when exception is

propagated from a method marked with UnmanagedCallersOnly attribute to

an immediate caller that's also managed (and calls the method via a

native delegate). In this case, there is no native frame in between and

the stack walker accidentally skips the InlinedCallFrame processing

during first pass of exception handling. The InlinedCallFrame is expected

to be processed before the related PInvoke IL stub and it doesn't happen

in this case, as a native frame is required to make the stack frame iterator

move to the explicit InlinedCallFrame. The fact that it is not processed

results in ignoring the upper limit on the stack walk and walking the stack

further than expected.
This change fixes it by detecting the direct transition from

UnmanagedCallersOnly to managed caller and forces the stack frame

iterator to process the InlinedCallFrame.
Fixes#57915
@janvorli
Fix x86 EH for UnmanagedCallersOnly methods
a943ad5
There is a bug in the exception handling on x86 when exception is
propagated from a method marked with UnmanagedCallersOnly attribute to
an immediate caller that's also managed (and calls the method via a
native delegate). In this case, there is no native frame in between and
the stack walker accidentally skips the InlinedCallFrame processing
during first pass of exception handling. The InlinedCallFrame is expected
to be processed before the related PInvoke IL stub and it doesn't happen
in this case, as a native frame is required to make the stack frame iterator
move to the explicit InlinedCallFrame. The fact that it is not processed
results in ignoring the upper limit on the stack walk and walking the stack
further than expected.
This change fixes it by detecting the direct transition from
UnmanagedCallersOnly to managed caller and forces the stack frame
iterator to process the InlinedCallFrame.
@ghost
ghost
commented
Sep 8, 2021

Copy link


Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.
@janvorli
janvorli
requested a review
from jkotasSeptember 8, 2021 12:04
@janvorlijanvorli
self-assigned this
Sep 8, 2021
@janvorlijanvorli
added
the 
area-ExceptionHandling-coreclr
only use for closed issues
label
Sep 8, 2021
@janvorlijanvorli
added this to the 7.0.0 milestone
Sep 8, 2021
@janvorli
janvorli
commented
Sep 8, 2021

Copy link


Copy Markdown

MemberAuthor
cc: @AaronRobinsonMSFT
jkotas
jkotas
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/methodtablebuilder.cpp
Outdated

AaronRobinsonMSFT
AaronRobinsonMSFT
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/ilstubcache.cpp
Outdated

@janvorli
janvorliforce-pushed
the
fix-x86-eh-unmanagedcallers
branch
from
62f65f1 to
36861eeCompareSeptember 8, 2021 19:37
jkotas
jkotas
reviewed
Sep 8, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/stackwalk.cpp

#ifdef TARGET_X86
 hdrInfo gcHdrInfo;
 DecodeGCHdrInfo(m_crawl.codeInfo.GetGCInfoToken(), 0, &gcHdrInfo);
@jkotasjkotasSep 8, 2021

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.

All other places that decode GCInfo in this file go through m_crawl.GetCodeManager(). Is it worth following the pattern?
@janvorlijanvorliSep 8, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Good idea, but let me do that as a separate cleanup so that this change is just a fix for the bug. I will need to add a new method to the EECodeManager and then I'd use it in the exceptionhandling.cpp too where we currently also have this raw decoding.
jkotas
jkotas
approved these changes
Sep 8, 2021
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
@janvorli
janvorli
merged commit 7754114
into
dotnet:mainSep 9, 2021
@janvorli
janvorli
deleted the

fix-x86-eh-unmanagedcallers

branch
September 9, 2021 08:54
@ghostghost
locked as resolved and limited conversation to collaborators
Oct 9, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@AaronRobinsonMSFTAaronRobinsonMSFTAaronRobinsonMSFT approved these changes

Assignees

@janvorlijanvorli

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

[JitStressWithRandomNumbers] UnmanagedCallersOnlyTest fails.


3 participants

@janvorli@jkotas@AaronRobinsonMSFT