Fix incorrect codemanager implementation on x86 by tommcdon · Pull Request #115391 · dotnet/runtime

tommcdon · 2025-05-08T04:33:05Z

This addresses windows x86 debugger setip issues introduced by #114170

dotnet-policy-service · 2025-05-08T04:33:38Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Copilot

Pull Request Overview

This PR fixes debugger setup issues on Windows x86 by addressing the incorrect implementation of the code manager.

Introduces a new DecodeGCHdrInfo method in jitinterface.cpp that uses a relative offset to compute the correct GC header info pointer.
Updates all affected call sites in eetwain.cpp to pass the proper structure (using a value instance and its address) and a dwRelOffset parameter.
Updates the codeman.h header with the new function prototype to support x86 changes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/coreclr/vm/jitinterface.cpp	Added the new DecodeGCHdrInfo method for proper GC header info decoding
src/coreclr/vm/eetwain.cpp	Modified calls to DecodeGCHdrInfo and adjusted usage of the hdrInfo structure
src/coreclr/vm/codeman.h	Added the function prototype for DecodeGCHdrInfo

Comments suppressed due to low confidence (1)

src/coreclr/vm/eetwain.cpp:90

Verify that the updated API for GetHandlerFrameInfo (which now accepts a pointer to hdrInfo) is consistently implemented across all call sites.

GetHandlerFrameInfo(&hdrInfoBody, ctx->Ebp,

filipnavara

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

…:GetAmbientSP

tommcdon · 2025-05-08T16:40:39Z

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I've pushed a commit to take advantage of the perf improvement when the relative offset matches the code manager's built-in offset.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

No worries. You are correct - we have some internal Visual Studio testing that caught the issue.

tommcdon · 2025-05-08T04:33:05Z

This addresses windows x86 debugger setip issues introduced by #114170

dotnet-policy-service · 2025-05-08T04:33:38Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Copilot

Pull Request Overview

This PR fixes debugger setup issues on Windows x86 by addressing the incorrect implementation of the code manager.

Introduces a new DecodeGCHdrInfo method in jitinterface.cpp that uses a relative offset to compute the correct GC header info pointer.
Updates all affected call sites in eetwain.cpp to pass the proper structure (using a value instance and its address) and a dwRelOffset parameter.
Updates the codeman.h header with the new function prototype to support x86 changes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/coreclr/vm/jitinterface.cpp	Added the new DecodeGCHdrInfo method for proper GC header info decoding
src/coreclr/vm/eetwain.cpp	Modified calls to DecodeGCHdrInfo and adjusted usage of the hdrInfo structure
src/coreclr/vm/codeman.h	Added the function prototype for DecodeGCHdrInfo

Comments suppressed due to low confidence (1)

src/coreclr/vm/eetwain.cpp:90

Verify that the updated API for GetHandlerFrameInfo (which now accepts a pointer to hdrInfo) is consistently implemented across all call sites.

GetHandlerFrameInfo(&hdrInfoBody, ctx->Ebp,

filipnavara

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

…:GetAmbientSP

tommcdon · 2025-05-08T16:40:39Z

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I've pushed a commit to take advantage of the perf improvement when the relative offset matches the code manager's built-in offset.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

No worries. You are correct - we have some internal Visual Studio testing that caught the issue.

tommcdon · 2025-05-08T04:33:05Z

This addresses windows x86 debugger setip issues introduced by #114170

dotnet-policy-service · 2025-05-08T04:33:38Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Copilot

Pull Request Overview

This PR fixes debugger setup issues on Windows x86 by addressing the incorrect implementation of the code manager.

Introduces a new DecodeGCHdrInfo method in jitinterface.cpp that uses a relative offset to compute the correct GC header info pointer.
Updates all affected call sites in eetwain.cpp to pass the proper structure (using a value instance and its address) and a dwRelOffset parameter.
Updates the codeman.h header with the new function prototype to support x86 changes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/coreclr/vm/jitinterface.cpp	Added the new DecodeGCHdrInfo method for proper GC header info decoding
src/coreclr/vm/eetwain.cpp	Modified calls to DecodeGCHdrInfo and adjusted usage of the hdrInfo structure
src/coreclr/vm/codeman.h	Added the function prototype for DecodeGCHdrInfo

Comments suppressed due to low confidence (1)

src/coreclr/vm/eetwain.cpp:90

Verify that the updated API for GetHandlerFrameInfo (which now accepts a pointer to hdrInfo) is consistently implemented across all call sites.

GetHandlerFrameInfo(&hdrInfoBody, ctx->Ebp,

filipnavara

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

…:GetAmbientSP

tommcdon · 2025-05-08T16:40:39Z

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I've pushed a commit to take advantage of the perf improvement when the relative offset matches the code manager's built-in offset.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

No worries. You are correct - we have some internal Visual Studio testing that caught the issue.

tommcdon · 2025-05-08T04:33:05Z

This addresses windows x86 debugger setip issues introduced by #114170

dotnet-policy-service · 2025-05-08T04:33:38Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Copilot

Pull Request Overview

This PR fixes debugger setup issues on Windows x86 by addressing the incorrect implementation of the code manager.

Introduces a new DecodeGCHdrInfo method in jitinterface.cpp that uses a relative offset to compute the correct GC header info pointer.
Updates all affected call sites in eetwain.cpp to pass the proper structure (using a value instance and its address) and a dwRelOffset parameter.
Updates the codeman.h header with the new function prototype to support x86 changes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/coreclr/vm/jitinterface.cpp	Added the new DecodeGCHdrInfo method for proper GC header info decoding
src/coreclr/vm/eetwain.cpp	Modified calls to DecodeGCHdrInfo and adjusted usage of the hdrInfo structure
src/coreclr/vm/codeman.h	Added the function prototype for DecodeGCHdrInfo

Comments suppressed due to low confidence (1)

src/coreclr/vm/eetwain.cpp:90

Verify that the updated API for GetHandlerFrameInfo (which now accepts a pointer to hdrInfo) is consistently implemented across all call sites.

GetHandlerFrameInfo(&hdrInfoBody, ctx->Ebp,

filipnavara

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

…:GetAmbientSP

tommcdon · 2025-05-08T16:40:39Z

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I've pushed a commit to take advantage of the perf improvement when the relative offset matches the code manager's built-in offset.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

No worries. You are correct - we have some internal Visual Studio testing that caught the issue.

tommcdon · 2025-05-08T04:33:05Z

This addresses windows x86 debugger setip issues introduced by #114170

dotnet-policy-service · 2025-05-08T04:33:38Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Copilot

Pull Request Overview

This PR fixes debugger setup issues on Windows x86 by addressing the incorrect implementation of the code manager.

Introduces a new DecodeGCHdrInfo method in jitinterface.cpp that uses a relative offset to compute the correct GC header info pointer.
Updates all affected call sites in eetwain.cpp to pass the proper structure (using a value instance and its address) and a dwRelOffset parameter.
Updates the codeman.h header with the new function prototype to support x86 changes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/coreclr/vm/jitinterface.cpp	Added the new DecodeGCHdrInfo method for proper GC header info decoding
src/coreclr/vm/eetwain.cpp	Modified calls to DecodeGCHdrInfo and adjusted usage of the hdrInfo structure
src/coreclr/vm/codeman.h	Added the function prototype for DecodeGCHdrInfo

Comments suppressed due to low confidence (1)

src/coreclr/vm/eetwain.cpp:90

Verify that the updated API for GetHandlerFrameInfo (which now accepts a pointer to hdrInfo) is consistently implemented across all call sites.

GetHandlerFrameInfo(&hdrInfoBody, ctx->Ebp,

filipnavara

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

…:GetAmbientSP

tommcdon · 2025-05-08T16:40:39Z

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I've pushed a commit to take advantage of the perf improvement when the relative offset matches the code manager's built-in offset.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

No worries. You are correct - we have some internal Visual Studio testing that caught the issue.

tommcdon · 2025-05-08T04:33:05Z

This addresses windows x86 debugger setip issues introduced by #114170

dotnet-policy-service · 2025-05-08T04:33:38Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Copilot

Pull Request Overview

This PR fixes debugger setup issues on Windows x86 by addressing the incorrect implementation of the code manager.

Introduces a new DecodeGCHdrInfo method in jitinterface.cpp that uses a relative offset to compute the correct GC header info pointer.
Updates all affected call sites in eetwain.cpp to pass the proper structure (using a value instance and its address) and a dwRelOffset parameter.
Updates the codeman.h header with the new function prototype to support x86 changes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/coreclr/vm/jitinterface.cpp	Added the new DecodeGCHdrInfo method for proper GC header info decoding
src/coreclr/vm/eetwain.cpp	Modified calls to DecodeGCHdrInfo and adjusted usage of the hdrInfo structure
src/coreclr/vm/codeman.h	Added the function prototype for DecodeGCHdrInfo

Comments suppressed due to low confidence (1)

src/coreclr/vm/eetwain.cpp:90

Verify that the updated API for GetHandlerFrameInfo (which now accepts a pointer to hdrInfo) is consistently implemented across all call sites.

GetHandlerFrameInfo(&hdrInfoBody, ctx->Ebp,

filipnavara

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

…:GetAmbientSP

tommcdon · 2025-05-08T16:40:39Z

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I've pushed a commit to take advantage of the perf improvement when the relative offset matches the code manager's built-in offset.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

No worries. You are correct - we have some internal Visual Studio testing that caught the issue.

tommcdon · 2025-05-08T04:33:05Z

This addresses windows x86 debugger setip issues introduced by #114170

dotnet-policy-service · 2025-05-08T04:33:38Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Copilot

Pull Request Overview

This PR fixes debugger setup issues on Windows x86 by addressing the incorrect implementation of the code manager.

Introduces a new DecodeGCHdrInfo method in jitinterface.cpp that uses a relative offset to compute the correct GC header info pointer.
Updates all affected call sites in eetwain.cpp to pass the proper structure (using a value instance and its address) and a dwRelOffset parameter.
Updates the codeman.h header with the new function prototype to support x86 changes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/coreclr/vm/jitinterface.cpp	Added the new DecodeGCHdrInfo method for proper GC header info decoding
src/coreclr/vm/eetwain.cpp	Modified calls to DecodeGCHdrInfo and adjusted usage of the hdrInfo structure
src/coreclr/vm/codeman.h	Added the function prototype for DecodeGCHdrInfo

Comments suppressed due to low confidence (1)

src/coreclr/vm/eetwain.cpp:90

Verify that the updated API for GetHandlerFrameInfo (which now accepts a pointer to hdrInfo) is consistently implemented across all call sites.

GetHandlerFrameInfo(&hdrInfoBody, ctx->Ebp,

filipnavara

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

…:GetAmbientSP

tommcdon · 2025-05-08T16:40:39Z

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I've pushed a commit to take advantage of the perf improvement when the relative offset matches the code manager's built-in offset.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

No worries. You are correct - we have some internal Visual Studio testing that caught the issue.

tommcdon · 2025-05-08T04:33:05Z

This addresses windows x86 debugger setip issues introduced by #114170

dotnet-policy-service · 2025-05-08T04:33:38Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Copilot

Pull Request Overview

This PR fixes debugger setup issues on Windows x86 by addressing the incorrect implementation of the code manager.

Introduces a new DecodeGCHdrInfo method in jitinterface.cpp that uses a relative offset to compute the correct GC header info pointer.
Updates all affected call sites in eetwain.cpp to pass the proper structure (using a value instance and its address) and a dwRelOffset parameter.
Updates the codeman.h header with the new function prototype to support x86 changes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/coreclr/vm/jitinterface.cpp	Added the new DecodeGCHdrInfo method for proper GC header info decoding
src/coreclr/vm/eetwain.cpp	Modified calls to DecodeGCHdrInfo and adjusted usage of the hdrInfo structure
src/coreclr/vm/codeman.h	Added the function prototype for DecodeGCHdrInfo

Comments suppressed due to low confidence (1)

src/coreclr/vm/eetwain.cpp:90

Verify that the updated API for GetHandlerFrameInfo (which now accepts a pointer to hdrInfo) is consistently implemented across all call sites.

GetHandlerFrameInfo(&hdrInfoBody, ctx->Ebp,

filipnavara

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

…:GetAmbientSP

tommcdon · 2025-05-08T16:40:39Z

Thanks! There's one code path that could possibly benefit from the cache optimization (GetAmbientSPFromCrawlFrame -> GetAmbientSP seems to use the same relOffset as the cache in EECodeInfo) but it's not worth the extra trouble, it is only used by debugger.

I've pushed a commit to take advantage of the perf improvement when the relative offset matches the code manager's built-in offset.

I'm sorry for breaking this but I assume there're no public tests that would alert me about this, right?

No worries. You are correct - we have some internal Visual Studio testing that caught the issue.

Fix incorrect codemanager implementation on x86
36582c8

tommcdon added this to the 10.0.0 milestone May 8, 2025

tommcdon self-assigned this May 8, 2025

CopilotAI review requested due to automatic review settings May 8, 2025 04:33

tommcdon added the area-Diagnostics-coreclr label May 8, 2025

tommcdon requested review from davidwrighton and noahfalk May 8, 2025 04:33

CopilotAI reviewed May 8, 2025
View reviewed changes

Comment threadsrc/coreclr/vm/jitinterface.cpp
Comment threadsrc/coreclr/vm/eetwain.cpp

tommcdon requested review from hoyosjs and thaystg May 8, 2025 04:35

jkotas requested a review from filipnavara May 8, 2025 05:37

filipnavara approved these changes May 8, 2025
View reviewed changes

This was referenced May 8, 2025
Nuget: Central Directory corrupt dotnet/dnceng#3099
Closed
The feed is either invalid or required packages were removed while the current operation was in progress dotnet/dnceng#3825
Closed

thaystg approved these changes May 8, 2025
View reviewed changes

Re-enable perf optimization for stackwalk scenairio in EECodeManager:…
297b0e3
…:GetAmbientSP

tommcdon force-pushed the dev/tommcdon/fix_x86_codemanager branch from 15b0feb to 297b0e3Compare May 8, 2025 16:34

Merge branch 'main' into dev/tommcdon/fix_x86_codemanager
6e95b38

thaystg approved these changes May 8, 2025
View reviewed changes

tommcdon merged commit db0da94 into dotnet:mainMay 8, 2025

hoyosjs mentioned this pull request May 9, 2025
[main] Source code updates from dotnet/dotnet dotnet/diagnostics#5473
Merged

github-actionsBot locked and limited conversation to collaborators Jun 8, 2025