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][debugger] Fix method overload resolution for arguments of type array in debugger's eval #97776

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Jan 31, 2024

In the EvaluateObjectByNonIntLocals test we were trying to evaluate an expression like this s[arr].
We are trying to call get_Item method that expects an object as a parameter and this is not correct.
With the PR we are calling the get_Item that expects an array as a parameter .

Error on CI:
https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-97752-merge-dc23193aa5e44908bf/chrome-DebuggerTests.EvaluateOnCallFrameTests/1/console.e4d5ccf7.log?helixlogtype=result

Error message:
Unexpected runtime error/warning message detected: console.error: [MONO] * Assertion at /__w/1/s/src/mono/mono/component/debugger-agent.c:2018, condition ' not met`

@ghost
Copy link

ghost commented Jan 31, 2024

Tagging subscribers to this area: @thaystg
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix assertion on CI

Author: thaystg
Assignees: -
Labels:

area-Debugger-mono

Milestone: -

@thaystg thaystg changed the title [wasm][debugger] Do not try to invoke a method that expects a object passing an array [wasm][debugger] Fix method overload resolution for arguments of type array in debugger's eval Jan 31, 2024
@thaystg thaystg merged commit 45eccaf into dotnet:main Jan 31, 2024
17 checks passed
@radical
Copy link
Member

radical commented Jan 31, 2024

@thaystg just curious - how did this break, and why didn't it get caught by CI on a PR?

@thaystg
Copy link
Member Author

thaystg commented Jan 31, 2024

The test was passing on CI. But there was an assertion in the log. And Pavel asked me to take a look.

@radical
Copy link
Member

radical commented Jan 31, 2024

Is the link that you shared not from the same thing, as it shows a test failure?

Also, we have https://github.com/radical/runtime/blob/2d999cb443cb85d882a6dbfe9144178d70c6fbf3/src/mono/browser/debugger/DebuggerTestSuite/Inspector.cs#L269-L277 to catch any assertions that might get missed. Would that be usable here?

@radical
Copy link
Member

radical commented Jan 31, 2024

oh lol it did Unexpected runtime error/warning message detected: - that should have failed on the PR's CI then.

@thaystg
Copy link
Member Author

thaystg commented Jan 31, 2024

I remembered that you did it.
But it looks like it's not always catching it? Or something it was not always asserting. Locally I was able to reproduce 100% of my runs. maybe it was reading an information that was "valid" depending on the run.

@radical
Copy link
Member

radical commented Jan 31, 2024

Interesting. Maybe the format of the string different or something. Something to keep an eye out in the future I guess. thanks for the explanations!

@github-actions github-actions bot locked and limited conversation to collaborators Mar 2, 2024
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.

3 participants