[cdac] GetMethodDescData for jitted methods - #109187

Merged
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions
Nov 1, 2024
Merged

[cdac] GetMethodDescData for jitted methods#109187
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions

Conversation

@lambdageek

@lambdageeklambdageek commented Oct 24, 2024

Copy link
Copy Markdown
Member

This is enough for !PrintException without R2R methods on the stack

There's also a ReJIT contract here which just checks whether rejit is enabled.

Most of the complexity is in validating MethodDescs

Contributes to #99302
Contributes to #108553

also move method flags out of the RuntimeTypeSystem_1 contract
for the cases where the method validation needs to call back to type validation, go via the contract
@lambdageek
lambdageek marked this pull request as ready for review October 30, 2024 18:40
Comment threadsrc/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs Outdated
lambdageekand others added 3 commits October 30, 2024 15:30

@davidwrightondavidwrighton 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.

I've read through this and it seems like you've made good progress towards our goal here. I've a few comments about formatting + a few more general comments.

  1. I see a lot of code dedicated to MethodDesc validation. Is that really necessary, and does the behavior match our existing DAC? We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results. Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location? Is the validation logic documented in the data contract markdown?
  2. I don't see documentation updates in the markdown yet. I see that some existing contracts look to have their implementation modified, but I don't see the corresponding documentation updates.
  3. Would it be possible to build a bulleted list of the TODO items that are not assertions that are needed to make this contract work completely?

@lambdageek

Copy link
Copy Markdown
MemberAuthor

I see a lot of code dedicated to MethodDesc validation.

yes

Is that really necessary, and does the behavior match our existing DAC?

I tried to make it match the existing DAC.

Is it necessary? I'm not sure. We do need some validation - I have discovered that the !U command in SOS doesn't differentiate between a MethodDesc and an IP address and expects to be able to call GetMethodDescData with an IP address as the "method desc" argument and get E_INVALIDARG to tell it "this was not a method desc, it was an IP address"

We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results.

Understood

Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location?

There is additional validation here. The version checked in previously was an incomplete snapshot (basically just establishing the internal API for validation).

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

@davidwrighton

Copy link
Copy Markdown
Member

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

I don't know. It's certainly something that the implementation of the contract depends on to some extent, so all of the dependencies of it (such as the fields it accesses, etc) should probably be in the contract, but the actual validation logic is a more interesting question. I would leave it out of the contract spec for now, so I wouldn't worry about it this week, but given that SOS actually depends on this logic doing something useful for correct functioning, its implied that something is required to make our diagnostic stack work. I would file a bug that you didn't write contract docs for this and leave it at that.

@lambdageek

lambdageek commented Oct 31, 2024

Copy link
Copy Markdown
MemberAuthor

I updated the RuntimeTypeSystem contract markdown with the new code and fixed up other contracts to match the implementations as necessary

Created issues:

the "additional pointers" logic in RuntimeTypeSystem_1 depends on the sizes
@lambdageek
lambdageek merged commit 65d6ef6 into dotnet:mainNov 1, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 2, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@lambdageek@davidwrighton@AaronRobinsonMSFT
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[cdac] GetMethodDescData for jitted methods - #109187

Merged
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions
Nov 1, 2024
Merged

[cdac] GetMethodDescData for jitted methods#109187
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions

Conversation

@lambdageek

@lambdageeklambdageek commented Oct 24, 2024

Copy link
Copy Markdown
Member

This is enough for !PrintException without R2R methods on the stack

There's also a ReJIT contract here which just checks whether rejit is enabled.

Most of the complexity is in validating MethodDescs

Contributes to #99302
Contributes to #108553

also move method flags out of the RuntimeTypeSystem_1 contract
for the cases where the method validation needs to call back to type validation, go via the contract
@lambdageek
lambdageek marked this pull request as ready for review October 30, 2024 18:40
Comment threadsrc/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs Outdated
lambdageekand others added 3 commits October 30, 2024 15:30

@davidwrightondavidwrighton 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.

I've read through this and it seems like you've made good progress towards our goal here. I've a few comments about formatting + a few more general comments.

  1. I see a lot of code dedicated to MethodDesc validation. Is that really necessary, and does the behavior match our existing DAC? We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results. Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location? Is the validation logic documented in the data contract markdown?
  2. I don't see documentation updates in the markdown yet. I see that some existing contracts look to have their implementation modified, but I don't see the corresponding documentation updates.
  3. Would it be possible to build a bulleted list of the TODO items that are not assertions that are needed to make this contract work completely?

@lambdageek

Copy link
Copy Markdown
MemberAuthor

I see a lot of code dedicated to MethodDesc validation.

yes

Is that really necessary, and does the behavior match our existing DAC?

I tried to make it match the existing DAC.

Is it necessary? I'm not sure. We do need some validation - I have discovered that the !U command in SOS doesn't differentiate between a MethodDesc and an IP address and expects to be able to call GetMethodDescData with an IP address as the "method desc" argument and get E_INVALIDARG to tell it "this was not a method desc, it was an IP address"

We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results.

Understood

Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location?

There is additional validation here. The version checked in previously was an incomplete snapshot (basically just establishing the internal API for validation).

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

@davidwrighton

Copy link
Copy Markdown
Member

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

I don't know. It's certainly something that the implementation of the contract depends on to some extent, so all of the dependencies of it (such as the fields it accesses, etc) should probably be in the contract, but the actual validation logic is a more interesting question. I would leave it out of the contract spec for now, so I wouldn't worry about it this week, but given that SOS actually depends on this logic doing something useful for correct functioning, its implied that something is required to make our diagnostic stack work. I would file a bug that you didn't write contract docs for this and leave it at that.

@lambdageek

lambdageek commented Oct 31, 2024

Copy link
Copy Markdown
MemberAuthor

I updated the RuntimeTypeSystem contract markdown with the new code and fixed up other contracts to match the implementations as necessary

Created issues:

the "additional pointers" logic in RuntimeTypeSystem_1 depends on the sizes
@lambdageek
lambdageek merged commit 65d6ef6 into dotnet:mainNov 1, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 2, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@lambdageek@davidwrighton@AaronRobinsonMSFT
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[cdac] GetMethodDescData for jitted methods - #109187

Merged
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions
Nov 1, 2024
Merged

[cdac] GetMethodDescData for jitted methods#109187
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions

Conversation

@lambdageek

@lambdageeklambdageek commented Oct 24, 2024

Copy link
Copy Markdown
Member

This is enough for !PrintException without R2R methods on the stack

There's also a ReJIT contract here which just checks whether rejit is enabled.

Most of the complexity is in validating MethodDescs

Contributes to #99302
Contributes to #108553

also move method flags out of the RuntimeTypeSystem_1 contract
for the cases where the method validation needs to call back to type validation, go via the contract
@lambdageek
lambdageek marked this pull request as ready for review October 30, 2024 18:40
Comment threadsrc/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs Outdated
lambdageekand others added 3 commits October 30, 2024 15:30

@davidwrightondavidwrighton 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.

I've read through this and it seems like you've made good progress towards our goal here. I've a few comments about formatting + a few more general comments.

  1. I see a lot of code dedicated to MethodDesc validation. Is that really necessary, and does the behavior match our existing DAC? We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results. Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location? Is the validation logic documented in the data contract markdown?
  2. I don't see documentation updates in the markdown yet. I see that some existing contracts look to have their implementation modified, but I don't see the corresponding documentation updates.
  3. Would it be possible to build a bulleted list of the TODO items that are not assertions that are needed to make this contract work completely?

@lambdageek

Copy link
Copy Markdown
MemberAuthor

I see a lot of code dedicated to MethodDesc validation.

yes

Is that really necessary, and does the behavior match our existing DAC?

I tried to make it match the existing DAC.

Is it necessary? I'm not sure. We do need some validation - I have discovered that the !U command in SOS doesn't differentiate between a MethodDesc and an IP address and expects to be able to call GetMethodDescData with an IP address as the "method desc" argument and get E_INVALIDARG to tell it "this was not a method desc, it was an IP address"

We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results.

Understood

Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location?

There is additional validation here. The version checked in previously was an incomplete snapshot (basically just establishing the internal API for validation).

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

@davidwrighton

Copy link
Copy Markdown
Member

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

I don't know. It's certainly something that the implementation of the contract depends on to some extent, so all of the dependencies of it (such as the fields it accesses, etc) should probably be in the contract, but the actual validation logic is a more interesting question. I would leave it out of the contract spec for now, so I wouldn't worry about it this week, but given that SOS actually depends on this logic doing something useful for correct functioning, its implied that something is required to make our diagnostic stack work. I would file a bug that you didn't write contract docs for this and leave it at that.

@lambdageek

lambdageek commented Oct 31, 2024

Copy link
Copy Markdown
MemberAuthor

I updated the RuntimeTypeSystem contract markdown with the new code and fixed up other contracts to match the implementations as necessary

Created issues:

the "additional pointers" logic in RuntimeTypeSystem_1 depends on the sizes
@lambdageek
lambdageek merged commit 65d6ef6 into dotnet:mainNov 1, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 2, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@lambdageek@davidwrighton@AaronRobinsonMSFT
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[cdac] GetMethodDescData for jitted methods - #109187

Merged
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions
Nov 1, 2024
Merged

[cdac] GetMethodDescData for jitted methods#109187
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions

Conversation

@lambdageek

@lambdageeklambdageek commented Oct 24, 2024

Copy link
Copy Markdown
Member

This is enough for !PrintException without R2R methods on the stack

There's also a ReJIT contract here which just checks whether rejit is enabled.

Most of the complexity is in validating MethodDescs

Contributes to #99302
Contributes to #108553

also move method flags out of the RuntimeTypeSystem_1 contract
for the cases where the method validation needs to call back to type validation, go via the contract
@lambdageek
lambdageek marked this pull request as ready for review October 30, 2024 18:40
Comment threadsrc/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs Outdated
lambdageekand others added 3 commits October 30, 2024 15:30

@davidwrightondavidwrighton 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.

I've read through this and it seems like you've made good progress towards our goal here. I've a few comments about formatting + a few more general comments.

  1. I see a lot of code dedicated to MethodDesc validation. Is that really necessary, and does the behavior match our existing DAC? We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results. Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location? Is the validation logic documented in the data contract markdown?
  2. I don't see documentation updates in the markdown yet. I see that some existing contracts look to have their implementation modified, but I don't see the corresponding documentation updates.
  3. Would it be possible to build a bulleted list of the TODO items that are not assertions that are needed to make this contract work completely?

@lambdageek

Copy link
Copy Markdown
MemberAuthor

I see a lot of code dedicated to MethodDesc validation.

yes

Is that really necessary, and does the behavior match our existing DAC?

I tried to make it match the existing DAC.

Is it necessary? I'm not sure. We do need some validation - I have discovered that the !U command in SOS doesn't differentiate between a MethodDesc and an IP address and expects to be able to call GetMethodDescData with an IP address as the "method desc" argument and get E_INVALIDARG to tell it "this was not a method desc, it was an IP address"

We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results.

Understood

Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location?

There is additional validation here. The version checked in previously was an incomplete snapshot (basically just establishing the internal API for validation).

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

@davidwrighton

Copy link
Copy Markdown
Member

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

I don't know. It's certainly something that the implementation of the contract depends on to some extent, so all of the dependencies of it (such as the fields it accesses, etc) should probably be in the contract, but the actual validation logic is a more interesting question. I would leave it out of the contract spec for now, so I wouldn't worry about it this week, but given that SOS actually depends on this logic doing something useful for correct functioning, its implied that something is required to make our diagnostic stack work. I would file a bug that you didn't write contract docs for this and leave it at that.

@lambdageek

lambdageek commented Oct 31, 2024

Copy link
Copy Markdown
MemberAuthor

I updated the RuntimeTypeSystem contract markdown with the new code and fixed up other contracts to match the implementations as necessary

Created issues:

the "additional pointers" logic in RuntimeTypeSystem_1 depends on the sizes
@lambdageek
lambdageek merged commit 65d6ef6 into dotnet:mainNov 1, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 2, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@lambdageek@davidwrighton@AaronRobinsonMSFT
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[cdac] GetMethodDescData for jitted methods - #109187

Merged
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions
Nov 1, 2024
Merged

[cdac] GetMethodDescData for jitted methods#109187
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions

Conversation

@lambdageek

@lambdageeklambdageek commented Oct 24, 2024

Copy link
Copy Markdown
Member

This is enough for !PrintException without R2R methods on the stack

There's also a ReJIT contract here which just checks whether rejit is enabled.

Most of the complexity is in validating MethodDescs

Contributes to #99302
Contributes to #108553

also move method flags out of the RuntimeTypeSystem_1 contract
for the cases where the method validation needs to call back to type validation, go via the contract
@lambdageek
lambdageek marked this pull request as ready for review October 30, 2024 18:40
Comment threadsrc/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs Outdated
lambdageekand others added 3 commits October 30, 2024 15:30

@davidwrightondavidwrighton 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.

I've read through this and it seems like you've made good progress towards our goal here. I've a few comments about formatting + a few more general comments.

  1. I see a lot of code dedicated to MethodDesc validation. Is that really necessary, and does the behavior match our existing DAC? We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results. Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location? Is the validation logic documented in the data contract markdown?
  2. I don't see documentation updates in the markdown yet. I see that some existing contracts look to have their implementation modified, but I don't see the corresponding documentation updates.
  3. Would it be possible to build a bulleted list of the TODO items that are not assertions that are needed to make this contract work completely?

@lambdageek

Copy link
Copy Markdown
MemberAuthor

I see a lot of code dedicated to MethodDesc validation.

yes

Is that really necessary, and does the behavior match our existing DAC?

I tried to make it match the existing DAC.

Is it necessary? I'm not sure. We do need some validation - I have discovered that the !U command in SOS doesn't differentiate between a MethodDesc and an IP address and expects to be able to call GetMethodDescData with an IP address as the "method desc" argument and get E_INVALIDARG to tell it "this was not a method desc, it was an IP address"

We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results.

Understood

Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location?

There is additional validation here. The version checked in previously was an incomplete snapshot (basically just establishing the internal API for validation).

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

@davidwrighton

Copy link
Copy Markdown
Member

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

I don't know. It's certainly something that the implementation of the contract depends on to some extent, so all of the dependencies of it (such as the fields it accesses, etc) should probably be in the contract, but the actual validation logic is a more interesting question. I would leave it out of the contract spec for now, so I wouldn't worry about it this week, but given that SOS actually depends on this logic doing something useful for correct functioning, its implied that something is required to make our diagnostic stack work. I would file a bug that you didn't write contract docs for this and leave it at that.

@lambdageek

lambdageek commented Oct 31, 2024

Copy link
Copy Markdown
MemberAuthor

I updated the RuntimeTypeSystem contract markdown with the new code and fixed up other contracts to match the implementations as necessary

Created issues:

the "additional pointers" logic in RuntimeTypeSystem_1 depends on the sizes
@lambdageek
lambdageek merged commit 65d6ef6 into dotnet:mainNov 1, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 2, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@lambdageek@davidwrighton@AaronRobinsonMSFT
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[cdac] GetMethodDescData for jitted methods - #109187

Merged
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions
Nov 1, 2024
Merged

[cdac] GetMethodDescData for jitted methods#109187
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions

Conversation

@lambdageek

@lambdageeklambdageek commented Oct 24, 2024

Copy link
Copy Markdown
Member

This is enough for !PrintException without R2R methods on the stack

There's also a ReJIT contract here which just checks whether rejit is enabled.

Most of the complexity is in validating MethodDescs

Contributes to #99302
Contributes to #108553

also move method flags out of the RuntimeTypeSystem_1 contract
for the cases where the method validation needs to call back to type validation, go via the contract
@lambdageek
lambdageek marked this pull request as ready for review October 30, 2024 18:40
Comment threadsrc/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs Outdated
lambdageekand others added 3 commits October 30, 2024 15:30

@davidwrightondavidwrighton 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.

I've read through this and it seems like you've made good progress towards our goal here. I've a few comments about formatting + a few more general comments.

  1. I see a lot of code dedicated to MethodDesc validation. Is that really necessary, and does the behavior match our existing DAC? We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results. Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location? Is the validation logic documented in the data contract markdown?
  2. I don't see documentation updates in the markdown yet. I see that some existing contracts look to have their implementation modified, but I don't see the corresponding documentation updates.
  3. Would it be possible to build a bulleted list of the TODO items that are not assertions that are needed to make this contract work completely?

@lambdageek

Copy link
Copy Markdown
MemberAuthor

I see a lot of code dedicated to MethodDesc validation.

yes

Is that really necessary, and does the behavior match our existing DAC?

I tried to make it match the existing DAC.

Is it necessary? I'm not sure. We do need some validation - I have discovered that the !U command in SOS doesn't differentiate between a MethodDesc and an IP address and expects to be able to call GetMethodDescData with an IP address as the "method desc" argument and get E_INVALIDARG to tell it "this was not a method desc, it was an IP address"

We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results.

Understood

Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location?

There is additional validation here. The version checked in previously was an incomplete snapshot (basically just establishing the internal API for validation).

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

@davidwrighton

Copy link
Copy Markdown
Member

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

I don't know. It's certainly something that the implementation of the contract depends on to some extent, so all of the dependencies of it (such as the fields it accesses, etc) should probably be in the contract, but the actual validation logic is a more interesting question. I would leave it out of the contract spec for now, so I wouldn't worry about it this week, but given that SOS actually depends on this logic doing something useful for correct functioning, its implied that something is required to make our diagnostic stack work. I would file a bug that you didn't write contract docs for this and leave it at that.

@lambdageek

lambdageek commented Oct 31, 2024

Copy link
Copy Markdown
MemberAuthor

I updated the RuntimeTypeSystem contract markdown with the new code and fixed up other contracts to match the implementations as necessary

Created issues:

the "additional pointers" logic in RuntimeTypeSystem_1 depends on the sizes
@lambdageek
lambdageek merged commit 65d6ef6 into dotnet:mainNov 1, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 2, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@lambdageek@davidwrighton@AaronRobinsonMSFT
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[cdac] GetMethodDescData for jitted methods - #109187

Merged
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions
Nov 1, 2024
Merged

[cdac] GetMethodDescData for jitted methods#109187
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions

Conversation

@lambdageek

@lambdageeklambdageek commented Oct 24, 2024

Copy link
Copy Markdown
Member

This is enough for !PrintException without R2R methods on the stack

There's also a ReJIT contract here which just checks whether rejit is enabled.

Most of the complexity is in validating MethodDescs

Contributes to #99302
Contributes to #108553

also move method flags out of the RuntimeTypeSystem_1 contract
for the cases where the method validation needs to call back to type validation, go via the contract
@lambdageek
lambdageek marked this pull request as ready for review October 30, 2024 18:40
Comment threadsrc/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs Outdated
lambdageekand others added 3 commits October 30, 2024 15:30

@davidwrightondavidwrighton 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.

I've read through this and it seems like you've made good progress towards our goal here. I've a few comments about formatting + a few more general comments.

  1. I see a lot of code dedicated to MethodDesc validation. Is that really necessary, and does the behavior match our existing DAC? We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results. Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location? Is the validation logic documented in the data contract markdown?
  2. I don't see documentation updates in the markdown yet. I see that some existing contracts look to have their implementation modified, but I don't see the corresponding documentation updates.
  3. Would it be possible to build a bulleted list of the TODO items that are not assertions that are needed to make this contract work completely?

@lambdageek

Copy link
Copy Markdown
MemberAuthor

I see a lot of code dedicated to MethodDesc validation.

yes

Is that really necessary, and does the behavior match our existing DAC?

I tried to make it match the existing DAC.

Is it necessary? I'm not sure. We do need some validation - I have discovered that the !U command in SOS doesn't differentiate between a MethodDesc and an IP address and expects to be able to call GetMethodDescData with an IP address as the "method desc" argument and get E_INVALIDARG to tell it "this was not a method desc, it was an IP address"

We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results.

Understood

Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location?

There is additional validation here. The version checked in previously was an incomplete snapshot (basically just establishing the internal API for validation).

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

@davidwrighton

Copy link
Copy Markdown
Member

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

I don't know. It's certainly something that the implementation of the contract depends on to some extent, so all of the dependencies of it (such as the fields it accesses, etc) should probably be in the contract, but the actual validation logic is a more interesting question. I would leave it out of the contract spec for now, so I wouldn't worry about it this week, but given that SOS actually depends on this logic doing something useful for correct functioning, its implied that something is required to make our diagnostic stack work. I would file a bug that you didn't write contract docs for this and leave it at that.

@lambdageek

lambdageek commented Oct 31, 2024

Copy link
Copy Markdown
MemberAuthor

I updated the RuntimeTypeSystem contract markdown with the new code and fixed up other contracts to match the implementations as necessary

Created issues:

the "additional pointers" logic in RuntimeTypeSystem_1 depends on the sizes
@lambdageek
lambdageek merged commit 65d6ef6 into dotnet:mainNov 1, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 2, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@lambdageek@davidwrighton@AaronRobinsonMSFT
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[cdac] GetMethodDescData for jitted methods - #109187

Merged
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions
Nov 1, 2024
Merged

[cdac] GetMethodDescData for jitted methods#109187
lambdageek merged 295 commits into
dotnet:mainfrom
lambdageek:cdac-abstractions

Conversation

@lambdageek

@lambdageeklambdageek commented Oct 24, 2024

Copy link
Copy Markdown
Member

This is enough for !PrintException without R2R methods on the stack

There's also a ReJIT contract here which just checks whether rejit is enabled.

Most of the complexity is in validating MethodDescs

Contributes to #99302
Contributes to #108553

also move method flags out of the RuntimeTypeSystem_1 contract
for the cases where the method validation needs to call back to type validation, go via the contract
@lambdageek
lambdageek marked this pull request as ready for review October 30, 2024 18:40
Comment threadsrc/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs Outdated
lambdageekand others added 3 commits October 30, 2024 15:30

@davidwrightondavidwrighton 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.

I've read through this and it seems like you've made good progress towards our goal here. I've a few comments about formatting + a few more general comments.

  1. I see a lot of code dedicated to MethodDesc validation. Is that really necessary, and does the behavior match our existing DAC? We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results. Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location? Is the validation logic documented in the data contract markdown?
  2. I don't see documentation updates in the markdown yet. I see that some existing contracts look to have their implementation modified, but I don't see the corresponding documentation updates.
  3. Would it be possible to build a bulleted list of the TODO items that are not assertions that are needed to make this contract work completely?

@lambdageek

Copy link
Copy Markdown
MemberAuthor

I see a lot of code dedicated to MethodDesc validation.

yes

Is that really necessary, and does the behavior match our existing DAC?

I tried to make it match the existing DAC.

Is it necessary? I'm not sure. We do need some validation - I have discovered that the !U command in SOS doesn't differentiate between a MethodDesc and an IP address and expects to be able to call GetMethodDescData with an IP address as the "method desc" argument and get E_INVALIDARG to tell it "this was not a method desc, it was an IP address"

We may run this in an environment where the full heap isn't available, and we might want code to do a best effort job of producing results.

Understood

Also, are there changes to the MethodDesc validation logic here, or just refactoring to a new location?

There is additional validation here. The version checked in previously was an incomplete snapshot (basically just establishing the internal API for validation).

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

@davidwrighton

Copy link
Copy Markdown
Member

Is the validation logic documented in the data contract markdown?

No. Is it part of the contract? I've been thinking of it as an artifact of the reader

I don't know. It's certainly something that the implementation of the contract depends on to some extent, so all of the dependencies of it (such as the fields it accesses, etc) should probably be in the contract, but the actual validation logic is a more interesting question. I would leave it out of the contract spec for now, so I wouldn't worry about it this week, but given that SOS actually depends on this logic doing something useful for correct functioning, its implied that something is required to make our diagnostic stack work. I would file a bug that you didn't write contract docs for this and leave it at that.

@lambdageek

lambdageek commented Oct 31, 2024

Copy link
Copy Markdown
MemberAuthor

I updated the RuntimeTypeSystem contract markdown with the new code and fixed up other contracts to match the implementations as necessary

Created issues:

the "additional pointers" logic in RuntimeTypeSystem_1 depends on the sizes
@lambdageek
lambdageek merged commit 65d6ef6 into dotnet:mainNov 1, 2024
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 2, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@lambdageek@davidwrighton@AaronRobinsonMSFT