Merge branch 'ondrej/fix-crash-on-arm64-from-weak-cmpxchg' into 'master'

Fix crash on arm64 from using atomic_compare_exchange_weak outside of the loop

See merge request isc-projects/bind9!3042

(cherry picked from commit e4671ef2fa4f9764ddc11e95ed5f1bfa56a9e0d7)

fa68a0d8 Added atomic_compare_exchange_strong_acq_rel macro
4cf275ba Replace non-loop usage of atomic_compare_exchange_weak with strong variant
4ff887db Add arm64 to GitLab CI

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 4c8774379c97fbe34787e4fccd9edb101dc76fcd..111d673800f54f47540a527b7e34a81c692a3b6e 100644 (file)
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -55,6 +55,11 @@ stages:
     - linux
     - amd64
 
+.linux-arm64: &linux_arm64
+  tags:
+    - linux
+    - arm64
+
 .linux-i386: &linux_i386
   tags:
     - linux
@@ -109,6 +114,10 @@ stages:
   image: "$CI_REGISTRY_IMAGE:debian-sid-amd64"
   <<: *linux_amd64
 
+.debian-sid-arm64: &debian_sid_arm64_image
+  image: "$CI_REGISTRY_IMAGE:debian-sid-arm64"
+  <<: *linux_arm64
+
 .debian-sid-i386: &debian_sid_i386_image
   image: "$CI_REGISTRY_IMAGE:debian-sid-i386"
   <<: *linux_i386
@@ -731,6 +740,39 @@ unit:tarball:sid:amd64:
   only:
     - tags
 
+# Jobs for regular GCC builds on Debian Sid (arm64)
+
+gcc:sid:arm64:
+  variables:
+    CC: gcc
+    CFLAGS: "${CFLAGS_COMMON} -O3"
+    EXTRA_CONFIGURE: "--enable-dnstap --with-libidn2"
+    RUN_MAKE_INSTALL: 1
+    MAKE: bear make
+  <<: *debian_sid_arm64_image
+  <<: *build_job
+
+system:gcc:sid:arm64:
+  <<: *debian_sid_arm64_image
+  <<: *system_test_job
+  dependencies:
+    - gcc:sid:arm64
+  needs: ["gcc:sid:arm64"]
+
+unit:gcc:sid:arm64:
+  <<: *debian_sid_arm64_image
+  <<: *unit_test_job
+  dependencies:
+    - gcc:sid:arm64
+  needs: ["gcc:sid:arm64"]
+
+cppcheck:gcc:sid:arm64:
+  <<: *debian_sid_arm64_image
+  <<: *cppcheck_job
+  dependencies:
+    - gcc:sid:arm64
+  needs: ["gcc:sid:arm64"]
+
 # Jobs for regular GCC builds on Debian Sid (i386)
 
 gcc:sid:i386:

diff --git a/lib/isc/app.c b/lib/isc/app.c
index a6424db5f94da3cc6e66478cd768408bda8da7cb..9513780948bcb02216874f2886599b2372c1cb99 100644 (file)
--- a/lib/isc/app.c
+++ b/lib/isc/app.c
@@ -228,8 +228,6 @@ isc_result_t
 isc_app_ctxrun(isc_appctx_t *ctx) {
        isc_event_t *event, *next_event;
        isc_task_t *task;
-       bool exp_false = false;
-       bool exp_true = true;
 
        REQUIRE(VALID_APPCTX(ctx));
 
@@ -237,8 +235,8 @@ isc_app_ctxrun(isc_appctx_t *ctx) {
        REQUIRE(main_thread == GetCurrentThread());
 #endif /* ifdef WIN32 */
 
-       if (atomic_compare_exchange_weak_acq_rel(&ctx->running, &exp_false,
-                                                true) == true)
+       if (atomic_compare_exchange_strong_acq_rel(
+                   &ctx->running, &(bool){ false }, true) == true)
        {
                /*
                 * Post any on-run events (in FIFO order).
@@ -344,9 +342,9 @@ isc_app_ctxrun(isc_appctx_t *ctx) {
                        }
                }
 #endif /* WIN32 */
-               exp_true = true;
-               if (atomic_compare_exchange_weak_acq_rel(&ctx->want_reload,
-                                                        &exp_true, false)) {
+               if (atomic_compare_exchange_strong_acq_rel(
+                           &ctx->want_reload, &(bool){ true }, false))
+               {
                        return (ISC_R_RELOAD);
                }
 
@@ -363,10 +361,9 @@ isc_app_ctxrun(isc_appctx_t *ctx) {
 isc_result_t
 isc_app_run(void) {
        isc_result_t result;
-       bool exp_false = false;
 
-       REQUIRE(atomic_compare_exchange_weak_acq_rel(&is_running, &exp_false,
-                                                    true) == true);
+       REQUIRE(atomic_compare_exchange_strong_acq_rel(
+                       &is_running, &(bool){ false }, true) == true);
        result = isc_app_ctxrun(&isc_g_appctx);
        atomic_store_release(&is_running, false);
 
@@ -380,8 +377,6 @@ isc_app_isrunning() {
 
 void
 isc_app_ctxshutdown(isc_appctx_t *ctx) {
-       bool exp_false = false;
-
        REQUIRE(VALID_APPCTX(ctx));
 
        REQUIRE(atomic_load_acquire(&ctx->running));
@@ -389,8 +384,8 @@ isc_app_ctxshutdown(isc_appctx_t *ctx) {
        /* If ctx->shutdown_requested == true, we are already shutting
         * down and we want to just bail out.
         */
-       if (atomic_compare_exchange_weak_acq_rel(&ctx->shutdown_requested,
-                                                &exp_false, true))
+       if (atomic_compare_exchange_strong_acq_rel(&ctx->shutdown_requested,
+                                                  &(bool){ false }, true))
        {
 #ifdef WIN32
                SetEvent(ctx->hEvents[SHUTDOWN_EVENT]);
@@ -480,10 +475,9 @@ isc_app_finish(void) {
 
 void
 isc_app_block(void) {
-       bool exp_false = false;
        REQUIRE(atomic_load_acquire(&isc_g_appctx.running));
-       REQUIRE(atomic_compare_exchange_weak_acq_rel(&isc_g_appctx.blocked,
-                                                    &exp_false, true));
+       REQUIRE(atomic_compare_exchange_strong_acq_rel(&isc_g_appctx.blocked,
+                                                      &(bool){ false }, true));
 
 #ifdef WIN32
        blockedthread = GetCurrentThread();
@@ -499,11 +493,9 @@ isc_app_block(void) {
 
 void
 isc_app_unblock(void) {
-       bool exp_true = true;
-
        REQUIRE(atomic_load_acquire(&isc_g_appctx.running));
-       REQUIRE(atomic_compare_exchange_weak_acq_rel(&isc_g_appctx.blocked,
-                                                    &exp_true, false));
+       REQUIRE(atomic_compare_exchange_strong_acq_rel(&isc_g_appctx.blocked,
+                                                      &(bool){ true }, false));
 
 #ifdef WIN32
        REQUIRE(blockedthread == GetCurrentThread());

diff --git a/lib/isc/include/isc/atomic.h b/lib/isc/include/isc/atomic.h
index 809ee3e42594e5d335502ef0d27a07848e5bf096..f18a94869913ee195a4f228af68d28538b710ef6 100644 (file)
--- a/lib/isc/include/isc/atomic.h
+++ b/lib/isc/include/isc/atomic.h
@@ -65,3 +65,6 @@
 #define atomic_compare_exchange_weak_acq_rel(o, e, d) \
        atomic_compare_exchange_weak_explicit(        \
                (o), (e), (d), memory_order_acq_rel, memory_order_acquire)
+#define atomic_compare_exchange_strong_acq_rel(o, e, d) \
+       atomic_compare_exchange_strong_explicit(        \
+               (o), (e), (d), memory_order_acq_rel, memory_order_acquire)

diff --git a/lib/isc/rwlock.c b/lib/isc/rwlock.c
index f6dc7d87cdff51a81517872a2fb8fb2b6fc6e98a..876f4471d407466daa7773364cf06d9dc39496a0 100644 (file)
--- a/lib/isc/rwlock.c
+++ b/lib/isc/rwlock.c
@@ -400,14 +400,10 @@ isc__rwlock_lock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {
                        break;
                }
 
-               while (1) {
-                       int_fast32_t zero = 0;
-                       if (atomic_compare_exchange_weak_acq_rel(
-                                   &rwl->cnt_and_flag, &zero, WRITER_ACTIVE))
-                       {
-                               break;
-                       }
-
+               while (!atomic_compare_exchange_weak_acq_rel(
+                       &rwl->cnt_and_flag, &(int_fast32_t){ 0 },
+                       WRITER_ACTIVE))
+               {
                        /* Another active reader or writer is working. */
                        LOCK(&rwl->lock);
                        if (atomic_load_acquire(&rwl->cnt_and_flag) != 0) {
@@ -494,8 +490,8 @@ isc_rwlock_trylock(isc_rwlock_t *rwl, isc_rwlocktype_t type) {
        } else {
                /* Try locking without entering the waiting queue. */
                int_fast32_t zero = 0;
-               if (!atomic_compare_exchange_weak_acq_rel(&rwl->cnt_and_flag,
-                                                         &zero, WRITER_ACTIVE))
+               if (!atomic_compare_exchange_strong_acq_rel(
+                           &rwl->cnt_and_flag, &zero, WRITER_ACTIVE))
                {
                        return (ISC_R_LOCKBUSY);
                }
@@ -519,28 +515,26 @@ isc_result_t
 isc_rwlock_tryupgrade(isc_rwlock_t *rwl) {
        REQUIRE(VALID_RWLOCK(rwl));
 
-       {
-               int_fast32_t reader_incr = READER_INCR;
+       int_fast32_t reader_incr = READER_INCR;
+
+       /* Try to acquire write access. */
+       atomic_compare_exchange_strong_acq_rel(&rwl->cnt_and_flag, &reader_incr,
+                                              WRITER_ACTIVE);
+       /*
+        * There must have been no writer, and there must have
+        * been at least one reader.
+        */
+       INSIST((reader_incr & WRITER_ACTIVE) == 0 &&
+              (reader_incr & ~WRITER_ACTIVE) != 0);
 
-               /* Try to acquire write access. */
-               atomic_compare_exchange_weak_acq_rel(
-                       &rwl->cnt_and_flag, &reader_incr, WRITER_ACTIVE);
+       if (reader_incr == READER_INCR) {
                /*
-                * There must have been no writer, and there must have
-                * been at least one reader.
+                * We are the only reader and have been upgraded.
+                * Now jump into the head of the writer waiting queue.
                 */
-               INSIST((reader_incr & WRITER_ACTIVE) == 0 &&
-                      (reader_incr & ~WRITER_ACTIVE) != 0);
-
-               if (reader_incr == READER_INCR) {
-                       /*
-                        * We are the only reader and have been upgraded.
-                        * Now jump into the head of the writer waiting queue.
-                        */
-                       atomic_fetch_sub_release(&rwl->write_completions, 1);
-               } else {
-                       return (ISC_R_LOCKBUSY);
-               }
+               atomic_fetch_sub_release(&rwl->write_completions, 1);
+       } else {
+               return (ISC_R_LOCKBUSY);
        }
 
        return (ISC_R_SUCCESS);