[mono] Second try at using C11 atomics #91808

lambdageek · 2023-09-08T17:29:29Z

For each platform decide if we will use C11 standard atomics, Win32 API atomics, GCC atomic intrinsics or emulated atomics.

To use C11 atomics we generally want them to be lock-free for the primitive types we care about (that is, ATOMIC_LONG_LONG_LOCK_FREE == 2 not 1 and similarly for other macros) because otherwise we cannot be sure if the atomic might use a global lock in which case we could have thread suspend problems if both the GC and the rest of the runtime use an atomic at the same time.
On win32, use the win32 atomics until MSVC atomics support is not experimental, or we update our build to pass /experimental:c11atomics; and also until we build our C++ code with C++23 or later (otherwise MSVC will complain about including stdatomic.h)
If the header gets included while we're generating offsets using offsets-tool.py, pretend we're using emulated atomics. On some Linux configurations, the libclang that we use ends up picking up the platform atomic.h header, not the clang one, and then errors out on their underlying implementation.
Replace the use of bool in some macros in Mono - it will expand to _Bool in C11 and mess things up.

Use boolean instead

Fixes [Android][arm64] Use of undeclared identifier 'ICALL_SIG_TYPE__Bool' #91779

lambdageek · 2023-09-08T17:45:33Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-09-08T17:45:48Z

Azure Pipelines successfully started running 1 pipeline(s).

lambdageek · 2023-09-08T18:41:22Z

"Build linux-arm64 Release AllSubsets_Mono_LLVMFullAot_RuntimeTests llvmfullaot failed" is failing with

https://dev.azure.com/dnceng-public/public/_build/results?buildId=400016&view=logs&jobId=e45de9b4-b3b3-54f9-2ea5-8e56201c788d&j=e45de9b4-b3b3-54f9-2ea5-8e56201c788d&t=5472c3eb-a71b-57d3-d7b0-31d38a8b059e

Running 'python3 /__w/1/s/src/mono/mono/tools/offsets-tool/offsets-tool.py --abi=aarch64-linux-gnu --netcore --targetdir="/__w/1/s/artifacts/obj/mono/linux.arm64.Release" --monodir="/__w/1/s/src/mono" --nativedir="/__w/1/s/src/native" --outfile="/__w/1/s/artifacts/obj/mono/linux.arm64.Release/cross/offsets-aarch-linux-gnu.h" --libclang="/usr/local/lib/libclang.so.16" --prefix="/crossrootfs/arm64/usr/lib/gcc/aarch64-linux-gnu/5" --sysroot="/crossrootfs/arm64"'
  Running clang: --target=aarch64-linux-gnu --sysroot /crossrootfs/arm64 -I /crossrootfs/arm64/include -I /crossrootfs/arm64/usr/lib/gcc/aarch64-linux-gnu/5/include -I /crossrootfs/arm64/usr/lib/gcc/aarch64-linux-gnu/5/include-fixed -std=gnu11 -DMONO_GENERATING_OFFSETS -I /__w/1/s/src/mono -I /__w/1/s/src/mono/mono -I /__w/1/s/src/mono/mono/eglib -I /__w/1/s/src/native -I /__w/1/s/src/native/public -I /__w/1/s/artifacts/obj/mono/linux.arm64.Release -I /__w/1/s/artifacts/obj/mono/linux.arm64.Release/mono/eglib -DTARGET_ARM64 -DHOST_LINUX -DMONO_CROSS_COMPILE -DUSE_MONO_CTX -DHAVE_SGEN_GC -DHAVE_MOVING_COLLECTOR /__w/1/s/src/mono/mono/metadata/metadata-cross-helpers.c
  
  /__w/1/s/src/mono/mono/utils/atomic.h:79:8: error: address argument to atomic operation must be a pointer to a trivially-copyable type ('volatile atomic_int *' (aka 'volatile _Atomic(int) *') invalid)

which looks similar to https://bugs.launchpad.net/ubuntu/+source/llvm-toolchain-6.0/+bug/1780747 which says that clang 6.0.1 had some kind of a regression. Are we running some really old clang?

lewing · 2023-09-08T18:59:34Z

should we revert the atomics pr while we sort this out?

lewing · 2023-09-08T19:12:58Z

reverting in #91812 while we figure out the failure

lambdageek · 2023-09-08T19:56:28Z

I kind of suspect the LLVMFullAot lane is failing because it's using the GCC stdatomic.h header, not the clang stdatomic.h header.

In GCC atomic_compare_exchange_strong is a macro defined in terms of __atomic_compare_exchange. And clang doesn't like atomic_int * as an argument type to that intrinsic:

Gnu header:
https://github.com/gcc-mirror/gcc/blob/b96b554592c5cbb6a2c1797ffcb5706fd295f4fd/gcc/ginclude/stdatomic.h#L168-L177

Clang output: https://godbolt.org/z/8WP7hbvG1

Clang header:

https://clang.llvm.org/doxygen/stdatomic_8h_source.html

Note it defines the standard function as __c11_atomic_compare_exchange_strong

Updated godbolt: https://godbolt.org/z/Kq6qrWTjW

lambdageek · 2023-09-08T20:02:41Z

I merged main and reverted Larry's revert. And also added self.target_args += ["-isystem", clang_path + "/../lib/clang/16/include"] to offsets-tool on linux-arm64 to try and convince it to get the clang header

lambdageek · 2023-09-08T20:27:03Z

@directhex do you think it's safe to do clang_path = os.path.dirname(args.libclang) self.target_args += ["-isystem", clang_path + "/../lib/clang/16/include"] on all platforms? I alaready do it for wasm and now for linux-arm64.

directhex · 2023-09-08T20:56:11Z

@lambdageek it seems fair to try for our infra. I can't guarantee it working on anyone else's.

lambdageek · 2023-09-08T21:59:56Z

The include path didn't help on linux-arm64 llvmfullaot. Its clear what the problem is, but I have no idea how to fix the include paths without a repro environment.

lambdageek · 2023-09-08T23:07:39Z

New approach. The offsets tool doesn't actually need to look at anything in atomic.h, so just ifdef it out if we're generating offsets.

It will expand to `_Bool` in C11 and mess things up. Use `boolean` instead Fixes dotnet#91779

dotnet#91489)" (dotnet#91812)" This reverts commit 2157e37.

lambdageek · 2023-10-06T18:01:56Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-10-06T18:02:11Z

Azure Pipelines successfully started running 1 pipeline(s).

the offset tool doesn't need to see the inlined atomic operations at all Also, refactor the ifdef logic up front and define one of MONO_USE_C11_ATOMIC, MONO_USE_WIN32_ATOMIC, MONO_USE_GCC_ATOMIC or MONO_USE_EMULATED_ATOMIC depending on what we will use

lambdageek · 2023-10-06T18:32:04Z

I'm back to shaving this yak. New approach: don't try to make the offsets tool smarter, just pretend we're going to use emulated atomics. The offsets tool doesn't actually need to see the implementations of mono's atomic operations, so just avoid the whole problem.

This reverts commit edf1850.

lambdageek · 2023-10-06T18:45:45Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-10-06T18:46:01Z

Azure Pipelines successfully started running 1 pipeline(s).

lambdageek · 2023-10-06T20:43:40Z

@vargaz @jkoritzinsky @akoeplinger I assume your reviews are still 👍 ? Nothing substantially different - just some refactoring to not show offsets-tools the atomic implementations (it doesn't need to see function bodies)

lambdageek · 2023-10-10T16:04:28Z

I believe the remaining failures are unrelated

lambdageek requested review from vargaz, SamMonoRT and thaystg as code owners September 8, 2023 17:29

dotnet-issue-labeler bot added the area-VM-meta-mono label Sep 8, 2023

ghost assigned lambdageek Sep 8, 2023

This comment was marked as outdated.

Sign in to view

lambdageek force-pushed the fix-gh-91779 branch from 6aa48ba to 2c2e131 Compare September 8, 2023 17:41

akoeplinger approved these changes Sep 8, 2023

View reviewed changes

vargaz approved these changes Sep 8, 2023

View reviewed changes

jkoritzinsky approved these changes Sep 8, 2023

View reviewed changes

lambdageek mentioned this pull request Sep 8, 2023

[Mono] Consider switching to C11 #90404

Closed

lewing mentioned this pull request Sep 8, 2023

Revert "[c11] define mono atomics in terms of standard atomics" #91812

Merged

lambdageek requested a review from marek-safar as a code owner September 8, 2023 20:01

This comment was marked as outdated.

Sign in to view

build-analysis bot mentioned this pull request Sep 8, 2023

Networking certificate test failures #91705

Closed

This comment was marked as outdated.

Sign in to view

build-analysis bot mentioned this pull request Sep 11, 2023

[iOS.Simulator] Failed to determine the exit code dotnet/dnceng#639

Closed

3 tasks

lambdageek marked this pull request as draft September 11, 2023 14:16

lambdageek added 3 commits October 6, 2023 14:00

[mono] Dont' use bareword bool in icall sig macros

f0daa9c

It will expand to `_Bool` in C11 and mess things up. Use `boolean` instead Fixes dotnet#91779

Revert "Revert "[c11] define mono atomics in terms of standard atomics (

816883f

dotnet#91489)" (dotnet#91812)" This reverts commit 2157e37.

Try to convince offset-tool to use clang's headers

edf1850

lambdageek force-pushed the fix-gh-91779 branch from 8661be9 to edf1850 Compare October 6, 2023 18:00

This comment was marked as outdated.

Sign in to view

lambdageek changed the title ~~[mono] Dont' use bareword bool in icall sig macros~~ [mono] Second try at using C11 atomics Oct 6, 2023

fixup - non-arm64 android

d268043

This comment was marked as outdated.

Sign in to view

Revert "Try to convince offset-tool to use clang's headers"

b0dd0b1

This reverts commit edf1850.

lambdageek mentioned this pull request Oct 6, 2023

Implement Interlocked for small types. #92974

Merged

lambdageek marked this pull request as ready for review October 6, 2023 19:08

jkoritzinsky approved these changes Oct 6, 2023

View reviewed changes

vargaz approved these changes Oct 6, 2023

View reviewed changes

build-analysis bot mentioned this pull request Oct 6, 2023

Test infra failure on iOS.Device.Aot.Test.WorkItemExecution CI leg #92792

Closed

3 tasks

akoeplinger approved these changes Oct 6, 2023

View reviewed changes

build-analysis bot mentioned this pull request Oct 6, 2023

[6.0][Android][mono] XHarness exit code 78 PACKAGE_INSTALLATION_FAILURE #84174

Open

lambdageek merged commit c48aa81 into dotnet:main Oct 10, 2023
146 of 159 checks passed

ghost locked as resolved and limited conversation to collaborators Nov 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono] Second try at using C11 atomics #91808

[mono] Second try at using C11 atomics #91808

lambdageek commented Sep 8, 2023 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

lambdageek commented Sep 8, 2023

azure-pipelines bot commented Sep 8, 2023

lambdageek commented Sep 8, 2023 •

edited

Loading

lewing commented Sep 8, 2023

lewing commented Sep 8, 2023

lambdageek commented Sep 8, 2023 •

edited

Loading

lambdageek commented Sep 8, 2023

This comment was marked as outdated.

This comment was marked as outdated.

lambdageek commented Sep 8, 2023

directhex commented Sep 8, 2023

lambdageek commented Sep 8, 2023

lambdageek commented Sep 8, 2023

This comment was marked as outdated.

This comment was marked as outdated.

lambdageek commented Oct 6, 2023

azure-pipelines bot commented Oct 6, 2023

This comment was marked as outdated.

This comment was marked as outdated.

lambdageek commented Oct 6, 2023

This comment was marked as outdated.

This comment was marked as outdated.

lambdageek commented Oct 6, 2023

azure-pipelines bot commented Oct 6, 2023

lambdageek commented Oct 6, 2023

lambdageek commented Oct 10, 2023

[mono] Second try at using C11 atomics #91808

[mono] Second try at using C11 atomics #91808

Conversation

lambdageek commented Sep 8, 2023 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

lambdageek commented Sep 8, 2023

azure-pipelines bot commented Sep 8, 2023

lambdageek commented Sep 8, 2023 • edited Loading

lewing commented Sep 8, 2023

lewing commented Sep 8, 2023

lambdageek commented Sep 8, 2023 • edited Loading

lambdageek commented Sep 8, 2023

This comment was marked as outdated.

This comment was marked as outdated.

lambdageek commented Sep 8, 2023

directhex commented Sep 8, 2023

lambdageek commented Sep 8, 2023

lambdageek commented Sep 8, 2023

This comment was marked as outdated.

This comment was marked as outdated.

lambdageek commented Oct 6, 2023

azure-pipelines bot commented Oct 6, 2023

This comment was marked as outdated.

This comment was marked as outdated.

lambdageek commented Oct 6, 2023

This comment was marked as outdated.

This comment was marked as outdated.

lambdageek commented Oct 6, 2023

azure-pipelines bot commented Oct 6, 2023

lambdageek commented Oct 6, 2023

lambdageek commented Oct 10, 2023

lambdageek commented Sep 8, 2023 •

edited

Loading

lambdageek commented Sep 8, 2023 •

edited

Loading

lambdageek commented Sep 8, 2023 •

edited

Loading