Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IMAGE_FILE_MACHINE_ARM64X and IMAGE_FILE_MACHINE_ARM64EC #2858

Merged
merged 9 commits into from
Sep 19, 2024

Conversation

hoyosjs
Copy link
Member

@hoyosjs hoyosjs commented Feb 9, 2022

Co-authored-by: David Mason davmason@microsoft.com

@hoyosjs hoyosjs requested a review from a team as a code owner February 9, 2022 00:05
Copy link
Member

@davmason davmason left a comment

Choose a reason for hiding this comment

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

You can also get IMAGE_FILE_MACHINE_ARM64X and IMAGE_FILE_MACHINE_ARM64EC when you are debugging in x64 emulation on arm64 machines. For a complete fix you should add logic so when you are in SOS_TARGET_X64 and you see either of them then load the x64 machine

@hoyosjs
Copy link
Member Author

hoyosjs commented Feb 14, 2022

Then this is not going to work - if we can get such value in both arm64 and x64 scenarios we might need something different to identify process arch.

@mikem8361
Copy link
Member

I think it will work. If SOS is running on x64 (either real or emulated), both SOS_TARGET_AMD64 and SOS_TARGET_ARM64 are defined so putting the checks for those two processor types in the SOS_TARGET_ARM64 section should work.

@mikem8361
Copy link
Member

Should work the way Juan has it now.

@hoyosjs
Copy link
Member Author

hoyosjs commented Feb 14, 2022

ArchQuery/GetTargetMachine should return the target's process instruction architecture/machine (regardless of SOS's host). Right now it will return the ARM64 IMachine. I think David had mentioned there are cases where this value can be returned on x64 processes on arm64 machines. If that's true, this patch is incomplete. Otherwise, this fix seems fine as we define both targets for the x64 SOS.

@noahfalk
Copy link
Member

I am closing old PRs that have had no progress in a long time. You are welcome to resume the work any time and open a new PR.

@mikem8361
Copy link
Member

@hoyosjs can you review?

@mikem8361 mikem8361 dismissed davmason’s stale review September 13, 2024 17:04

The changes requested have been fixed.

@mikem8361 mikem8361 force-pushed the juhoyosa/sos-arm64ecx branch from 87d0620 to 756fb9d Compare September 14, 2024 22:34
src/dbgshim/debugshim.cpp Outdated Show resolved Hide resolved
src/SOS/SOS.Extensions/DebuggerServices.cs Show resolved Hide resolved
src/SOS/SOS.Extensions/TargetFromFromDebuggerServices.cs Outdated Show resolved Hide resolved
src/SOS/Strike/dbgengservices.cpp Outdated Show resolved Hide resolved
src/SOS/Strike/exts.h Outdated Show resolved Hide resolved
src/SOS/Strike/exts.cpp Outdated Show resolved Hide resolved
src/SOS/Strike/exts.cpp Outdated Show resolved Hide resolved
src/SOS/Strike/dbgengservices.cpp Outdated Show resolved Hide resolved
src/dbgshim/debugshim.cpp Outdated Show resolved Hide resolved
Copy link
Member Author

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

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

LGTM modulo the thing.

@mikem8361 mikem8361 merged commit 6f2e688 into main Sep 19, 2024
20 checks passed
@mikem8361 mikem8361 deleted the juhoyosa/sos-arm64ecx branch September 19, 2024 01:57
@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants