Skip to content

Conversation

kg
Copy link
Member

@kg kg commented May 8, 2025

  • Adds a custom opcode for pinvoke calls that don't rely on a marshaling wrapper
  • Adds a custom C++ library with some functions to call via pinvoke for testing
  • Adds a couple basic tests for p/invoke, with more commented out that don't work yet
  • Flips the meaning of the helper tag bit (this tag bit will be removed in a followup PR)

@kg
Copy link
Member Author

kg commented May 8, 2025

cc @BrzVlad @janvorli
Main remaining problem here is the check inside the ndirect import worker. I don't really know what the right fix is, other than updating it to be happy seeing an interpreter frame.

@janvorli
Copy link
Member

@AaronRobinsonMSFT all the things you've commented on are from my WIP PR that I am still cleaning up, Katelyn has built her stuff on top of that.

@kg kg mentioned this pull request May 14, 2025
65 tasks
@kg kg force-pushed the interp-pinvoke branch from a1804ae to 1adfbab Compare May 19, 2025 13:42
@kg
Copy link
Member Author

kg commented May 20, 2025

The working directory change is controversial, so I'm open to changing it around and making InterpreterTester be the one that builds the dll instead.

IMO Interpreter.dll should be built alongside pinvoke.dll since that's the actual dependency, and we should be running it where-it-was-built instead of in InterpreterTester's artifacts dir. IIRC msbuild has never been able to express 'when foo.csproj is referenced by bar.csproj, copy these satellite files along with foo.dll'.

I like being able to directly run Interpreter.dll with corerun in the debugger and have it work correctly without having to know to set a special cwd. But I can see the argument for doing it the other way. This will fail in a less confusing way once we have EH, at least - at that point you would see the dll not found error and be able to figure out what's wrong pretty quickly.

@kg kg marked this pull request as ready for review May 20, 2025 19:19
@kg kg requested a review from BrzVlad as a code owner May 20, 2025 19:19
@kg kg changed the title [WIP] Interpreter P/Invoke support Interpreter P/Invoke support May 21, 2025
@kg kg force-pushed the interp-pinvoke branch from bd0ead7 to 75a8b42 Compare May 22, 2025 16:20
@janvorli
Copy link
Member

@AaronRobinsonMSFT do you have any comments? I'd like to get this change in soon.

Copy link
Contributor

Tagging subscribers to this area: @BrzVlad, @janvorli, @kg
See info in area-owners.md if you want to be subscribed.

@kg kg added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 12, 2025
@kg kg force-pushed the interp-pinvoke branch from 007a300 to 71b4c6a Compare June 12, 2025 07:46
@kg kg removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Jun 12, 2025
@kg
Copy link
Member Author

kg commented Jun 12, 2025

Everything works now as long as the prestub for the pinvoke doesn't throw (due to a missing entry point). If the pinvoke throws, our managed catch block runs (in the interpreter, I think) and then the prestub's catch block runs and asserts because there's no exception. It seems like we're probably missing an InlinedCallFrame for the pinvoke like jan mentioned in a chat, which suggests we need a dedicated call opcode for pinvokes even though a regular call mostly works.

kg added 7 commits July 15, 2025 07:54
Marshaling wrappers work so enable test for them

Separate call opcode for unmarshaled pinvokes

Cleanup

Do inlined pinvoke for CALL_PINVOKE opcode

Update pinvoke test for new behavior
…lpers are compatible with arm32 thumb

Checkpoint pinvoke changes
Encapsulate inlined callframe creation
@kg kg force-pushed the interp-pinvoke branch from 5b105c8 to de173d5 Compare July 15, 2025 15:06
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli
Copy link
Member

/ba-g the Android build timeouts in all recent PRs.

@janvorli janvorli merged commit 19a9b70 into dotnet:main Jul 15, 2025
102 of 105 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants