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

[wasm] Don't log errors for unsupported cases in PInvokeCollector #113212

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

akoeplinger
Copy link
Member

After #112787 was merged we saw the runtime being relinked in the runtime tests which uncovered a couple cases where we were throwing an exception or logging an unactionable error in the PInvokeCollector. Log warnings instead and clean up the code.

Fixes #113025

After #112787 was merged we saw the runtime being relinked in the runtime tests which uncovered a couple cases where we were throwing an exception or logging an unactionable error in the PInvokeCollector. Log warnings instead and clean up the code.

Fixes #113025

Choose a reason for hiding this comment

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

PR Overview

This PR refines the error logging and exception handling in the PInvokeCollector and related icall signature generation logic to avoid throwing exceptions or logging unactionable errors.

  • Changes the icall signature processing to log warnings instead of throwing or erroring out for unsupported cases.
  • Updates logging formats and consistency in referencing method names.
  • Exposes TryIsMethodGetParametersUnsupported publicly in the PInvokeTableGenerator for external use.

Reviewed Changes

File Description
src/tasks/WasmAppBuilder/IcallTableGenerator.cs Renames AddSignature to AddSignatureForMethod and updates logging for unsupported method cases.
src/tasks/WasmAppBuilder/PInvokeCollector.cs Wraps method processing in a try-catch and refines error logging and callback handling.
src/tasks/WasmAppBuilder/PInvokeTableGenerator.cs Changes access of TryIsMethodGetParametersUnsupported to public and updates log messages for consistency.

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

@akoeplinger
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lewing
Copy link
Member

lewing commented Mar 6, 2025

Looks like some tests need to be fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants