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

[NativeAOT LLVM] Using together with .NET 8 SDK #2235

Open
kant2002 opened this issue Mar 23, 2023 · 121 comments
Open

[NativeAOT LLVM] Using together with .NET 8 SDK #2235

kant2002 opened this issue Mar 23, 2023 · 121 comments
Labels
area-NativeAOT-LLVM LLVM generation for Native AOT compilation (including Web Assembly)

Comments

@kant2002
Copy link
Contributor

Troubleshooting of recent issue uncover fact that .NET 8 SDK is not supported.
So my current proposal is advocate following way of consuming NativeAOT-LLVM

<ItemGroup Condition="$(RuntimeIdentifier) == 'browser-wasm'">
  <!-- Remove existing ILCompilers -->
  <KnownILCompilerPack Remove="Microsoft.DotNet.ILCompiler" />
  <!-- Add NativeAOT LLVM. -->
  <KnownILCompilerPack Include="Microsoft.DotNet.ILCompiler.LLVM"
    TargetFramework="net7.0"
    ILCompilerPackNamePattern="runtime.**RID**.Microsoft.DotNet.ILCompiler.LLVM"
    ILCompilerPackVersion="8.0.0"
    ILCompilerRuntimeIdentifiers="browser-wasm;win-x64" />

     <!-- This line needed only for wildcard consumption -->
    <PackageReference Include="Microsoft.DotNet.ILCompiler.LLVM" Version="8.0.0-*" />
</ItemGroup>

Now a bit about how that's working.
Detection of supported ILC package happens here by matching TFM with KnownILCompilerPack
https://github.com/dotnet/sdk/blob/d8b549b85299046654ba8e9bd0caff0eed507ae6/src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs#L613-L618
and then added as implicit package references here
https://github.com/dotnet/sdk/blob/d8b549b85299046654ba8e9bd0caff0eed507ae6/src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs#L690-L697

I initially thought that first place can be extended to perform ILC package selection based on the TFM + RID, but second place insert additioal reference on ILCompiler which causing issues.

So this led me to following solution where I remove existing KnownILCompilerPack and create new one in suited for NativeAOT-LLVM.

If dotnet/sdk#31398 lands then removal of existing ILCompiler does not needed.

I did not dig deeper why we cannot use NativeAOT-LLVM without PublishAot, maybe that would be more viable solution with less divergence from main.

@SingleAccretion
Copy link

Thank you for looking into this! I'll share my thinking on the problem space.

At a high level, this can be approached from 2 angles:

  1. Add the ability to publish using the LLVM package into SDK - write logic such that careful shimming is possible.
  2. Decouple LLVM-based publishing from the SDK. This would still require changes to the SDK logic, unfortunately, if we want to support PublishAot == true (which we do), but they would be in a form of "add a switch to disable warnings/errors". I believe we can make it work for PublishAot == false without SDK changes, by reverting to the old package-based scheme of acquiring runtime packs.

I have a feeling 1 may not work well long term as upstream is likely to bake more assumptions into the SDK. 2 is not without its risks either, however. I do not have a great handle on which one of these is ultimately better.

One way to look at this is that it is categorically not possible to avoid some kind of project file manipulation beyond the package references (either shimming of KnownILCompilerPack, or setting some hypothetical PublishAotButIWouldLikeTheSdkToThinkNot property) because of restore, since that runs ProcessFrameworkReferences.cs with the hardcoded pack set. From this it follows that 1 and 2 both have equally suboptimal user experience, so it's just about the maintenance.

There is also dotnet/sdk#28495 which would help us a lot if finished.

@jkotas
Copy link
Member

jkotas commented Mar 23, 2023

we can make it work for PublishAot == false without SDK changes, by reverting to the old package-based scheme of acquiring runtime packs.

I think PublishAot==false + old package based scheme would work best for as-long-as the project lives in runtimelab.

@yowl
Copy link
Contributor

yowl commented Apr 9, 2023

@kant2002 you might want to try this again with the latest packages.

@kant2002
Copy link
Contributor Author

kant2002 commented Apr 9, 2023

Don't worry, I keep close eye on this :)

@kant2002
Copy link
Contributor Author

kant2002 commented Apr 9, 2023

.NET 7.0.200 working
.NET 8 preview 2 don't with error error MSB4057: The target "PrepareForILLink" does not exist in the project

C:\nativeaotwasm\hellojs\.packages\microsoft.dotnet.ilcompiler.llvm\8.0.0-alpha.1.23208.2\build\Microsoft.NETCore.Native.targets(226,7): error MSB4057: The target "PrepareForILLink" does not exist in the project. [C:\nativeaotwasm\hellojs\hellojs.csproj]

@yowl
Copy link
Contributor

yowl commented Apr 9, 2023

Thanks, yes agree, does removing the line BeforeTargets="_RunILLink" line 228, maybe, in Microsoft.NetCore.Native.targets fix it?

@kant2002
Copy link
Contributor Author

kant2002 commented Apr 9, 2023

That does not working. For .NET 8 (because it rely on ProcessFrameworkReferences task and KnownILLinkPack I have to add <PackageReference Include="Microsoft.NET.ILLink.Tasks" Version="8.0.0-*" />.

Now I have to use

<ItemGroup>
  <PackageReference Include="Microsoft.DotNet.ILCompiler.LLVM" Version="8.0.0-*" />
  <PackageReference Include="runtime.win-x64.Microsoft.DotNet.ILCompiler.LLVM" Version="8.0.0-*" />
  <PackageReference Include="Microsoft.NET.ILLink.Tasks" Version="8.0.0-*" />
</ItemGroup>

But that combination does not work with .NET 7 SDK

@kant2002
Copy link
Contributor Author

Okay. I seems to have working workaround.

<ItemGroup>
  <PackageReference Include="Microsoft.DotNet.ILCompiler.LLVM" Version="8.0.0-*" />
  <PackageReference Include="runtime.win-x64.Microsoft.DotNet.ILCompiler.LLVM" Version="8.0.0-*" />
  <PackageReference Include="Microsoft.NET.ILLink.Tasks" Version="8.0.0-*" Condition="'$(ILLinkTargetsPath)' == ''" />
</ItemGroup>

@RReverser
Copy link

This simplifies so much compared to the previous template 😍

@yowl
Copy link
Contributor

yowl commented Apr 19, 2023

we can make it work for PublishAot == false without SDK changes, by reverting to the old package-based scheme of acquiring runtime packs.

I think PublishAot==false + old package based scheme would work best for as-long-as the project lives in runtimelab.

I should be able to use %(PackageDefinitions.Name) ?

@jkotas jkotas added the area-NativeAOT-LLVM LLVM generation for Native AOT compilation (including Web Assembly) label Apr 20, 2023
@hez2010
Copy link

hez2010 commented Apr 22, 2023

You can simply pass /p:PublishTrimmed=true as well to fix this.

@RReverser
Copy link

Okay. I seems to have working workaround.

So this worked for me when you published it but seems it got broken over the last week or so:

C:\Users\me\Documents\NativeAOTWasm\helloworld\.packages\microsoft.dotnet.ilcompiler.llvm\8.0.0-alpha.1.23215.1\build\
Microsoft.NETCore.Native.Publish.targets(73,5): error : Add a PackageReference for 'runtime.browser-x64.Microsoft.DotN
et.ILCompiler.LLVM' to allow cross-compilation for wasm [C:\Users\me\Documents\NativeAOTWasm\helloworld\helloworld.csp
roj]

I guess I need to find a pinned version to revert to, as adding said package reference does not help (it doesn't resolve).

@RReverser
Copy link

Looks like 8.0.0-alpha.1.23208.2 is the last one that works w/o modifications, going to pin to that.

@hez2010
Copy link

hez2010 commented Apr 24, 2023

@RReverser You can set IlcHostPackagePath in property manually to workaround this issue, e.g. <IlcHostPackagePath>C:\Users\<username>\.nuget\packages\runtime.win-x64.microsoft.dotnet.ilcompiler.llvm\<version></IlcHostPackagePath>

@RReverser
Copy link

Yeah I don't want to hardcode paths in a shared repo like that, I'm okay with pinned version for now.

@SteveSandersonMS
Copy link
Member

To avoid hardcoding a path I worked around it like this:

	<Target Name="FixIlcHostPackageName" BeforeTargets="ImportRuntimeIlcPackageTarget">
		<PropertyGroup>
			<IlcHostPackageName>runtime.win-x64.Microsoft.DotNet.ILCompiler.LLVM</IlcHostPackageName>
		</PropertyGroup>
	</Target>

@yowl
Copy link
Contributor

yowl commented May 6, 2023

With the latest packages this should be fixed. Add

<PropertyGroup>
  <PublishTrimmed>true</PublishTrimmed>
</PropertyGroup>

to your csproj

@NCLnclNCL
Copy link

true

with net8 preview 5, you dont need it

@RReverser
Copy link

RReverser commented Aug 16, 2023

Seeing some new build issue on 8.0.0-*, but only with -c Release, debug mode works fine:

  Generating native code
  /usr/bin/sh: 2: /tmp/MSBuildTemprreverser/tmp3710694853c443faab955f051a8dd6ad.exec.cmd: \tools\ilc: not found
/home/rreverser/wasiconsole/.packages/microsoft.dotnet.ilcompiler.llvm/8.0.0-preview.7.23414.1/build/Microsoft.NETCore.Native.targets(324,5): error MSB3073: The command ""\tools\\ilc" @"obj/Release/net8.0/browser-wasm/native/wasiconsole.ilc.rsp"" exited with code 127. [/home/rreverser/wasiconsole/wasiconsole.csproj]

Although it's been couple of months since I touched NativeAOT, maybe this is not new and just me not having something configured locally?

@SingleAccretion
Copy link

Looks a lot like the problem that was fixed in #2334.

@RReverser
Copy link

Hm that PR seems reasonably old so the fix should be shipped in the latest version that I'm using?

@RReverser
Copy link

The error itself in the PR description looks different too.

@SingleAccretion
Copy link

Agree it actually looks different. It is also odd that it'd depend on the configuration. Could you share a binlog?

@RReverser
Copy link

Actually nevermind, I think I changed something unrelated - it does fail on Debug too.

@SingleAccretion
Copy link

One thing that I should have noted sooner - we never had a Unix build of the host compiler working, and this looks suspiciously like a Unix system.

@RReverser
Copy link

Oh. Yes, it is.

we never had a Unix build of the host compiler working

Weird, I thought I did build NativeAOT Wasm experiments successfully on Unix in the past?.. Unless I always did it outside WSL, but that would be weird as I still have some projects in WSL configured for NativeAOT...

@SingleAccretion
Copy link

Weird, I thought I did build NativeAOT Wasm experiments successfully on Unix in the past?.. Unless I always did it outside WSL, but that would be weird as I still have some projects in WSL configured for NativeAOT...

What I believe was "working" in the past is host build of the compiler targeting not-WASM (i. e. basically the same compiler that's in the mainline PublishAot experience). There are a few issues related to the work required (e. g. #1890, #2119, ...).

@RReverser
Copy link

First thing to try is to clean, then build clr.aot+clr.wasmjit+libs ...

Hm does clr.wasmjit need to be built for -arch wasm -os wasi as well? Looking at the docs it seemed it only needs to be built for the actual host, while clr.aot+libs for both host and Wasm.

@SingleAccretion
Copy link

The build output makes it seem that the native runtime pieces are not built at all, which would explain the missing AsmOffsets.cs.

If I were debugging this, I would diff build-runtime.cmd in this branch against upstream to see what was needed to enable the wasm build.

@RReverser
Copy link

The build output makes it seem that the native runtime pieces are not built at all

FWIW if you're referring to ... in the beginning, I just trimmed that output before posting for convenience. But I'll recheck after trying @yowl's suggestion.

@yowl
Copy link
Contributor

yowl commented Sep 5, 2023

First thing to try is to clean, then build clr.aot+clr.wasmjit+libs ...

Hm does clr.wasmjit need to be built for -arch wasm -os wasi as well? Looking at the docs it seemed it only needs to be built for the actual host, while clr.aot+libs for both host and Wasm.

Sorry, missed it was the wasm build that was failing. No you don't want the clr.wasmjit there, but if you had a previous failure for some missing include, then I wouldn't be surprised if a clean "fixed" it.

@RReverser
Copy link

but if you had a previous failure for some missing include, then I wouldn't be surprised if a clean "fixed" it

The missing include is in UnixContext.h: it refers to ucontext_t but doesn't #include <ucontext.h> that declares that type. I think that one was a legitimate issue and adding the include fixed it. I probably should make a PR for this one.

Also now that I looked upstream, I see it has the same line I had to add: https://github.com/dotnet/runtime/blob/ce3b14da2fcc4c97ebdfbbd26d47a9e441db5297/src/coreclr/nativeaot/Runtime/unix/UnixContext.h#L7

No you don't want the clr.wasmjit there

Ah cool. So to double-check, now I've cleaned the artifacts and just doing ./build.sh -arch wasm -os wasi -s clr.aot+libs -c Debug, this time skipping the non-Wasm build I did in the previous attempt.

[...later...]

Ok yeah now it fails with a lot of C errors instead. I guess it picked up some artifacts from non-Wasm build before? The docs were a bit unclear on the order of Wasm and non-Wasm builds. Anyway, at least this is not build config issue anymore - thanks - so will look into those C errors now.

@SingleAccretion
Copy link

The docs were a bit unclear on the order of Wasm and non-Wasm builds

Nominally, it should not matter and the two builds should be completely independent (we are building artifacts for two very different platforms, after all). Theory != practice, of course.

@yowl
Copy link
Contributor

yowl commented Sep 5, 2023

I'm not sure that it is picking up artifacts from the other build, as @SingleAccretion says, should be independent and in my experience they are. I just suspect (and never investigated to be fair), that something is not quite right with the dependencies and it skips the step that includes the generation of this file because some other artifact that did work is present.

@RReverser
Copy link

RReverser commented Sep 5, 2023

I'm not sure that it is picking up artifacts from the other build,

Out of curiosity I just tried again and I'm getting same error if I try to build ./build.sh clr.wasmjit+clr.aot+libs -c Debug and then try ./build.sh -arch wasm -os wasi -s clr.aot+clr.wasmjit+libs -c Debug again.

If I'm only doing ./build.sh -arch wasm -os wasi -s clr.aot+clr.wasmjit+libs -c Debug on a clean build, I'm never running into the AsmOffsetsPortable issue, so I'm still leaning into the theory that builds somehow conflict with each other. I might be wrong, but it would at least explain the symptoms.

@RReverser
Copy link

If I'm only doing ./build.sh -arch wasm -os wasi -s clr.aot+clr.wasmjit+libs -c Debug on a clean build, I'm never running into

or maybe not "never"... 😅

@yowl
Copy link
Contributor

yowl commented Sep 5, 2023

Is there a reason why you have clr.wasmjit in the -arch wasm -os wasi build, you don't need or want that?

@RReverser
Copy link

Sorry, this time it was my turn, I copied the wrong thing into the issue. Indeed, I'm not building clr.wasmjit in Wasm build.

@RReverser
Copy link

...and now I'm consistently getting the AsmOffsetsPortable.cs error again even with clean build followed by ./build.sh -s clr.aot+libs -c Debug -a wasm -os wasi. Even though that's what seemed to have fixed the issue for me above.

I love build systems.

@RReverser
Copy link

If I were debugging this, I would diff build-runtime.cmd in this branch against upstream to see what was needed to enable the wasm build.

I'll try this next.

@RReverser
Copy link

RReverser commented Sep 6, 2023

Ok at least ./build.sh -s clr.aot -c Debug -a wasm -os wasi --ninja is now compiling too. Next up, libs. clr.aot+libs too.

@RReverser
Copy link

RReverser commented Sep 6, 2023

Looking at wasmjit config:

create_standalone_jit(TARGET clrjit_browser_wasm32_${ARCH_HOST_NAME} OS browser ARCH wasm32 DESTINATIONS .)
install_clr(TARGETS clrjit_browser_wasm32_${ARCH_HOST_NAME} DESTINATIONS . COMPONENT wasmjit)

Do I understand right from those lines that it's only built for the browser and not WASI? That probably doesn't make sense unless wasmjit is optional somehow?

@SingleAccretion
Copy link

So, the OS moniker here refers to the target OS. Some Jit's are built with hardcoded target OSes, others use dynamic checks. The wasmjit is of the latter variety and should really be called clrjit_universal_wasm32.dll, it's just a historical artifact that it is not now.

@RReverser
Copy link

Ah ok thanks. I'll try to get clr.wasmjit to build now. It kinda did earlier, but it was only because CLR_CMAKE_BUILD_LLVM_JIT was not actually set by the script so it wasn't really built yet unfortunately.

@RReverser
Copy link

Right now I'm mostly struggling with all the missing headers in PAL. I admit I don't really understand this system or why it (in pal/inc/rt/cpp) duplicates STL headers but with dummy contents in some places and custom replacements in others.

There also seems to be some USE_STL build variable that sounds like it should switch to C++ STL but it still tries to and fails to find many headers in that PAL folder instead :/

@RReverser
Copy link

And even if I add those dummy headers, then I'm getting errors like use of undeclared identifier 'size_t', unknown type name 'uint32_t', etc, which suggests they shouldn't be dummy? Why doesn't build system use real C / C++ headers instead like it does on Windows?

@SingleAccretion
Copy link

I admit I don't really understand this system

Neither do I, truth be told. I know there have been (a number of) conversations in the past about all these problems and what the ways to solve them are, but I do not recall where.

@jkotas would you have advice on the direction here?

@yowl
Copy link
Contributor

yowl commented Sep 11, 2023

#1797 is the old issue. feature/NativeAOT-LLVM...yowl:llvm-linux#diff-af66f6b851332d1e155eef9bf358246005bd77648b156261a74ef36ee90ea1e9R237 are a couple of the changes I made around STL. All a bit old now.

@RReverser
Copy link

@yowl Oh wow, the size of that changeset. I guess I was optimistic 😅

I have the shell script changes already which look similar and enabled me to build runtime packages; I don't have the if (CLR_CMAKE_BUILD_LLVM_JIT) parts though - I'll try and see how far that gets me without all the rest.

@jkotas
Copy link
Member

jkotas commented Sep 11, 2023

@jkotas would you have advice on the direction here?

CoreCLR PAL for non-Windows platforms emulates Windows SDK behaviors that are incompatible with standard headers on Linux. It means that a single .cpp file cannot include both PAL headers and Linux standard headers at the same time.

@AaronRobinsonMSFT and others have been working towards cleaning this up and removing this limitation.

@RReverser
Copy link

RReverser commented Sep 11, 2023

PAL used to build CoreCLR for non-Windows platforms emulates Windows SDK behaviors that are incompatible with standard headers on Linux.

I think I saw that mentioned about emulating Windows SDK and that makes sense for platform-specific APIs, but why would C++ STL itself be so different that it needs to replace/emulate those headers as well? Is this down to differences between MSVC and GCC/Clang headers?

@jkotas
Copy link
Member

jkotas commented Sep 11, 2023

Yes. Also, CoreCLR PAL does not emulate everything. It only emulates parts needed by CoreCLR. C++ STL depends on much more.

@RReverser
Copy link

Yes. Also, CoreCLR PAL does not emulate everything. It only emulates parts needed by CoreCLR. C++ STL depends on much more.

Right, and that mismatch seems to be what leads to complications. When you say it's being worked on you mean the code will be compatible with any STL or that more things will be emulated over time?

@jkotas
Copy link
Member

jkotas commented Sep 11, 2023

The goal is to remove all PAL constructors that conflict with standard headers, so that it is possible to use both the PAL and standard headers at the same time.

@RReverser
Copy link

I got only couple more compilation errors fixed but I think I'm going to give up for now :( The interaction between PAL and STL in the build system is beyond my comprehension at the moment, too many weird errors.

@jkotas
Copy link
Member

jkotas commented Sep 11, 2023

The interaction between PAL and STL in the build system is beyond my comprehension at the moment, too many weird errors.

Yes, this is something that needs to be cleaned up upstream first. It does not make sense to do this in this repo/branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-LLVM LLVM generation for Native AOT compilation (including Web Assembly)
Projects
None yet
Development

No branches or pull requests

8 participants