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

[mono] Include filename in Invalid image error messages #32560

Merged
merged 8 commits into from
Mar 3, 2020

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Feb 19, 2020

Fixes #31649
Fixes #31650

Testing:
Reproduced Error via
<Runtime Repo Root>/build.sh /p:RuntimeFlavor=mono
Removing [ActiveIssue("https://github.com/dotnet/runtime/issues/31650", TestRuntimes.Mono)] in AssemblyTests.cs
Running dotnet msbuild /t:RebuildAndTest /p:XunitMethodName=System.Reflection.Tests.AssemblyTests.LoadFile_NoSuchPath_ThrowsFileNotFoundException

Fixing issue resulted in no errors.

Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

I think we should use mono_error_set_simple_file_not_found for some of these FileNotFoundExceptions. It seems to already include the file name in its error message and includes a nicer error message.

Additionally, we should make the change to include the filename with the error message in more than just the function calls failing the tests. There are some cases where it doesn't make sense to do so because we have an in-memory assembly (e.g. mono_error_set_bad_image_by_name (error, "In memory assembly", "0x%p", assembly_data);) but otherwise we should try and include it across the board. For FileNotFound, the main candidates are the two calls in icall.c, but we have a bunch of invalid image calls in metadata.c and aot-runtime.c that seem like they would benefit from this.

@mdh1418 mdh1418 requested a review from SamMonoRT as a code owner February 20, 2020 19:58
@danmoseley
Copy link
Member

Please add exactly one area label if the bot doesn't. It trains the bot

@CoffeeFlux CoffeeFlux self-assigned this Feb 28, 2020
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just a few little things to fix up.

src/mono/mono/metadata/appdomain.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/icall.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/icall.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/icall.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/metadata.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/metadata.c Outdated Show resolved Hide resolved
src/mono/mono/mini/aot-runtime.c Outdated Show resolved Hide resolved
src/mono/mono/mini/aot-runtime.c Outdated Show resolved Hide resolved
@mdh1418 mdh1418 force-pushed the mdhwang/fix_runtime_issue_31650 branch from 5357328 to f15a4a5 Compare March 2, 2020 19:12
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

So close! Watch out for whitespace changes.

src/mono/mono/metadata/metadata.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/metadata.c Outdated Show resolved Hide resolved
@mdh1418 mdh1418 force-pushed the mdhwang/fix_runtime_issue_31650 branch from f15a4a5 to f57ff48 Compare March 2, 2020 19:50
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@mdh1418 mdh1418 merged commit 5371ef4 into dotnet:master Mar 3, 2020
@mdh1418 mdh1418 deleted the mdhwang/fix_runtime_issue_31650 branch March 3, 2020 16:14
mdh1418 added a commit that referenced this pull request Mar 3, 2020
akoeplinger pushed a commit that referenced this pull request Mar 3, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants