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

ARM64_32 Debug Mode #7012

Merged
merged 3 commits into from Sep 24, 2019
Merged

ARM64_32 Debug Mode #7012

merged 3 commits into from Sep 24, 2019

Conversation

lewurm
Copy link
Contributor

@lewurm lewurm commented Sep 17, 2019

Depends on mono/mono#16886

Contributes to mono/mono#10641

@spouliot spouliot added the do-not-merge Do not merge this pull request label Sep 17, 2019
Copy link
Contributor

@spouliot spouliot left a comment

Choose a reason for hiding this comment

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

set do-not-merge label since it depends on another PR, remove it once it's merged

Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

You're targeting themono-2019-08 branch, but we've already merged that branch into master (so you can target master directly with your PR`).

@@ -372,7 +372,12 @@ public static string GetAotCompiler (Application app, bool is64bits)
return Path.Combine (cross_prefix, "bin", "arm-darwin-mono-sgen");
}
case ApplePlatform.WatchOS:
return Path.Combine (cross_prefix, "bin", "armv7k-unknown-darwin-mono-sgen");
/* Use arm64_32 cross only for Debug mode */
if (app.IsArchEnabled (Abi.ARM64_32) && !app.EnableLLVMOnlyBitCode) {
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong: when building for both armv7k and arm64_32 this if condition will be true when building for either architecture, not only arm64_32.

The function doesn't have the information it needs, it needs to be passed the Abi as wells.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, thanks! Right, because MtouchArch can be set to ARMv7k, ARM64_32. However MtouchExtraArgs cannot be set separately; --interpreter will not work for ARMv7k.

In terms of user experience, any suggestion what we should do here? Maybe do not require the user to set --interpreter explicitly in MtouchExtraArgs, but force it in mtouch when ARM64_32 + Debug Mode is detected?

Copy link
Member

Choose a reason for hiding this comment

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

force it in mtouch when ARM64_32 + Debug Mode is detected

I think this would be the best option for customers, but I think we'd want to emit a warning in this case explaining what we're doing (I don't like silently changing behavior), and explain that it's possible to get the normal AOT compiler by disabling Debug mode in the Debug configuration (I'm assuming that actually works).

That said, this won't be a problem if customers have device-specific builds turned on (which iirc is on by default in new projects).

Copy link
Contributor Author

@lewurm lewurm Sep 17, 2019

Choose a reason for hiding this comment

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

"normal" AOT aka. FullAOT for arm64_32 doesn't work currently, hence the interpreter. It would only be needed for Debug Mode (even when the managed debugger is disabled), but since we can use the interpreter there, I'm not sure if we will ever need to do the work to support FullAOT.

Copy link
Member

Choose a reason for hiding this comment

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

"normal" AOT aka.

Right now people can use Debug mode and it works, except that you can't debug. My concern is that if someone runs into a bug in the interpreter, they should be able to stop using the interpreter somehow so that the app runs correctly on the watch, even though debugging doesn't work (they could at least use Console.WriteLines).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, FullAOT does not work at all for arm64_32 currently 😕

@monojenkins
Copy link
Collaborator

Build success
Build succeeded
API Diff (from stable)
API Diff (from PR only) (no change)
Generator Diff (no change)
Test run succeeded

@lewurm lewurm changed the base branch from mono-2019-08 to master September 17, 2019 15:08
@monojenkins
Copy link
Collaborator

Build failure
Build failed or was aborted

Build succeeded
🔥 Failed to create API Diff 🔥

@lewurm lewurm changed the title [mono-2019-08] ARM64_32 Debug Mode ARM64_32 Debug Mode Sep 17, 2019
@monojenkins
Copy link
Collaborator

Build success
Build succeeded
API Diff (from stable)
API Diff (from PR only) (no change)
Generator Diff (no change)
Test run succeeded

} else {
if (app.Platform == ApplePlatform.WatchOS && app.IsArchEnabled (Abi.ARM64_32)) {
if (app.IsArchEnabled (Abi.ARMv7k)) {
throw ErrorHelper.CreateError (99, "Please use device builds on WatchOS.");
Copy link
Contributor Author

@lewurm lewurm Sep 17, 2019

Choose a reason for hiding this comment

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

I didn't manage to disable device-specific builds, but I believe this would be the condition.

Also: To what does the error code map to? Is there some guideline?

Copy link
Member

Choose a reason for hiding this comment

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

mtouch error codes are listed and documented here: https://github.com/xamarin/xamarin-macios/blob/master/docs/website/mtouch-errors.md#mt0142-cannot-find-the-assembly-assembly-passed-as-an-argument-to---interpreter

it looks like the next available code would be 143.

Also you're not checking for debug mode here, so this will trigger for release builds as well.

if (app.IsArchEnabled (Abi.ARMv7k)) {
throw ErrorHelper.CreateError (99, "Please use device builds on WatchOS.");
} else {
ErrorHelper.Warning (99, "ARM64_32 Debug mode requires --interpreter[=all], forcing it.");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To what does the warning code map to? Is there some guideline?

@monojenkins
Copy link
Collaborator

Build success
Build succeeded
API Diff (from stable)
API Diff (from PR only) (no change)
Generator Diff (no change)
Test run succeeded

@monojenkins
Copy link
Collaborator

Build failure
Build succeeded
API Diff (from stable)
🔥 Failed to compare API and create generator diff 🔥
    Failed to update apidiff references
    Search for Comparing API & creating generator diff in the log to view the complete log.
🔥 Test run failed 🔥

@monojenkins
Copy link
Collaborator

Build failure
Build failed or was aborted

🔥 Build failed 🔥

@rolfbjarne
Copy link
Member

build

@lewurm lewurm removed the do-not-merge Do not merge this pull request label Sep 24, 2019
@monojenkins
Copy link
Collaborator

Build success
Build succeeded
API Diff (from stable)
API Diff (from PR only) (no change)
Generator Diff (no change)
Test run succeeded

@rolfbjarne rolfbjarne merged commit 26ec282 into xamarin:master Sep 24, 2019
@lewurm
Copy link
Contributor Author

lewurm commented Sep 24, 2019

Leaving a papertrail here:

Depends on mono/mono#16886

This is in xamarin-macios/master now, it went in with this bump: #7044

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-mention Deserves a mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants