Skip to content

Commit

Permalink
[Xamarin.Android.Build.Tasks] Quote AOT+LLVM ld-flags values (#5979)
Browse files Browse the repository at this point in the history
Fixes: #5964

Context: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1336413

Context: accc846
Context: 8923c11
Context: 9b928f9

Customers report that when building a project:

 1. On Windows
 2. With Visual Studio 16.10, *not* 16.9 or earlier
 3. In Release configuration
 4. With `$(AotAssemblies)`=True *and* `$(EnableLLVM)`=True
 5. With the NDK installed into a directory containing spaces

then the app fails to build:

	[aot-compiler stdout] Executing the native linker: "C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\arm-linux-androideabi-ld.EXE" -Bsymbolic -shared -o obj\Release\110\aot\armeabi-v7a\libaot-Xamarin.AndroidX.CardView.dll.so.tmp "obj\Release\110\aot\armeabi-v7a\Xamarin.AndroidX.CardView.dll\temp-llvm.o" obj\Release\110\aot\armeabi-v7a\Xamarin.AndroidX.CardView.dll\temp.s.o -LC:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\lib\gcc\arm-linux-androideabi\4.9.x -LC:\Program Files (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm\usr\lib -LC:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\..\sysroot\usr\lib\arm-linux-androideabi "C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\lib\gcc\arm-linux-androideabi\4.9.x\libgcc.a" "C:\Program Files (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm\usr\lib\libc.so" "C:\Program Files (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm\usr\lib\libm.so"
	[aot-compiler stderr] C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\arm-linux-androideabi-ld.EXE: error: cannot open Files: No such file or directory
	[aot-compiler stderr] C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\arm-linux-androideabi-ld.EXE: error: cannot open (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\lib\gcc\arm-linux-androideabi\4.9.x: No such file or directory
	[aot-compiler stderr] C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\arm-linux-androideabi-ld.EXE: error: cannot open Files: No such file or directory
	[aot-compiler stderr] C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\arm-linux-androideabi-ld.EXE: error: cannot open (x86)\Android\android-sdk\ndk-bundle\platforms\android-21\arch-arm\usr\lib: No such file or directory
	[aot-compiler stderr] C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\arm-linux-androideabi-ld.EXE: error: cannot open Files: No such file or directory
	[aot-compiler stderr] C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\arm-linux-androideabi-ld.EXE: error: cannot open (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\..\sysroot\usr\lib\arm-linux-androideabi: No such file or directory
	[aot-compiler stderr] C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\arm-linux-androideabi-ld.EXE: error: cannot find -lunwind
	[aot-compiler stderr] C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\arm-linux-androideabi-ld.EXE: error: cannot find -lcompiler_rt-extras
	[aot-compiler stderr] C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\arm-linux-androideabi-ld.EXE: error: cannot find -lgcc_real
	[aot-compiler stderr] C:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\bin\arm-linux-androideabi-ld.EXE: error: cannot find -ldl

The *immediate* cause of the failure is path quoting: the
`arm-linux-androideabi-ld.EXE` invocation contains:

	-LC:\Program Files (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\lib\gcc\arm-linux-androideabi\4.9.x 

This value is not quoted, and parts of that path are seen in the
error messages:

	error: cannot open Files: No such file or directory
	error: cannot open (x86)\Android\android-sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\lib\gcc\arm-linux-androideabi\4.9.x: No such file or directory

"So", we say, "let's quote those paths!"

Update the `<Aot/>` task so that *all* the paths passed to Mono in the
`ld-flags` parameter are quoted, not just `libgcc.a`, `libc.so`, and
`libm.so`.  This fixes the above linker invocation.

Additionally, update the `AotTests` test fixture so that when the
tests run on Windows, Android SDK and NDK installation locations are
symlinked into a directory containing spaces and Ümläüts.
These "SDK Ümläüts" directories are then used by the unit tests.
This helps *ensure* that we're appropriately wrapping paths, by
ensuring that the SDK & NDK are in a directory with spaces.

We don't (yet) use these "SDK Ümläüts" for macOS unit tests because
the Android NDK `aarch64-linux-android21-clang` & related scripts
don't properly quote paths either:

	`dirname $0`/clang --target=aarch64-linux-android21 "$@"

which means an `SDK Ümläüts/ndk/…/aarch64-linux-android21-clang`
invocation fails with:

	usage: dirname path

TODO: address this shortcoming, by skipping
`aarch64-linux-android21-clang` and instead directly executing
`…/clang --target=aarch64-linux-android21`.

---

…except.  Except.

Except a lot of this doesn't *quite* make sense.

Lack of path quoting, *in and of itself*, is readily explainable and
plainly observed here.

The problem is that `ld-flags` construction hasn't changed much since
commit 8923c11, in 2019-Sep-4 (21 months ago!), in particular the
`libs.Add($"-L{toolchainLibDir}")` and
`libs.Add($"-L{androidLibPath}")` statements.  Commit 9b928f9
changed this slightly, wrapping `libgcc.a`/etc. with quotes, but did
*not* touch `toolchainLibDir` or `androidLibPath`, both of which are
responsible for the `-L…` values implicated in the above errors.

Thus, we are getting customer complaints related to AOT path quoting
when none of the relevant code has changed between d16-9 and d16-10.
The only AOT-related change is accc846, which added support to pass
the new-to-mono `ld-name` option for AOT.  `ld-name` *doesn't*
involve the directory name, and thus doesn't seem relevant here.

We *know* `ld-flags` is *involved*, because updating the `ld-flags`
value we provide to mono fixes the issue.  However, is this a
*fix* or a *workaround*? ¯\_(ツ)_/¯ 

Additionally, *why* did this start failing with d16-10?

  * Xamarin.Android change that isn't "obviously" related to AOT?

  * Mono changes?
    mono/mono@5e9cb6d...b4a3858

    There are no "obvious" changes in behavior related to `ld-flags`
    or `ld_flags`, nor anything else "obvious" that would alter
    behavior.

  * "Environment" change, e.g. in d16-9 the NDK was installed into a
    directory that lacked spaces, while in d16-10 the NDK is now
    installed into a directory containing spaces?

    There *may* be something here, but it's hard to say.
    The [GitHub Actions Microsoft Windows Server 2019 Datacenter][0]
    environment lists the NDK installation location as a path with
    *no* spaces:

        C:\Android\android-sdk\ndk\22.1.7171670

    Yet we have a report of the same "path quoting" failure happening
    ["on Github actions"][1], referencing a path that differs from
    the documented path:

        C:\Program Files (x86)\Android\android-sdk\ndk\21.4.7075529\toolchains\llvm\prebuilt\windows-x86_64\lib\gcc\arm-linux-androideabi\4.9.x

    Why does the documentation mention `C:\Android`, while
    `C:\Program Files (x86)` is used?

    Did our NDK path lookup logic change?

We don't currently know why things started blowing up in
Visual Studio 16.10.  Investigation is ongoing.

[0]: https://github.com/actions/virtual-environments/blob/2823a3cb6a62cc74961527bdf7623b4b9afb4107/images/win/Windows2019-Readme.md
[1]: #5964 (comment)
  • Loading branch information
dellis1972 authored Jun 2, 2021
1 parent c677a16 commit 6a5df16
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 12 deletions.
20 changes: 10 additions & 10 deletions src/Xamarin.Android.Build.Tasks/Tasks/Aot.cs
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ IEnumerable<Config> GetAotConfigs ()
outdir = Path.Combine (AotOutputDirectory, "arm64-v8a");
mtriple = "aarch64-linux-android";
arch = AndroidTargetArch.Arm64;
break;
break;

case "x86":
aotCompiler = Path.Combine (sdkBinDirectory, "cross-x86");
Expand Down Expand Up @@ -366,21 +366,21 @@ IEnumerable<Config> GetAotConfigs ()

var libs = new List<string>();
if (NdkUtil.UsingClangNDK) {
libs.Add ($"-L{toolchainLibDir}");
libs.Add ($"-L{androidLibPath}");
libs.Add ($"-L{toolchainLibDir.TrimEnd ('\\')}");
libs.Add ($"-L{androidLibPath.TrimEnd ('\\')}");

if (arch == AndroidTargetArch.Arm) {
// Needed for -lunwind to work
string compilerLibDir = Path.Combine (toolchainPath, "..", "sysroot", "usr", "lib", NdkUtil.GetArchDirName (arch));
libs.Add ($"-L{compilerLibDir}");
libs.Add ($"-L{compilerLibDir.TrimEnd ('\\')}");
}
}

libs.Add ($"\\\"{Path.Combine (toolchainLibDir, "libgcc.a")}\\\"");
libs.Add ($"\\\"{Path.Combine (androidLibPath, "libc.so")}\\\"");
libs.Add ($"\\\"{Path.Combine (androidLibPath, "libm.so")}\\\"");
libs.Add (Path.Combine (toolchainLibDir, "libgcc.a"));
libs.Add (Path.Combine (androidLibPath, "libc.so"));
libs.Add (Path.Combine (androidLibPath, "libm.so"));

ldFlags = string.Join(";", libs);
ldFlags = $"\\\"{string.Join("\\\";\\\"", libs)}\\\"";
}

string ldName = String.Empty;
Expand Down Expand Up @@ -466,7 +466,7 @@ IEnumerable<Config> GetAotConfigs ()
}
}
}

bool RunAotCompiler (string assembliesPath, string aotCompiler, string aotOptions, string assembly, string responseFile)
{
var stdout_completed = new ManualResetEvent (false);
Expand All @@ -487,7 +487,7 @@ bool RunAotCompiler (string assembliesPath, string aotCompiler, string aotOption
WindowStyle=ProcessWindowStyle.Hidden,
WorkingDirectory = WorkingDirectory,
};

// we do not want options to be provided out of band to the cross compilers
psi.EnvironmentVariables ["MONO_ENV_OPTIONS"] = String.Empty;
// the C code cannot parse all the license details, including the activation code that tell us which license level is allowed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,49 @@
using Mono.Cecil;
using NUnit.Framework;
using Xamarin.ProjectTools;
using Xamarin.Android.Build;

namespace Xamarin.Android.Build.Tests
{
[Category ("Node-2"), Category ("AOT")]
[Parallelizable (ParallelScope.Children)]
public class AotTests : BaseTest
{
public string SdkWithSpacesPath {
get {
return Path.Combine (Root, "temp", string.Format ("SDK Ümläüts"));
}
}

[OneTimeSetUp]
public void Setup ()
{
if (!IsWindows)
return;

var sdkPath = AndroidSdkPath;
var ndkPath = AndroidNdkPath;

var symSdkPath = Path.Combine (SdkWithSpacesPath, "sdk");
var symNdkPath = Path.Combine (SdkWithSpacesPath, "ndk");

SymbolicLink.Create (symSdkPath, sdkPath);
SymbolicLink.Create (symNdkPath, ndkPath);

Environment.SetEnvironmentVariable ("TEST_ANDROID_SDK_PATH", symSdkPath);
Environment.SetEnvironmentVariable ("TEST_ANDROID_NDK_PATH", symNdkPath);
}

[OneTimeTearDown]
public void TearDown ()
{
if (!IsWindows)
return;
Environment.SetEnvironmentVariable ("TEST_ANDROID_SDK_PATH", "");
Environment.SetEnvironmentVariable ("TEST_ANDROID_NDK_PATH", "");
Directory.Delete (SdkWithSpacesPath, recursive: true);
}

[Test, Category ("SmokeTests")]
public void BuildBasicApplicationReleaseProfiledAot ()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
<ItemGroup>
<Compile Remove="DebuggingTasksTests.cs" Condition="!Exists('$(XAInstallPrefix)xbuild\Xamarin\Android\Xamarin.Android.Build.Debugging.Tasks.dll')" />
<Compile Remove="Expected\**" />
<Compile Include="..\..\..\..\tools\xabuild\SymbolicLink.cs" />
<Content Include="Expected\GenerateDesignerFileExpected.cs">
<Link>..\Expected\GenerateDesignerFileExpected.cs</Link>
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ static string GetPathFromRegistry (string valueName)

public static string GetAndroidSdkPath ()
{
var sdkPath = Environment.GetEnvironmentVariable ("ANDROID_SDK_PATH");
var sdkPath = Environment.GetEnvironmentVariable ("TEST_ANDROID_SDK_PATH");
if (String.IsNullOrEmpty (sdkPath))
sdkPath = Environment.GetEnvironmentVariable ("ANDROID_SDK_PATH");
if (String.IsNullOrEmpty (sdkPath))
sdkPath = GetPathFromRegistry ("AndroidSdkDirectory");
if (String.IsNullOrEmpty (sdkPath))
Expand All @@ -34,7 +36,9 @@ public static string GetAndroidSdkPath ()

public static string GetAndroidNdkPath ()
{
var ndkPath = Environment.GetEnvironmentVariable ("ANDROID_NDK_PATH");
var ndkPath = Environment.GetEnvironmentVariable ("TEST_ANDROID_NDK_PATH");
if (String.IsNullOrEmpty (ndkPath))
ndkPath = Environment.GetEnvironmentVariable ("ANDROID_NDK_PATH");
if (String.IsNullOrEmpty (ndkPath))
ndkPath = GetPathFromRegistry ("AndroidNdkDirectory");
if (String.IsNullOrEmpty (ndkPath))
Expand Down

0 comments on commit 6a5df16

Please sign in to comment.