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.

Sign up for GitHub

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

Jump to bottom

[wasm] Unmanaged structs are considered blittable if module has DisableRuntimeMarshallingAttribute #73310

Merged
merged 18 commits into from
Aug 15, 2022
Merged

[wasm] Unmanaged structs are considered blittable if module has DisableRuntimeMarshallingAttribute #73310

merged 18 commits into from
Aug 15, 2022

Conversation

maraf
Copy link
Member

@maraf maraf commented Aug 3, 2022 •
edited by radical
Loading

Consider all types in pinvokes from assembly marked with DisableRuntimeMarshallingAttribute as blittable.
Based on API proposal in #60639.

  • PInvokeTableGenerator: Avoid crash when processing unmanaged callbacks with function pointers.

Fixes #61146.

@maraf maraf added arch-wasm WebAssembly architecture area-Interop-mono labels Aug 3, 2022
@maraf maraf added this to the 7.0.0 milestone Aug 3, 2022
@maraf maraf self-assigned this Aug 3, 2022
@maraf
Copy link
Member Author

maraf commented Aug 3, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ghost
Copy link

ghost commented Aug 3, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Consider all types from assembly marked with DisableRuntimeMarshallingAttribute as blittable.
Based on #60639.

Fixes #61146.

Author: maraf
Assignees: maraf
Labels:

arch-wasm, area-Interop-mono
Milestone: 7.0.0

@jkoritzinsky
Copy link
Member

This is not entirely true. An unmanaged struct defined in an assembly with DisableRuntimeMarshalling isn't necessarily blittable if it is used in a P/Invoke in an assembly without DisableRuntimeMarshallingAttribute.

@maraf maraf force-pushed the WasmPInvokeGeneratorDisableRuntimeMarshalling branch from e32a7b8 to 4e044cf Compare August 4, 2022 07:51
@maraf
Copy link
Member Author

maraf commented Aug 4, 2022

This is not entirely true. An unmanaged struct defined in an assembly with DisableRuntimeMarshalling isn't necessarily blittable if it is used in a P/Invoke in an assembly without DisableRuntimeMarshallingAttribute.

Thanks!
Must both struct and p/invoke assemblies be marked with DisableRuntimeMarshallingAttribute?

@jkoritzinsky
Copy link
Member

Only the P/Invoke assembly must be marked with the attribute.

maraf
maraf commented Aug 5, 2022
View reviewed changes
Copy link
Member Author

@maraf maraf left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Look for the DisableRuntimeMarshallingAttribute on the assembly where pinvoke method is declared.
Mistake...

@maraf
Copy link
Member Author

maraf commented Aug 5, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@maraf maraf marked this pull request as ready for review August 5, 2022 15:54
@maraf
Copy link
Member Author

maraf commented Aug 5, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Aug 5, 2022
Closed
Closed
@maraf
Copy link
Member Author

maraf commented Aug 9, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@maraf
Copy link
Member Author

maraf commented Aug 9, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@maraf @radical
Update src/tasks/WasmAppBuilder/PInvokeTableGenerator.cs
2b148dd 
Co-authored-by: Ankit Jain <radical@gmail.com>
@maraf
Copy link
Member Author

maraf commented Aug 12, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@maraf
Copy link
Member Author

maraf commented Aug 12, 2022 •
edited
Loading

Failures don't seem relevant.

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-73310-merge-6827ac8318034a2eb2/Workloads-.WasmTemplateTests/1/console.88ad2f22.log?helixlogtype=result

@maraf maraf requested a review from radical August 12, 2022 12:37
@radical
Copy link
Member

radical commented Aug 14, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member

radical commented Aug 15, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member

radical commented Aug 15, 2022

The wasm EAT/AOT test failures are #73932 .
The Perftracing build failure is #73933 .

@radical
Copy link
Member

radical commented Aug 15, 2022

The Linux_musl x64 coreclr failure is #73930 .

@radical radical merged commit 058bed7 into dotnet:main Aug 15, 2022
@maraf maraf deleted the WasmPInvokeGeneratorDisableRuntimeMarshalling branch August 15, 2022 08:55
kasperk81
kasperk81 reviewed Aug 15, 2022
View reviewed changes
src/tasks/WasmAppBuilder/PInvokeTableGenerator.cs
{
_assemblyDisableRuntimeMarshallingAttributeCache[assembly] = value = assembly
.GetCustomAttributesData()
.Any(d => d.AttributeType.Name == "DisableRuntimeMarshallingAttribute");
Copy link
Contributor

Choose a reason for hiding this comment

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

can use nameof(DisableRuntimeMarshallingAttribute) with using System.Runtime.CompilerServices;

Copy link
Member Author

Choose a reason for hiding this comment

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

The assembly is also compiled for net47 and the attribute is not available there.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kasperk81 kasperk81 kasperk81 left review comments

@radical radical radical approved these changes

@lambdageek lambdageek Awaiting requested review from lambdageek

Assignees

@maraf maraf

Labels
arch-wasm WebAssembly architecture area-Interop-mono
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

(Unmanaged) Structs are not considered blittable by Emscripten / Blazor WASM AOT
5 participants
@maraf @jkoritzinsky @radical @lambdageek @kasperk81