Skip to content

[cDAC] Fixes for 32bit platforms #116720

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

Merged
merged 11 commits into from
Jun 19, 2025
Merged

[cDAC] Fixes for 32bit platforms #116720

merged 11 commits into from
Jun 19, 2025

Conversation

max-charlamb
Copy link
Contributor

@max-charlamb max-charlamb commented Jun 16, 2025

DAC

  • DAC GetThreadData(...) modified to use HOST_CDADDR instead of PTR_HOST_TO_TADDR for firstNestedException

cDAC

  • Fixed issue with CodePointerFromAddress on ARM32 when handling TargetCodePointer.Null. The thumb bit should not be added.
  • IXCLRDataStackWalk.Request properly converts frameAddr to a ClrDataAddress
  • Legacy API ConversionExtensions
    • Added option for ClrDataAddress extension ToTargetPointer to override the bounds check. This allows malformed (non-sign extended) ClrDataAddress to be converted. The DAC has the same behavior on legacy APIs where consumers may pass in non-sign extended values.
    • Added ClrDataAddress extension to convert ToTargetCodePointer
    • Added TargetCodePointer extension ToAddress to remove the Arm32 thumb bit. Note, we already have an existing implementation of this in Microsoft.Diagnostics.DataContractReader.CodePointerUtils. This is not exposed through a contract interface and probably should be kept internal. I wasn't sure where something like this should be exposed. I'm open to suggestions so that we only have one implementation. For now, I thought it would be safe to add a 'legacy' copy which can be removed without breaking a contract.
  • Fixed several sign extension issues in SOSDacImpl.cs
  • Added pretty printing to some debug asserts in SOSDacImpl.cs
  • Modified compile-native.proj to allow building the cDAC on Arm32.

@Copilot Copilot AI review requested due to automatic review settings June 16, 2025 22:38
Copy link
Contributor

Tagging subscribers to this area: @steveisok, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

Copilot

This comment was marked as outdated.

@max-charlamb
Copy link
Contributor Author

/azp run runtime-diagnostics

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@max-charlamb max-charlamb requested a review from Copilot June 17, 2025 21:11
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes issues related to 32-bit platforms in DAC and cDAC components by adjusting pointer conversions and sign extension logic, as well as updating build configuration for Arm32 support. Key changes include:

  • Updating pointer conversion methods (e.g. using ToTargetCodePointer and ToAddress) in SOSDacImpl.cs and related files.
  • Enhancing legacy API conversions and adding override options for sign extension in ConversionExtensions.cs.
  • Modifying build configuration in compile-native.proj to support Arm32 builds.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/native/managed/compile-native.proj Adjusted condition for SupportedNativeAotComponents to allow building on Arm32
src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs Updated pointer conversion calls and enhanced debug assertions with formatted messages
src/native/managed/cdac/mscordaccore_universal/Legacy/ConversionExtensions.cs Introduced an override flag for ToTargetPointer and improved ToAddress logic
src/native/managed/cdac/mscordaccore_universal/Legacy/ClrDataStackWalk.cs Updated conversion call to use ToClrDataAddress
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/CodePointerUtils.cs Added null-check for TargetPointer in CodePointerFromAddress
src/coreclr/debug/daccess/request.cpp Replaced PTR_HOST_TO_TADDR with HOST_CDADDR for firstNestedException
Comments suppressed due to low confidence (2)

src/native/managed/compile-native.proj:26

  • Consider adding a comment explaining why support for x86 and traditional ARM was removed in this condition to clarify the intended supported architectures for NativeAOT components.
        <SupportsNativeAotComponents Condition="'$(SupportsNativeAotComponents)' == '' and ('$(TargetArchitecture)' == 'armel' or '$(TargetArchitecture)' == 'riscv64')">false</SupportsNativeAotComponents>

src/native/managed/cdac/mscordaccore_universal/Legacy/ConversionExtensions.cs:12

  • [nitpick] Consider adding a brief comment explaining the derivation or purpose of Arm32ThumbBit to improve clarity for future maintainers.
    private const uint Arm32ThumbBit = 1;

@max-charlamb
Copy link
Contributor Author

/ba-g Deadletter failures on mono unrelated to PR changes

@max-charlamb max-charlamb merged commit a5d7490 into main Jun 19, 2025
144 of 150 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants