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

[MonoAOTCompiler] more properties & custom WorkingDirectory #62725

Merged

Conversation

jonathanpeppers
Copy link
Member

Fixes: #56163

PR #58523 fixed something on Windows, but it didn't actually address
our issues on Android.

A directory name like foo Ümläüts fails:

  • mkdir 'foo Ümläüts' ; cd 'foo Ümläüts'
  • dotnet new android
  • dotnet build -c Release -p:RunAOTCompilation=true (adding
    -p:EnableLLVM=true complicates further)

The error:

Precompiling failed for C:\src\foo Ümläüts\obj\Release\android-arm64\linked\System.Private.CoreLib.dll: Error: Loaded assembly 'C:\src\foo ├£ml├ñ├╝ts\obj\Release\android-arm64\linked\System.Private.CoreLib.dll' doesn't match original file name 'C:\foo ▄mlΣⁿts\obj\Release\android-arm64\linked\System.Private.CoreLib.dll'. Set MONO_PATH to the assembly's location.

Reviewing the existing AOT implementation in Xamarin.Android, I found
out why Xamarin.Android works:

[AOT] response file obj\Release\120\aot\arm64-v8a\App36.dll\response.txt: --llvm "--aot=temp-path=obj\Release\120\aot\arm64-v8a\App36.dll,llvm-path=C:\Program Files\Microsoft Visual Studio\2022\Preview\MSBuild\Xamarin\Android,outfile=obj\Release\120\aot\arm64-v8a\libaot-App36.dll.so,msym-dir=obj\Release\120\aot\arm64-v8a,asmwriter,mtriple=aarch64-linux-android,tool-prefix=C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\aarch64-linux-android-,ld-name=ld.EXE,ld-flags=\"-LC:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\lib\gcc\aarch64-linux-android\4.9.x\";\"-LC:\Program Files (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm64\usr\lib\";\"C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\lib\gcc\aarch64-linux-android\4.9.x\libgcc.a\";\"C:\Program Files (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm64\usr\lib\libc.so\";\"C:\Program Files (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm64\usr\lib\libm.so\"" C:\Users\jopepper\source\repos\App36\App36\obj\Release\120\android\assets\shrunk\App36.dll
  1. Xamarin.Android passes relative paths. The foo Ümläüts
    directory name doesn't even come into play for some arguments.

  2. With LLVM, ld-flags contains a ;.

The existing code splits on ; and joins on ,:

var a = assemblyItem.GetMetadata("AotArguments");
if (a != null)
{
aotArgs.AddRange(a.Split(new char[]{ ';' }, StringSplitOptions.RemoveEmptyEntries));
}

So we lose any ; delimiters for the ld-flags value, they get
replaced by ,.

I think the solution here is:

  1. Add several missing properties to <MonoAOTCompiler/> so we don't
    have to rely on the %(AotArguments) item metadata. No splitting
    on ; would be required, ld-flags can be passed in and used as-is.

  2. Add a new WorkingDirectory property. When this is set, assume
    paths passed in might be relative -- and don't transform paths by
    calling Path.GetFullPath().

Lastly, I fixed a place where the UTF8 encoding wasn't passed when
MSBuild logging the response file.

These changes I tried to make in a way where this shouldn't break
other .NET workloads like wasm. If existing MSBuild targets call this
task (not using the new properties), the behavior should remain
unchanged.

I tested these changes by commiting a modified MonoAOTCompiler.dll:

dotnet/android#6562

I'm able to enable several AOT tests related to #56163.

Fixes: dotnet#56163

PR dotnet#58523 fixed something on Windows, but it didn't actually address
our issues on Android.

A directory name like `foo Ümläüts` fails:

* `mkdir 'foo Ümläüts' ; cd 'foo Ümläüts'`
* `dotnet new android`
* `dotnet build -c Release -p:RunAOTCompilation=true` (adding
  `-p:EnableLLVM=true` complicates further)

The error:

    Precompiling failed for C:\src\foo Ümläüts\obj\Release\android-arm64\linked\System.Private.CoreLib.dll: Error: Loaded assembly 'C:\src\foo ├£ml├ñ├╝ts\obj\Release\android-arm64\linked\System.Private.CoreLib.dll' doesn't match original file name 'C:\foo ▄mlΣⁿts\obj\Release\android-arm64\linked\System.Private.CoreLib.dll'. Set MONO_PATH to the assembly's location.

Reviewing the existing AOT implementation in Xamarin.Android, I found
out *why* Xamarin.Android works:

    [AOT] response file obj\Release\120\aot\arm64-v8a\App36.dll\response.txt: --llvm "--aot=temp-path=obj\Release\120\aot\arm64-v8a\App36.dll,llvm-path=C:\Program Files\Microsoft Visual Studio\2022\Preview\MSBuild\Xamarin\Android,outfile=obj\Release\120\aot\arm64-v8a\libaot-App36.dll.so,msym-dir=obj\Release\120\aot\arm64-v8a,asmwriter,mtriple=aarch64-linux-android,tool-prefix=C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\aarch64-linux-android-,ld-name=ld.EXE,ld-flags=\"-LC:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\lib\gcc\aarch64-linux-android\4.9.x\";\"-LC:\Program Files (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm64\usr\lib\";\"C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\lib\gcc\aarch64-linux-android\4.9.x\libgcc.a\";\"C:\Program Files (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm64\usr\lib\libc.so\";\"C:\Program Files (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm64\usr\lib\libm.so\"" C:\Users\jopepper\source\repos\App36\App36\obj\Release\120\android\assets\shrunk\App36.dll

1. Xamarin.Android passes *relative* paths. The `foo Ümläüts`
   directory name doesn't even come into play for some arguments.

2. With LLVM, `ld-flags` contains a `;`.

The existing code splits on `;` and joins on `,`:

https://github.com/dotnet/runtime/blob/25c207351c4f57cf2daa98caaf327a8b8d83edb8/src/tasks/AotCompilerTask/MonoAOTCompiler.cs#L505-L509

So we lose any `;` delimiters for the `ld-flags` value, they get
replaced by `,`.

I think the solution here is:

1. Add several missing properties to `<MonoAOTCompiler/>` so we don't
   have to rely on the `%(AotArguments)` item metadata. No splitting
   on `;` would be required, `ld-flags` can be passed in and used as-is.

2. Add a new `WorkingDirectory` property. When this is set, assume
   paths passed in might be relative -- and don't transform paths by
   calling `Path.GetFullPath()`.

Lastly, I fixed a place where the UTF8 encoding wasn't passed when
MSBuild logging the response file.

These changes I tried to make in a way where this shouldn't break
other .NET workloads like wasm. If existing MSBuild targets call this
task (not using the new properties), the behavior should remain
unchanged.

I tested these changes by commiting a modified `MonoAOTCompiler.dll`:

dotnet/android#6562

I'm able to enable several AOT tests related to dotnet#56163.
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@steveisok
Copy link
Member

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok
Copy link
Member

The runtime-manual failures are unrelated. The AOT legs for wasm and iOS were successful.

@radical and @akoeplinger please review.

Comment on lines +194 to +212
/// <summary>
/// Passes additional, custom arguments to --aot
/// </summary>
public string? AotArguments { get; set; }

/// <summary>
/// Passes temp-path to the AOT compiler
/// </summary>
public string? TempPath { get; set; }

/// <summary>
/// Passes ld-name to the AOT compiler, for use with UseLLVM=true
/// </summary>
public string? LdName { get; set; }

/// <summary>
/// Passes ld-flags to the AOT compiler, for use with UseLLVM=true
/// </summary>
public string? LdFlags { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it might be better to have a ITaskItem? CommonAotArguments with metadata which act as the key-value pairs to pass to --aot.

Copy link
Member Author

Choose a reason for hiding this comment

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

LdFlags will contain a ; on Android. So it seems like using item metadata would always have an issue?

Our usage is now:

<MonoAOTCompiler
    Triple="$(_Triple)"
    ToolPrefix="$(_ToolPrefix)"
    MsymPath="$(_MsymPath)"
    Assemblies="@(_MonoAOTAssemblies)"
    CompilerBinaryPath="$(_MonoAOTCompilerPath)"
    DisableParallelAot="$(_DisableParallelAot)"
    IntermediateOutputPath="$(IntermediateOutputPath)"
    LibraryFormat="So"
    Mode="$(AndroidAotMode)"
    OutputDir="$(IntermediateOutputPath)aot\"
    OutputType="Library"
    UseAotDataFile="false"
    UseLLVM="$(EnableLLVM)"
    LLVMPath="$(_LLVMPath)"
    LdName="$(_LdName)"
    LdFlags="$(_LdFlags)"
    WorkingDirectory="$(MSBuildProjectDirectory)"
    AotArguments="$(AndroidAotAdditionalArguments)">
  <Output TaskParameter="CompiledAssemblies" ItemName="_AotCompiledAssemblies" />
  <Output TaskParameter="FileWrites"         ItemName="FileWrites" />
</MonoAOTCompiler>

But maybe $(AndroidAotAdditionalArguments) could be passed in a different way? What would the XML look like for ITaskItem? CommonAotArguments?

Copy link
Member

Choose a reason for hiding this comment

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

LdFlags will contain a ; on Android. So it seems like using item metadata would always have an issue?

We won't split the metadata on CommonAotArguments.
As for the metadata names, we could either use the real argument name, like <ld-flags>foo; bar</ld-flags>, or we could have <LdFlags>foo; bar</LdFlags> and convert to the real name in the task.

  <CommonAotArguments Include="common">
    <ld-flags>foo;bar</ld-flags>
    <ld-name>xyz</ld-name>
  </CommonAotArguments>

or

  <CommonAotArguments Include="common">
    <LdFlags>foo;bar</LdFlags>
    <LdName>xyz</LdName>
  </CommonAotArguments>

And you could have a <PassVerbatim>..</PassVerbatim> metadata (or better name) for just any additional string to be passed to --aot.

Feel free to push back, I'm just thinking aloud.

Copy link
Member Author

Choose a reason for hiding this comment

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

The weird part about using @(CommonAotArguments), is that you put Include="" with any random word inside. You can't leave out Include, or that would be an empty item group. I also like the idea of having C# properties, as it checks for typos -- you'll get an MSBuild error if you put LdlFlags instead of LdFlags.

Adding something like @(CommonAotArguments) might be ok if there is some unknown set. At least for Android, these changes now include every parameter we use. Are there many more not represented here?

@jonathanpeppers jonathanpeppers merged commit b2f0441 into dotnet:main Dec 15, 2021
@jonathanpeppers jonathanpeppers deleted the monoaotcompiler-android-fixes branch December 15, 2021 17:15
@jonathanpeppers
Copy link
Member Author

@steveisok how would we get this on the mauipre branch? Is there a magic comment to backport it?

@steveisok
Copy link
Member

/backport to release/6.0-maui

@github-actions
Copy link
Contributor

Started backporting to release/6.0-maui: https://github.com/dotnet/runtime/actions/runs/1583847353

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Dec 20, 2021
Fixes: dotnet#6520
Context: dotnet/runtime#56163
Context: dotnet/runtime#62725

This fixes AOT builds for project names like `foo Ümläüts`, and
LLVM-related options are no longer space delimited.

Previously, `<MonoAOTCompiler/>` managed its own `WorkingDirectory` to
the location that `.dll` input files were located. The problem with
this, is that *other* paths like `temp-path`, etc. would then need to
be full paths. Full paths containing characters like `Ümläüts` would
break!

Additionally, `%(AotArguments)` metadata is split on `;` and joined on
`,`. And since `ld-flags` contains a `;`, we would lose it. So we
delimited by the space character, which also breaks if directories
contain a space!

To solve these issues:

* Added strongly-typed properties to the `<MonoAOTCompiler/>` MSBuild
  task. We no longer have to put as many things in `%(AotArguments)`
  on each assembly. Only assembly-specific settings go there now, like
  the path to AOT profiles. Right now the "main assembly" has no
  profile at all, and the rest get built-in profiles.

* Added a `WorkingDirectory` property, which we pass in
  `$(MSBuildProjectDirectory)` for the current project directory.

After these changes, I could remove many temporary comments around
dotnet/runtime#56163, as the tests work properly now.
jonpryor pushed a commit to dotnet/android that referenced this pull request Jan 5, 2022
Fixes: #6520

Context: dotnet/runtime#56163
Context: dotnet/runtime#62725

This fixes AOT builds for project names like `foo Ümläüts`, and
LLVM-related options are no longer space delimited.

Previously, the `<MonoAOTCompiler/>` task managed its own
`WorkingDirectory` to the location that `.dll` input files were
located.  The problem with this is that *other* paths like
`temp-path` would then need to become full paths.

Full paths containing characters like `Ümläüts` would break!

Additionally, `%(AotArguments)` metadata is split on `;` and joined
on `,`.  As `ld-flags` contained `;`, we would lose it.  To work
around that, we delimited by the space character, which breaks if
directories contain a space!

	[System.Runtime.dll] Exec (with response file contents expanded) in …\android\obj\Release\net6.0-android\android-arm64\linked:
	  MONO_PATH=…\android\obj\Release\net6.0-android\android-arm64\linked;
	  MONO_ENV_OPTIONS=
	  C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.android-arm64\6.0.0-rc.2.21480.5\Sdk\..\tools\mono-aot-cross.exe --debug --llvm
	  "--aot=asmwriter,mtriple=aarch64-linux-android,tool-prefix=C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\aarch64-linux-android-,ld-name=ld.EXE,ld-flags=-LC:\Program\ Files\ (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\lib\gcc\aarch64-linux-android\4.9.x -LC:\Program\ Files\ (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm64\usr\lib C:\Program\ Files\ (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\lib\gcc\aarch64-linux-android\4.9.x\libgcc.a C:\Program\ Files\ (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm64\usr\lib\libc.so C:\Program\ Files\ (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm64\usr\lib\libm.so,temp-path=…\android\obj\Release\net6.0-android\android-arm64\aot\arm64-v8a\System.Runtime,nodebug,llvm-path=C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.android-arm64\6.0.0-rc.2.21480.5\Sdk\..\tools,outfile=…\android\obj\Release\net6.0-android\android-arm64\aot\System.Runtime.dll.so,llvm-outfile=…\android\obj\Release\net6.0-android\android-arm64\aot\System.Runtime.dll-llvm.o"
	  "System.Runtime.dll"
	…
	C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\aarch64-linux-android-ld.EXE:
	  cannot find Files\: No such file or directory

To solve these issues:

  * Added strongly-typed properties to the `<MonoAOTCompiler/>`
    MSBuild task.  We no longer have to put as many things into
    `%(AotArguments)` on each assembly.  Only assembly-specific
    settings go there now, such as the path to AOT profiles.
    Right now the "main assembly" has no profile at all, and the rest
    get built-in profiles.

  * Added a `MonoAOTCompiler.WorkingDirectory` property, for which
    we pass in `$(MSBuildProjectDirectory)` for the current project
    directory.

After these changes, I could remove many temporary comments around
dotnet/runtime#56163, as the tests work properly now.
@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 2022
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.

.NET 6 Android AOT fails on certain folder names
3 participants