- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.2k
Expose the cDAC data contract for consumption on WASM. #120921
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
Conversation
| Tagging subscribers to this area: @mangod9 | 
Co-authored-by: Jeremy Koritzinsky <[email protected]>
There was a problem hiding this 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 enables the cDAC (Contract Data Access) data contract descriptor to be exposed and consumed on WebAssembly (WASM) platforms. Previously, the contract descriptor functionality was conditionally excluded from WASM builds.
Key changes:
- Removed conditional exclusion of GC_DESCRIPTORcompilation flag for WASM targets
- Added WASM-specific export function to expose the contract descriptor symbol
- Refactored contract descriptor structure definition into a shared header file
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description | 
|---|---|
| src/coreclr/vm/wks/CMakeLists.txt | Enabled GC_DESCRIPTORcompilation flag for WASM by removing the conditional exclusion | 
| src/coreclr/interpreter/CMakeLists.txt | Minor CMake syntax cleanup (endif formatting) | 
| src/coreclr/dlls/mscoree/exports.cpp | Added WASM-specific export function to expose contract descriptor symbol using Emscripten's keepalive attribute | 
| src/coreclr/dlls/mscoree/coreclr/CMakeLists.txt | Simplified CMake configuration by removing conditional variables and reordering link dependencies for consistency | 
| src/coreclr/debug/datadescriptor-shared/inc/contract-descriptor.h | Created new shared header file defining the ContractDescriptorstructure | 
| src/coreclr/debug/datadescriptor-shared/contractdescriptorstub.c | Removed duplicate structure definition, now includes shared header | 
| src/coreclr/debug/datadescriptor-shared/contractconfiguration.h.in | Added include for shared contract descriptor header | 
| src/coreclr/debug/datadescriptor-shared/contract-descriptor.c.in | Removed duplicate structure definition and unnecessary include, now uses shared header | 
| src/coreclr/clrdatadescriptors.cmake | Added include directory configuration for the new shared header location | 
| I don't know anything about cDAC but the changes in here look fine to me. Not comfortable giving it a green check until someone who knows the area better takes a look. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cDAC changes look good to me. I don't have as much insight into the other build changes.
Running the runtime-diagnostics pipeline since it was excluded from these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pipeline passed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| @AaronRobinsonMSFT this change broke windows-x86 Release Libraries_NET481 pipeline it seems:  | 
This is the first step in consume the cDAC scenario on WASM.
Removes duplication of the
ContractDescriptordata structure.Links together the cdac contracts so include directories flow correctly.
Exports a method during WASM builds so the Node.js APIs can acquire the
address of the data contract.