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

Add Native AOT smoke test #82

Merged
merged 38 commits into from
Sep 11, 2020

Conversation

MichalStrehovsky
Copy link
Member

This is expected to fail for now because the delegate test doesn't run. We need a LINQ expressions library that won't Reflection.Emit.

Just want to see what the CI will do.

@MichalStrehovsky MichalStrehovsky marked this pull request as ready for review September 3, 2020 14:38
@MichalStrehovsky MichalStrehovsky mentioned this pull request Sep 7, 2020
@MichalStrehovsky
Copy link
Member Author

Never been happier to see a test failure.

Testing delegates to value types...OK
Testing delegates to virtual methods...OK
Testing delegates to interface methods...OK
Testing static open and closed delegates...OK
Testing multicast delegates...OK
Testing dynamic invoke...OK
Testing LINQ Expressions...
Expected: 100
Actual: -950833581

@@ -100,88 +100,3 @@ jobs:
nameSuffix: All_Configurations
buildArgs: -s clr.runtime+libs -c $(_BuildConfig) -allConfigurations

#
Copy link
Member Author

Choose a reason for hiding this comment

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

@joperezr The CI infrastructure in the NativeAOT branch is going to be a bit different from how CI is done in the runtime repo (we can't do AOT compilation on the Helix machines because they miss some big prerequisites, so we'll do the testing on the build machine - this should be similar how the current plan for iOS testing in the runtime repo will be).

Do you think it makes sense to try to share the runtimelab.yml file, or just make a new one to avoid constant merge conflicts?

Copy link
Member

Choose a reason for hiding this comment

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

It’s ok to have another yml file if desired, but that will mean that we have to setup a whole new pipeline on AzDO for this branch so just keep thag in mind, since AzDO pipelines are tied to only one yml file. On the other hand, if you do decide to change runtimelab.yml to suit your needs, we could just ignore conflicts from that file and keep branch’s version always.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, you shouldn’t expect to get many conflicts as the idea is to have runtimelab.yml as stable as possible, which in fact was the whole reason we created that as opposed to using runtime.yml which gets changed much more frequently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I didn't know that extra yaml file would be work - I'll just keep this and we'll ignore conflicts in the future integrations.

@@ -7,53 +7,8 @@ parameters:

steps:
# Build coreclr native test output
- script: $(Build.SourcesDirectory)/src/coreclr/build-test$(scriptExt) skipstressdependencies skipmanaged skipgeneratelayout $(buildConfigUpper) ${{ parameters.archType }}
displayName: Build native test components
- script: $(Build.SourcesDirectory)/src/coreclr/build-test$(scriptExt) skipstressdependencies $(buildConfigUpper) ${{ parameters.archType }}
Copy link
Member

Choose a reason for hiding this comment

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

This will have conflict with #86. These scripts were just moved around dotnet/runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the heads up! Yeah, conflicts in infra/build seem inevitable - they churn a lot.

@MichalStrehovsky MichalStrehovsky added the area-NativeAOT-coreclr .NET runtime optimized for ahead of time compilation label Sep 9, 2020
@@ -55,7 +55,6 @@ The .NET Foundation licenses this file to you under the MIT license.

<ItemGroup>
<NativeLibrary Include="$(IlcPath)/framework/libSystem.Native$(NativeLibraryExtension)" />
<NativeLibrary Include="$(IlcPath)/framework/libSystem.Globalization.Native$(NativeLibraryExtension)" />
Copy link
Member

Choose a reason for hiding this comment

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

We need this. We need to fix the build to start building it again.

cc @VSadov

Copy link
Member

Choose a reason for hiding this comment

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

Just wondering - Is this going to be used or just to make the build happy?
Libraries in coreclr hardcode "QCall" at the moment. AOT uses different libraries?

Copy link
Member

Choose a reason for hiding this comment

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

Native AOT needs a regular .a in the package. It does not need any tables with mapping, etc. The native linker that produces the final native AOT binary will take care of stitching everything together.

Libraries in coreclr hardcode "QCall"

"QCall" is hardcoded just for the System.Globalization.Native in coreclr. Everything else uses actual name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Consider out of scope for this pull request and disable testing on Linux?

Or is this as easy as adding Globalization.Native to CMakeLists.txt and treating DllImport("QCall") same as the other libSystem imports (i.e. have linker deal with it)?

Copy link
Member

Choose a reason for hiding this comment

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

Consider out of scope for this pull request and disable testing on Linux?

I think so.

@MichalStrehovsky
Copy link
Member Author

  /__w/1/s/src/coreclr/src/nativeaot/Runtime/unix/PalRedhawkInline.h:52: undefined reference to `__sync_val_compare_and_swap_16'
  /__w/1/s/src/coreclr/src/nativeaot/Runtime/unix/PalRedhawkInline.h:52: undefined reference to `__sync_val_compare_and_swap_16'
  /__w/1/s/artifacts/tests/coreclr/Linux.x64.Checked/Tests/Core_Root/nativeaot/sdk/libRuntime.a(UnixNativeCodeManager.cpp.o): In function `GcInfoDecoder::GetStackSlot(int, GcStackSlotBase, REGDISPLAY*)':
  /__w/1/s/src/coreclr/src/nativeaot/Runtime/./../../vm/gcinfodecoder.cpp:1769: undefined reference to `GcInfoDecoder::GetCapturedRegister(int, REGDISPLAY*)'

Seems Linux also has other problems.

@MichalStrehovsky
Copy link
Member Author

OK, this is now ready for review!

I'll leave the Linux support pieces in place because they do work - it's just we need to fix Linux to be able to uncomment the test run.

@MichalStrehovsky
Copy link
Member Author

The tests are copied from the CoreRT repo without any changes. It's not all tests yet because there's some more issues.

E.g. the reflection test is hitting something weird with SR/ResourceManager. We had to upgrade SR source generation to the one used in Arcade and it seems like there's still some work to do.

@@ -23,5 +23,9 @@ source $scriptroot/eng/common/tools.sh
InitializeDotNetCli true # Install
__dotnetDir=${_InitializeDotNetCli}

# Temporarily make this dotnet more permanent
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to send PR with this change to dotnet/runtime too.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only needed because we don't publish ilc as a selfcontained app. Do you think it's still valuable in dotnet/runtime?

Copy link
Member

Choose a reason for hiding this comment

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

I thought that you have added this for your own convenience to make dotnet.sh work on anything. Is it the case, or is this needed to make the CI work as well?

If this is for convenience, I think it would be useful in dotnet/runtime too.
If this is a temporary workaround to make CI work, it would be nice to have better comments on it that describe what it is for and what it would take to remove it.

@@ -52,7 +52,8 @@ public override bool GenerateDirectCall(string importModule, string methodName)
methodName == "LoadLibraryExW" ||
methodName == "GetProcAddress" ||
methodName == "SetLastError" ||
methodName == "GetLastError")
methodName == "GetLastError" ||
methodName == "LocalAlloc")
Copy link
Member

Choose a reason for hiding this comment

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

Add LocalFree here as well to make it symmetric? Also, GetProcessHeap and HeapAlloc do not need to be in the list now.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

@MichalStrehovsky MichalStrehovsky merged commit a458687 into dotnet:NativeAOT Sep 11, 2020
@MichalStrehovsky MichalStrehovsky deleted the smoketest branch September 11, 2020 09:34
@MichalStrehovsky
Copy link
Member Author

Great, GitHub did the thing where it errors out during squash and merge and says you need to try again, but it actually merged, with a merge commit.

I now have a theory that this happens if you push a new commit, don't wait for the UI to refresh, and hit squash and merge.

@VSadov
Copy link
Member

VSadov commented Sep 12, 2020

For the libSystem.Globalization.Native.lib - I would prefer building it with coreclr, not with libraries. Would that work for AOT?

@jkotas
Copy link
Member

jkotas commented Sep 12, 2020

We need STATIC library for NativeAOT. CoreCLR needs OBJECT library given how the build is put together today.

Would we gain anything by building the STATIC library inside coreclr subset? The library would be otherwise unused within the coreclr subset.

@VSadov
Copy link
Member

VSadov commented Sep 12, 2020

I think the eventual state will be that:

  • huge coreclr_static.lib disappears
  • native libraries are built as a part of coreclr build
  • the libs are linked directly into singlefile flavor of coreclr and also installed as .dlls for the normal case

The above may take more than one change. To really "disappear" the coreclr_static.lib we need hosting components to move to coreclr and be built there.
As an intermediate step we may see static libs next to coreclr_static.lib, so they continue be stitched into singlefile host in the installer partition.

Ultimately, the static libs can be transient and do not need to be installed, but we can do that for external consumers - i.e AOT

By building with coreclr we are closer to this model.

@VSadov
Copy link
Member

VSadov commented Sep 12, 2020

One advantage is that there is no possibility of Debug/Release mismatch.

For example if we build native libraries in Libraries, then some common, and useful, configurations like "release clr; debug libs" will not be buildable on Windows.

It is one of the reasons we do not have superhost on Windows. It is not the major issue perhaps, but it is an issue.

@VSadov
Copy link
Member

VSadov commented Sep 12, 2020

This is the change I have in mind - dotnet/runtime#42166

@jkotas
Copy link
Member

jkotas commented Sep 12, 2020

If you change CoreCLR to be on the STATIC libs plan, I agree it starts to make more sense.

There is still a problem with Mono. How does Mono fit in? Is Mono still going to build its own STATIC lib?

@VSadov
Copy link
Member

VSadov commented Sep 13, 2020

The sources would stay in the same place, so Mono can continue building own static lib.

I am not sure if the same lib binary could work for Mono or if the arrangement of build dependencies would allow to build a static lib just once. If that is the question.

Globalization is a bit special. Since it is always embedded and as long as we do not have other uses we could use OBJECT lib. For other libs we have to build static, one way or another - as long as superhost builds in a separate partition and needs them..
If we have other consumers with whom to share the libs - like AOT, it may make sense to produce static libs even when superhost moves to coreclr.

@VSadov
Copy link
Member

VSadov commented Sep 13, 2020

I assume that AOT builds coreclr as a prerequisite - directly or because of dependency on libs.
Is that actually true?

@jkotas
Copy link
Member

jkotas commented Sep 13, 2020

Michal added a new subset for the native aot so we can conveniently build just those parts for the day-to-day work on the project, without building the whole dotnet/runtime: https://github.com/dotnet/runtimelab/blob/feature/NativeAOT/eng/Subsets.props#L95 . The subsets can be sets of arbitrary build targets. It just takes extra plumbing to get the arbitrary locations wired up.

scalablecory pushed a commit that referenced this pull request Sep 22, 2020
This fixes an existing bug where a connection can be leaked if the library is cleaned up while there are any outstanding internal connections. The fix is to have the worker shut them down if there is no external owner when the worker's thread is stopped.

The PR also disabled event validation tests on schannel, since they require the connection to succeed (which doesn't work on Azure Pipelines, because Schannel TLS 1.3 isn't supported yet).
MichalStrehovsky pushed a commit to MichalStrehovsky/runtimelab that referenced this pull request Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr .NET runtime optimized for ahead of time compilation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants