Skip to content

ARM64_32 Debug Mode - #7012

Merged
rolfbjarne merged 3 commits into
dotnet:masterfrom
lewurm:watch4-debug-mode
Sep 24, 2019
Merged

ARM64_32 Debug Mode#7012
rolfbjarne merged 3 commits into
dotnet:masterfrom
lewurm:watch4-debug-mode

Conversation

@lewurm

@lewurm lewurm commented Sep 17, 2019

Copy link
Copy Markdown
Contributor

Depends on mono/mono#16886

Contributes to mono/mono#10641

@lewurm
lewurm requested a review from rolfbjarne September 17, 2019 11:36
@lewurm
lewurm requested a review from spouliot as a code owner September 17, 2019 11:36
@spouliot spouliot added the do-not-merge Do not merge this pull request label Sep 17, 2019

@spouliot spouliot left a comment

Copy link
Copy Markdown
Contributor

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

@rolfbjarne rolfbjarne left a comment

Copy link
Copy Markdown
Member

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`).

Comment thread tools/mtouch/mtouch.cs Outdated
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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).

@lewurm lewurm Sep 17, 2019

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

Comment thread tools/mtouch/mtouch.cs Outdated
} 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.");

@lewurm lewurm Sep 17, 2019

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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.

Comment thread tools/mtouch/mtouch.cs Outdated
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
Copy Markdown
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
Copy Markdown
Contributor

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

@lewurm
lewurm requested a review from rolfbjarne September 23, 2019 19:57
@monojenkins

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

Build failure
Build failed or was aborted

🔥 Build failed 🔥

@rolfbjarne

Copy link
Copy Markdown
Member

build

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

Copy link
Copy Markdown
Contributor

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 dotnet:master Sep 24, 2019
@lewurm

lewurm commented Sep 24, 2019

Copy link
Copy Markdown
Contributor Author

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.

5 participants