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.

Sign up for GitHub

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

Jump to bottom

[One .NET] rework .NET 6 AOT support #6562

Merged
merged 1 commit into from
Jan 5, 2022
Merged

[One .NET] rework .NET 6 AOT support #6562

merged 1 commit into from
Jan 5, 2022

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Dec 9, 2021 •
edited
Loading

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, <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.

@jonathanpeppers jonathanpeppers force-pushed the dotnet-aot-fixes branch 6 times, most recently from 255faba to 450e4f9 Compare December 10, 2021 22:51
jonathanpeppers added a commit to jonathanpeppers/runtime that referenced this pull request Dec 13, 2021
@jonathanpeppers
[MonoAOTCompiler] more properties & custom WorkingDirectory
888e42f 
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.
@jonathanpeppers jonathanpeppers mentioned this pull request Dec 13, 2021
Merged
jonathanpeppers added a commit to dotnet/runtime that referenced this pull request Dec 15, 2021
@jonathanpeppers
[MonoAOTCompiler] more properties & custom WorkingDirectory (#62725)
b2f0441 
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 `,`:

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 #56163.
github-actions bot pushed a commit to dotnet/runtime that referenced this pull request Dec 15, 2021
@jonathanpeppers
[MonoAOTCompiler] more properties & custom WorkingDirectory
9f93966 
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 `,`:

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 #56163.
akoeplinger pushed a commit to dotnet/runtime that referenced this pull request Dec 16, 2021
@github-actions @jonathanpeppers
[MonoAOTCompiler] more properties & custom WorkingDirectory (#62849)
2ab2de4 
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 `,`:

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 #56163.

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
@jonathanpeppers
[One .NET] rework .NET 6 AOT support
cc090dc 
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.
@jonathanpeppers jonathanpeppers changed the title [WIP] rework .NET 6 AOT support [One .NET] rework .NET 6 AOT support Dec 20, 2021
@jonathanpeppers jonathanpeppers marked this pull request as ready for review December 20, 2021 18:55
jonpryor
jonpryor reviewed Dec 27, 2021
View reviewed changes
src/Xamarin.Android.Build.Tasks/Tasks/GetAotArguments.cs
[Required, Output]
public ITaskItem [] ResolvedAssemblies { get; set; } = Array.Empty<ITaskItem> ();

[Output]
public string? Triple { 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.

Naming suggestion: TargetTriplet (as per autoconf docs) instead of Triple. Triple is not necessarily "fully semantically meaningful."

Copy link
Member Author

Choose a reason for hiding this comment

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

Neither TargetTriplet or Triple really mean anything to me -- so open to suggestions.

The <MonoAOTCompiler/> task uses Triple, which passes mtriple to the actual AOT compiler:

https://github.com/dotnet/runtime/blob/bdc6ad73834a13e80f131e402e883903c31ea172/src/tasks/AotCompilerTask/MonoAOTCompiler.cs#L573-L576

We could use a different name, but is that worse? We'd still be using Triple on the next task unless we also renamed it -- and also got that change backported.

@jonpryor jonpryor merged commit ae6da92 into dotnet:main Jan 5, 2022
@jonathanpeppers jonathanpeppers deleted the dotnet-aot-fixes branch January 5, 2022 19:53
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonpryor jonpryor jonpryor left review comments

@dellis1972 dellis1972 dellis1972 approved these changes

@pjcollins pjcollins pjcollins approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

.NET SDK for Android + $(EnableLLVM)=True + $(AotAssemblies)=True + Windows: bad path quoting?
4 participants
@jonathanpeppers @jonpryor @dellis1972 @pjcollins