[NativeAOT-LLVM] add support for building WASI and browser debug & release on Linux #2605

yowl · 2024-06-06T18:51:53Z

This PR adds the scripts and YML for building WASI and browser on Linux.

Takes some of the changes from Joel's work at #2614 to build on Linux. I changed some cases of Browser to browser which seemed like the more consistent direction.

Decided to pass TestWrapperTargetsWindows down from runtimelab-post-build-steps.yml to avoid troublesome inferencing.

jkotas · 2024-06-25T02:45:26Z

src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets

@@ -22,7 +22,7 @@ The .NET Foundation licenses this file to you under the MIT license.
    <_SymbolPrefix Condition="'$(_IsApplePlatform)' == 'true'">_</_SymbolPrefix>
    <LinkerFlavor Condition="'$(LinkerFlavor)' == '' and '$(_targetOS)' == 'freebsd'">lld</LinkerFlavor>
    <LinkerFlavor Condition="'$(LinkerFlavor)' == '' and '$(_linuxLibcFlavor)' == 'bionic'">lld</LinkerFlavor>
-    <LinkerFlavor Condition="'$(LinkerFlavor)' == '' and '$(_targetOS)' == 'linux'">bfd</LinkerFlavor>
+    <LinkerFlavor Condition="'$(LinkerFlavor)' == '' and '$(_targetOS)' == 'linux'">lld</LinkerFlavor>


This should propagate from the top-level LDFLAGS set by the init-compiler.sh script. Is it getting lost somewhere?

I assume from the fact that changing this line to lld makes no difference, that it is arriving as bfd.

Something up with args to init-compiler.sh maybe.

eng/pipelines/runtimelab/install-wasi-sdk.ps1

eng/pipelines/runtimelab/install-wasmer.ps1

yowl · 2024-07-30T01:37:12Z

cc @dotnet/nativeaot-llvm

SingleAccretion · 2024-08-07T15:54:31Z

eng/pipelines/common/platform-matrix.yml

+# Browser WebAssembly Linux X64
+
+- ${{ if containsValue(parameters.platforms, 'Browser_wasm_linux_x64') }}:


My preference would be to name these in a way that makes it clear that they're downstream-specific. E. g. Browser_wasm_linux_x64_naot_llvm; and comment we're adding these because the "stock" wasi-wasm doesn't have the build tools (that sanitizer image) we need.

SingleAccretion · 2024-08-07T15:55:29Z

eng/pipelines/common/platform-matrix.yml

+      ${{ if eq(parameters.container, '') }}:
+        container: linux_x64
+      ${{ if ne(parameters.container, '') }}:
+        container:
+          image: ${{ parameters.container }}
+          registry: mcr


I think it would make sense to hardcode the azurelinux-3.0-cross-amd64-net9.0-sanitizer here instead of runtimelab* YMLs.

SingleAccretion · 2024-08-07T15:57:19Z

eng/pipelines/runtimelab.yml

+        #
+        # Build and test wasi linux with Release libraries and Release runtime with sanitzer container
+        #


Why not fold this into Build and test with Release libraries and Release runtime above?

SingleAccretion · 2024-08-07T15:59:51Z

eng/pipelines/runtimelab/install-llvm.ps1


-    Write-Host "Invoking CMake configure: 'cmake $CmakeConfigureCommandLine'"


Logging deleted intentionally?

SingleAccretion · 2024-08-07T16:07:48Z

eng/testing/FindWasmHostExecutableAndRun.sh

+
+if [ -n "${WASM_HOST_EXECUTABLE}" ]; then
+  shift
+  $WASM_HOST_EXECUTABLE "$WASM_BINARY_TO_EXECUTE" $WASM_HOST_ARGS_SEPERATOR "$@"


.cmd also logs the command it is about to run.

added log, thanks.

SingleAccretion · 2024-08-07T16:22:42Z

src/coreclr/jit/compiler.cpp

@@ -1275,8 +1275,12 @@ void DisplayNowayAssertMap()
            fout = _wfopen(strJitMeasureNowayAssertFile, W("a"));
            if (fout == nullptr)
            {
+#if !defined(HOST_WINDOWS)
+                // TODO: how do we print a `const char16_t*` portably?


Do these changes need for this to compile in CI/locally? There is an upstream change pending that will resolve them; it would be better to omit them here.

Yes, locally in docker, so presumably in CI docker as well. Without these changes we cannot compile on the image:

/runtime/src/coreclr/jit/compiler.cpp:1278:83: error: length modifier 'w' results in undefined behavior or no effect with 's' conversion specifier [-Werror,-Wformat] 1278 | fprintf(jitstdout(), "Failed to open JitMeasureNowayAssertFile \"%ws\"\n", | ~^~ /runtime/src/coreclr/jit/compiler.cpp:1279:25: error: format specifies type 'wchar_t *' but the argument has type 'LPCWSTR' (aka 'const char16_t *') [-Werror,-Wformat] 1278 | fprintf(jitstdout(), "Failed to open JitMeasureNowayAssertFile \"%ws\"\n", | ~~~ | %s 1279 | strJitMeasureNowayAssertFile); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2 errors generated. make[3]: *** [jit/CMakeFiles/clrjit_universal_wasm32_x64.dir/build.make:189: jit/CMakeFiles/clrjit_universal_wasm32_x64.dir/compiler.cpp.o] Error 1

SingleAccretion · 2024-08-07T16:23:53Z

src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets

@@ -418,6 +418,7 @@ The .NET Foundation licenses this file to you under the MIT license.
      <CompileWasmArgs Condition="'$(IlcLlvmExceptionHandlingModel)' == 'cpp'">$(CompileWasmArgs) -mllvm -enable-emscripten-cxx-exceptions</CompileWasmArgs>

      <ScriptExt Condition="'$(OS)' == 'Windows_NT'">.bat</ScriptExt>
+      <ScriptExt Condition="'$(OS)' != 'Windows_NT'"></ScriptExt>


Is ScriptExt set by something upstream? If so, this variable should be renamed to something less generic (e. g. _IlcScriptExt, or something like that).

SingleAccretion · 2024-08-07T16:32:43Z

eng/pipelines/runtimelab-official.yml

          - Browser_wasm_win
          - wasi_wasm_win
+          - Browser_wasm_linux_x64
+          - wasi_wasm_linux_x64


I suspect this will not work as-is.

We have two things being published:

Target packages: browser-wasm / wasi-wasm.

In the current setup, these are always published and so we'll get package conflicts, since both platforms will be producing the (hopefully) identical target packages.

Host packages.

In the current setup, only Browser_wasm_win publishes host packages.

To fix this up, we'll need to adjust the conditions that prevent package conflicts (e. g. only build target packages on Windows, only build host packages on Browser platforms). But I would suggest leaving the official build for a follow-up change.

Suggested change

- Browser_wasm_win

- wasi_wasm_win

- Browser_wasm_linux_x64

- wasi_wasm_linux_x64

- Browser_wasm_win

- wasi_wasm_win

ok, reverted

SingleAccretion · 2024-08-07T16:40:09Z

eng/pipelines/runtimelab/runtimelab-post-build-steps.yml

+    - script: $(Build.SourcesDirectory)/build$(scriptExt) clr.wasmjit+clr.aot -c $(buildConfigUpper) $(_officialBuildParameter) -ci -cross
+      env:
+        ROOTFS_DIR: /crossrootfs/x64


I think if you switch the container in the platform definition to linux_x64_sanitizer, this explicit ROOTFS_DIR can be dropped.

Yes, that works, thanks

SingleAccretion · 2024-08-07T16:41:12Z

eng/pipelines/runtimelab/runtimelab-post-build-steps.yml

+    - ${{ elseif eq(parameters.platform, 'wasi_wasm_linux_x64') }}:
+      - script: |
+          source $(Build.SourcesDirectory)/wasm-tools/emsdk/emsdk_env.sh
+          LD_LIBRARY_PATH=/usr/local/lib:/opt/lib:/lib:/usr/lib:/crossrootfs/x64/lib/x86_64-linux-gnu/ $(Build.SourcesDirectory)/src/tests/build$(scriptExt) nativeaot $(buildConfigUpper) -arch:${{ parameters.archType }} -target-os:wasi tree nativeaot /p:LibrariesConfiguration=${{ parameters.librariesConfiguration }}


Why is a LD_LIBRARY_PATH like this needed?

Because libtinfo.so.5 is only in the rootfs. The host has libtinfo.so.6

Do we build the LLVM with libtinfo dependency ourselves or is that for pre-built LLVM?

If we build it ourselves, we can get rid of libtinfo dependency like this: https://github.com/dotnet/runtimelab/pull/741/files#diff-8bb555c9ef0d91e66546d2fa904a310f4d22f129e191dff6b12e4fbdfe65e321R33

update platform name

yowl · 2024-08-20T18:58:09Z

nuget error

yowl · 2024-08-20T20:43:44Z

Green, think I've got all the feedback addressed.

SingleAccretion · 2024-08-21T09:04:47Z

eng/pipelines/runtimelab-official.yml

@@ -35,6 +35,7 @@ extends:
        parameters:
          jobTemplate: /eng/pipelines/common/global-build-job.yml
          buildConfig: Release
+          container: azurelinux-3.0-cross-amd64-net9.0-sanitizer


Suggested change

container: azurelinux-3.0-cross-amd64-net9.0-sanitizer

SingleAccretion · 2024-08-21T09:08:15Z

eng/pipelines/runtimelab/install-wasi-sdk.cmd

@@ -3,7 +3,7 @@ cd /D "%1"

 echo Installing Wasi SDK

-powershell -NoProfile -NoLogo -ExecutionPolicy ByPass -File "%~dp0install-wasi-sdk.ps1"
+powershell -NoProfile -NoLogo -ExecutionPolicy ByPass -File "%~dp0install-wasi-sdk.ps1" -InstallDir .


Now unused and can be deleted?

SingleAccretion · 2024-08-21T09:08:44Z

eng/pipelines/runtimelab/install-wasmer.ps1

@@ -0,0 +1,34 @@
+[CmdletBinding(PositionalBinding=$false)]


Now unused and can be deleted.

SingleAccretion · 2024-08-21T09:10:37Z

eng/pipelines/runtimelab/runtimelab-post-build-steps.yml

@@ -30,12 +30,22 @@ steps:
    - ${{ if eq(parameters.platform, 'browser_wasm_win') }}:
      - script: |
          call $(Build.SourcesDirectory)\wasm-tools\emsdk\emsdk_env
-          $(Build.SourcesDirectory)/src/tests/build$(scriptExt) nativeaot $(buildConfigUpper) ${{ parameters.archType }} tree nativeaot /p:LibrariesConfiguration=${{ parameters.librariesConfiguration }}
+          $(Build.SourcesDirectory)/src/tests/build$(scriptExt) nativeaot $(buildConfigUpper) ${{ parameters.archType }} tree nativeaot /p:LibrariesConfiguration=${{ parameters.librariesConfiguration }} /p:TestWrapperTargetsWindows=true


It is concerning that /p:TestWrapperTargetsWindows=true is needed to make things work on Windows. Does that mean the local test build is broken?

SingleAccretion · 2024-08-21T09:13:41Z

eng/pipelines/runtimelab/runtimelab-post-build-steps.yml

+      - ${{ elseif in(parameters.platform, 'wasi_wasm_linux_x64_naot_llvm') }}:
+        - script: |
+            $(Build.SourcesDirectory)/src/tests/run$(scriptExt) --runnativeaottests ${{ parameters.archType }} $(buildConfigUpper) wasi
+          displayName: Run WebAssembly tests in single file mode


Why does need to be separate? AFAICT it is just the same as the ${{ else }}: below, just different parameter order.

SingleAccretion · 2024-08-21T09:16:56Z

eng/pipelines/runtimelab/runtimelab-post-build-steps.yml

+              source $(Build.SourcesDirectory)/wasm-tools/emsdk/emsdk_env.sh
+              $(Build.SourcesDirectory)/build$(scriptExt) libs.tests -test -a ${{ parameters.archType }} -os ${{ parameters.osGroup }} -lc ${{ parameters.librariesConfiguration }} -rc $(buildConfigUpper) /p:TestNativeAot=true /p:RunSmokeTestsOnly=true
+          displayName: Build and run WebAssembly libraries tests


Why does this need to be separate? emsdk_env.sh is already part of build.sh.

SingleAccretion · 2024-08-21T09:21:08Z

src/tests/Directory.Build.props

-    <TestWrapperTargetsWindows>false</TestWrapperTargetsWindows>
-    <TestWrapperTargetsWindows Condition=" ('$(TargetsWindows)' != '' And '$(TargetsWindows)' ) OR ('$(TargetOS)' == 'android' And '$(TargetArchitecture)' == 'arm64' ) OR ($(WindowsHost) And '$(TargetArchitecture)' == 'wasm')">true</TestWrapperTargetsWindows>
+    <TestWrapperTargetsWindows Condition="'$(TestWrapperTargetsWindows)' == ''">false</TestWrapperTargetsWindows>
+    <TestWrapperTargetsWindows Condition=" ('$(TargetsWindows)' != '' And '$(TargetsWindows)' ) OR ('$(TargetOS)' == 'android' And '$(TargetArchitecture)' == 'arm64' )">true</TestWrapperTargetsWindows>


It is not clear to me any changes to TestWrapperTargetsWindows logic are required.

Here's how the upstream condition will evaluate on Windows-targeting-WASM:

('$(TargetsWindows)' != '' And '$(TargetsWindows)' ) OR ('$(TargetOS)' == 'android' And '$(TargetArchitecture)' == 'arm64' ) OR ($(WindowsHost) And '$(TargetArchitecture)' == 'wasm') => true due to (($(WindowsHost) And '$(TargetArchitecture)' == 'wasm').
On Not-Windows-targeting-WASM:

=> "" (none of the Windows conditions are true).

Which is how it is supposed to be.

You are right, it's not required. Thanks.

SingleAccretion · 2024-08-21T09:27:11Z

src/tests/build.sh

@@ -318,6 +320,30 @@ handle_arguments_local() {
            fi
            ;;

+        target-os*|-target-os*)


I think it would be better to structure this the same as the Windows and run.sh script: Add wasi/browser arguments (possibly also wasm, though it's not strictly required). It would make the command lines more consistent and simplify CI.

thanks, done.

src/tests/Common/scripts/nativeaottest.sh

Co-authored-by: SingleAccretion <[email protected]>

…nto linux-x64-jobs

yowl · 2024-08-23T03:42:51Z

A problem with our timers perhaps

SingleAccretion

LGTM modulo feedback.

Thank you 🚀!

SingleAccretion · 2024-08-23T08:53:30Z

src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs

+// TODO-LLVM: This is not upstreamable and should be deleted when https://github.com/dotnet/runtimelab/pull/2614 is merged
+#if TARGET_WASI && !NATIVE_AOT


Could you open an issue about reverting these changes?

src/tests/build.proj

src/tests/build.sh

SingleAccretion · 2024-08-23T08:58:00Z

src/tests/Common/CLRTest.Execute.Bash.targets

@@ -341,7 +341,7 @@ fi
      <WatcherRunFile>"$CORE_ROOT/watchdog" $_WatcherTimeoutMins</WatcherRunFile>

      <!-- Note that this overwrites CLRTestBashPreCommands rather than adding to it. -->
-      <CLRTestBashPreCommands Condition="'$(CLRTestKind)' == 'BuildAndRun' and '$(TargetArchitecture)' == 'wasm'"><![CDATA[
+      <CLRTestBashPreCommands Condition="'$(CLRTestKind)' == 'BuildAndRun' and '$(TargetArchitecture)' == 'wasm' And '$(TargetOS)' == 'browser' And '$(RuntimeFlavor)' == 'mono'"><![CDATA[


Is the '$(TargetOS)' == 'browser' check necessary?

no, removed.

Co-authored-by: SingleAccretion <[email protected]>

…nto linux-x64-jobs

yowl · 2024-08-23T23:09:36Z

Thanks for the reviews!

jkotas · 2024-08-23T23:57:05Z

Great work! Thanks a lot

jkotas added the area-NativeAOT-LLVM LLVM generation for Native AOT compilation (including Web Assembly) label Jun 19, 2024

jkotas reviewed Jun 25, 2024

View reviewed changes

yowl closed this Jul 19, 2024

yowl reopened this Jul 19, 2024

yowl marked this pull request as ready for review July 24, 2024 13:09

yowl mentioned this pull request Jul 24, 2024

[NativeAOT-LLVM] support System.Net.Http.HttpClient on WASIp2 #2614

Closed

yowl closed this Jul 29, 2024