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

Fix wrong AOT mode symbol being set #3940

Merged
merged 2 commits into from
Nov 21, 2019

Conversation

mathieubourgeois
Copy link
Contributor

Since decfbcc, the runtime code seems to expect that mono_aot_mode_name is a linked symbol that's written by the build system. It also expects it to be a string that's written, and the code tries to understand in what AOT mode it's supposed to be based on the first character (normal, hybrid or full). However, we currently convert the AotMode enum to a uint before converting it to a string, meaning instead of writing normal, we write 1. This causes issues with people receiving the following line in their logs :

W/monodroid(7449): Unknown Mono AOT mode: 1

This can be confirmed by analyzing the environment.{env}.s files generated in the intermediary folder :

 .section .rodata..L.str.2,"aMS",@progbits,1
 .type .L.str.2, @object
.L.str.2:
 .asciz "1"
 .size .L.str.2, 2
 .section .data.mono_aot_mode_name,"aw",@progbits
 .global mono_aot_mode_name
mono_aot_mode_name:
 .xword .L.str.2

Fix by making sure that we convert the enum to a string instead of the uint. Based on a cursory analysis of the code, this would not affect normal AOT mode, since the call to get_aot_mode seems to only be used for something if aot is not disabled and isn't "normal" (i.e. hybrid or full)

Update master from base repo
Since dotnet@decfbcc, the runtime code seems to expect that `mono_aot_mode_name` is a linked symbol that's written by the build system. It also expects it to be a string that's written, and the code tries to understand in what AOT mode it's supposed to be based on the first character (`n`ormal, `h`ybrid or `f`ull). However, we currently convert the AotMode enum to a `uint` before converting it to a string, meaning instead of writing `normal`, we write `1`. This causes issues with people receiving the following line in their logs :

`W/monodroid(7449): Unknown Mono AOT mode: 1`

This can be confirmed by analyzing the `environment.{env}.s` files generated in the intermediary folder :

```
 .section .rodata..L.str.2,"aMS",@progbits,1
 .type .L.str.2, @object
.L.str.2:
 .asciz "1"
 .size .L.str.2, 2
 .section .data.mono_aot_mode_name,"aw",@progbits
 .global mono_aot_mode_name
mono_aot_mode_name:
 .xword .L.str.2
```

Fix by making sure that we convert the enum to a string instead of the uint. Based on a cursory analysis of the code, this would not affect normal AOT mode, since the call to get_aot_mode seems to only be used for something if aot is not disabled and isn't "normal" (i.e. hybrid or full)
@mathieubourgeois
Copy link
Contributor Author

Just as a small addendum, I have also noticed that since 6be4bdb#diff-85c855eb9ec814df87fe0f2bf21b9311, it looks like the warning log has been inverted to if (aotMode == MonoAotMode::MONO_AOT_MODE_LAST), which means the log will probably complain that the AOT mode is wrong even if it's entirely correct (and will say it's right when it says it's wrong. I don't think that's intended.

@jonathanpeppers
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

grendello added a commit to grendello/xamarin-android that referenced this pull request Nov 20, 2019
Context: dotnet#3940 (comment)

The check for unknown AOT mode was inverted. Fix by using the correct,
inequality, operator.
jonpryor pushed a commit that referenced this pull request Nov 21, 2019
Context: #3940 (comment)

The check for unknown AOT mode was inverted.  Fix by using the
correct, inequality, operator.
@jonpryor jonpryor merged commit 82d3240 into dotnet:master Nov 21, 2019
jonpryor pushed a commit that referenced this pull request Nov 21, 2019
Context: #3940 (comment)

The check for unknown AOT mode was inverted.  Fix by using the
correct, inequality, operator.
jonpryor pushed a commit that referenced this pull request Nov 21, 2019
Since commit decfbcc, the runtime code seems to expect that
`mono_aot_mode_name` is a linked symbol that's generated by the build
system, located in `libxamarin-app.so`.  The runtime code also
expects it to be a string that's written, and the code tries to
understand in what AOT mode it's supposed to be based on the first
character of the string: `n`ormal, `h`ybrid, or `f`ull).

However, we currently convert the `AotMode` enum to a `uint` before
converting it to a string, meaning instead of writing e.g. `normal`,
we instead write `1`.  This causes issues with people receiving the
following line in their logs :

        W/monodroid(7449): Unknown Mono AOT mode: 1

This can be confirmed by analyzing the `environment.{env}.s` files
generated in the intermediary folder :

         .section .rodata..L.str.2,"aMS",@progbits,1
         .type .L.str.2, @object
        .L.str.2:
         .asciz "1"
         .size .L.str.2, 2
         .section .data.mono_aot_mode_name,"aw",@progbits
         .global mono_aot_mode_name
        mono_aot_mode_name:
         .xword .L.str.2

Fix by making sure that we convert the enum to a string instead of a
`uint`.

Based on a cursory analysis of the code, this would not affect normal
AOT mode, since the call to `get_mono_aot_mode()` seems to only be
used for something if aot is not disabled and isn't "normal",
i.e. hybrid or full.
@brendanzagaeski
Copy link
Contributor

Release status update

A new Preview version has now been published that includes the fix from this item. The fix is not yet included in a Release version. I will update this item again when a Release version is available that includes the fix.

Fix included in Xamarin.Android 10.2.0.84.

Fix included on Windows in Visual Studio 2019 version 16.5 Preview 2. To try the Preview version that includes the fix, check for the latest updates in Visual Studio Preview.

Fix included on macOS in Visual Studio 2019 for Mac version 8.5 Preview 1. To try the Preview version that includes the fix, check for the latest updates on the Preview updater channel.

@brendanzagaeski
Copy link
Contributor

Release status update

A new Release version has now been published on Windows that includes the fix from this item. The fix is not yet published in a Release version on macOS. I will update this item again when a Release version is available on macOS that includes the fix.

Fix included in Xamarin.Android 10.2.0.100.

Fix included on Windows in Visual Studio 2019 version 16.5. To get the new version that includes the fix, check for the latest updates or install the latest version from https://visualstudio.microsoft.com/downloads/.

(Fix also included on macOS in Visual Studio 2019 for Mac version 8.5 Preview 1 and higher. To try the Preview version that includes the fix, check for the latest updates on the Preview updater channel.)

@brendanzagaeski
Copy link
Contributor

Release status update

A new Release version has now been published on macOS that includes the fix from this item.

Fix included in Xamarin.Android 10.2.0.100.

Fix included on macOS in Visual Studio 2019 for Mac version 8.5. To get the new version that includes the fix, check for the latest updates on the Stable updater channel.

(Fix also included on Windows in Visual Studio 2019 version 16.5 and higher. To get the new version that includes the fix, check for the latest updates or install the latest version from https://visualstudio.microsoft.com/downloads/.)

@github-actions github-actions bot locked and limited conversation to collaborators Jan 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants