-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Run CoreCLR runtime tests using Mono with LLVM AOT. #38547
Conversation
82d8304
to
7f6ce13
Compare
Tagging subscribers to this area: @directhex |
This comment has been minimized.
This comment has been minimized.
88d2e8c
to
2726232
Compare
…lrProductArtifactName
…ere the host platform is identical to the target platform.
…s linked into Mono. This makes libtool use the C++ compiler driver when linking Mono--which uses whatever platform-specific flags are necessary to link against the C++ stdlib. Previously, libtool would use the C compiler driver, which didn't do this and would produce shared objects with no explicit dependency on libstdc++. This problem is normally masked because of the very lax dynamic linking semantics on ELF, but Mono on our CI setup is built in a CentOS 7 image (which does not contain a C++11 libstdc++) that has a GCC 7 compatibility package installed, along with a clang 9 installation that detects headers from the GCC 7 compatibility package. This compatibility package includes a libstdc++ linker script that links C++11 libstdc++ components statically into the target while dynamically linking against components present in pre-C++11 libstdc++. The end result of all of this is that Mono built with this configuration will depend on C++11 libstdc++ symbols that should have been statically linked into the library, and will outright fail to run on machines without a newer version of libstdc++ available. Future work may entail linking libstdc++ statically into Mono when using LLVM, preferably in a way that avoids including every libstdc++ symbol in libmonosgen-2.0.so's .dynsym section.
80d1aff
to
afee138
Compare
@@ -48,7 +48,7 @@ jobs: | |||
|
|||
variables: | |||
- name: coreClrProductArtifactName | |||
value: 'CoreCLRProduct_${{ parameters.runtimeVariant }}_$(osGroup)$(osSubgroup)_$(archType)_${{ parameters.liveRuntimeBuildConfig }}' | |||
value: 'CoreCLRProduct__$(osGroup)$(osSubgroup)_$(archType)_${{ parameters.liveRuntimeBuildConfig }}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this now shared or why are we changing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was done to allow LLVM AOT Mono-specific jobs to use the one-and-only variant of CoreCLR that we build right now.
Support for CoreCLR runtimeVariant
s was added in #35841. But runtimeVariant
isn't currently used at all when building CoreCLR. The choices I see right now are:
- remove
runtimeVariant
support entirely from eng/pipelines/coreclr/*.yml; - build an
llvmaot
variant of CoreCLR, for later consumption by LLVM AOT Mono-specific jobs, that is identical to the non-variant CoreCLR; or - explicitly depend on a shared non-variant CoreCLR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "explicitly depend on a shared non-variant CoreCLR." is the one that makes the most sense. I was mostly courious why was this change needed just to understand how the system worked.
It seems like we do have an extra _
in between CoreCLRProduct_
and osGroup
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "explicitly depend on a shared non-variant CoreCLR." is the one that makes the most sense. I was mostly courious why was this change needed just to understand how the system worked.
Ah, got it.
It seems like we do have an extra
_
in betweenCoreCLRProduct_
andosGroup
though.
Yeah. That's a consequence of how the product artifact name is concatenated: https://github.com/dotnet/runtime/pull/35841/files#diff-9d649abd6ea7b7db8a63e5dd8edfd14bR81
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to block this PR because of that since it seems like it is something we already heavily depend on and the change might be bigger that it seems.
eng/pipelines/runtime.yml
Outdated
runtimeVariant: llvmaot | ||
condition: >- | ||
or( | ||
eq(dependencies.checkout.outputs['SetPathVars_libraries.containsChange'], true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have to build mono AOT when libraries change? It seems like we should just need this when runtimetests or mono change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated from the non-LLVM AOT template instantiation directly above. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal was to run Mono lanes when there are Library changes, Mono changes and test changes. We have moved over to .NET BCL, so any change there can affect Mono, and we should test it out. (We just had a perf regression creep in due to a change in libraries a couple weeks ago !). I would recommend leaving the change as is, and see how badly this affects the queues/wait times for CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make sense to run runtime tests when libraries changes, libraries are not even used in that scope.
When libraries changes we run managed (libraries) tests using a mono runtime, that I agree 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - please wait for Santi's approval prior to merging.
coreclr/build-test.sh now has a new subcommand: 'mono_aot', which builds a
new target, named 'MonoAotCompileTests', added to
coreclr/tests/src/runtest.proj. This target compiles the runtime tests using
Mono LLVM AOT in a simple configuration where the host platform is identical to
the target platform. Parallel compilation happens via a hack: actual
compilation happens in mono/msbuild/aot-compile.proj, a single-target msbuild
file, and runtest.proj invokes this single-purpose project and target using
batching to create multiple parallelizable instances of this project. Future
work: use the MonoAOTCompiler custom task currently used to build the iOS
sample program.
Avoid using the runtimeVariant string when defining
coreClrProductArtifactName in mono/templates/xplat-pipeline-job.yml. There are
no "runtime variants" of CoreCLR configured with this parameter; instead,
depend on a shared non-runtime-variant build of CoreCLR.
Mark function DISubprograms as local definitions--this is an LLVM 9
compatibility fix.
Use --tag=CXX when linking libmonosgen-2.0.so via libtool when LLVM is linked
into Mono.
This makes libtool use the C++ compiler driver when linking Mono--which uses
whatever platform-specific flags are necessary to link against the C++ stdlib.
Previously, libtool would use the C compiler driver, which didn't do this and
would produce shared objects with no explicit dependency on libstdc++.
This problem is normally masked because of the very lax dynamic linking
semantics on ELF, but Mono on our CI setup is built in a CentOS 7 image (which
does not contain a C++11 libstdc++) that has a GCC 7 compatibility package
installed, along with a clang 9 installation that detects headers from the GCC
7 compatibility package. This compatibility package includes a libstdc++ linker
script that links C++11 libstdc++ components statically into the target while
dynamically linking against components present in pre-C++11 libstdc++. The end
result of all of this is that Mono built with this configuration will
dynamically depend on C++11 libstdc++ symbols that should have been statically
linked into the library, and will outright fail to run on machines without a
newer version of libstdc++ available.
Add tests that fail after LLVM AOT compilation to issues.targets.