[kudu-CR] PROTOTYPE: Port of IMPALA-11694 over to Kudu gutils

[Marton Greber (Code Review)]

Marton Greber has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/19882 )

Change subject: PROTOTYPE: Port of IMPALA-11694 over to Kudu gutils
......................................................................


Patch Set 1:

Did run some tests on M1(ARM) mac.
First it was failing with the following type of errors:

In file included from 
/Users/mgreber/git/kudu/src/kudu/util/block_bloom_filter.cc:41:
/Users/mgreber/git/kudu/src/kudu/util/memory/arena.h:197:9: error: no matching 
function for call to 'Acquire_Load'
        base::subtle::Acquire_Load(reinterpret_cast<AtomicWord*>(&current_)));
        ^~~~~~~~~~~~~~~~~~~~~~~~~~

In file included from 
/Users/mgreber/git/kudu/src/kudu/util/block_bloom_filter.cc:41:
/Users/mgreber/git/kudu/src/kudu/util/memory/arena.h:207:7: error: no matching 
function for call to 'Release_Store'
      base::subtle::Release_Store(reinterpret_cast<AtomicWord*>(&current_),
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~

As it turns out it can't properly match AtomicWord with Atomic64 as it would be 
expected. On ARM macOS:
AtomicWord -> intptr_t -> __darwin_intptr_t -> long
Atomic64 -> int64_t -> long long

I needed the following patch to get the build working:
------------------------------------------------------------------------
diff --git a/src/kudu/gutil/atomicops.h b/src/kudu/gutil/atomicops.h
index ee4198fac..1c85f0c79 100644
--- a/src/kudu/gutil/atomicops.h
+++ b/src/kudu/gutil/atomicops.h
@@ -41,7 +41,11 @@ typedef int32_t Atomic32;
 typedef int64_t Atomic64;
 // Use AtomicWord for a machine-sized pointer.  It will use the Atomic32 or
 // Atomic64 routines below, depending on your architecture.
+#ifdef __aarch64__ && __apple__
+typedef Atomic64 AtomicWord;
+#elif
 typedef intptr_t AtomicWord;
+#endif
 
 namespace base {
 namespace subtle {
@@ -51,7 +55,11 @@ typedef int64_t Atomic64;

 // Use AtomicWord for a machine-sized pointer.  It will use the Atomic32 or
 // Atomic64 routines below, depending on your architecture.
+#ifdef __aarch64__ && __apple__
+typedef Atomic64 AtomicWord;
+#elif
 typedef intptr_t AtomicWord;
+#endif

 // Atomically execute:
 //      result = *ptr;

---------------------------------------------------------------------------

Running ctest after the successful build showed the same test failures which 
are occurring on upstream/master. For the record:
running ctest, with 2x --rerun-failed to rule out the flaky tests:

The following tests FAILED:
        173 - memory_gc-itest (Failed)
        191 - security-itest (Failed)
        247 - external_mini_cluster-test (Failed)
        331 - cbtree-test (Failed)
        347 - kudu-tool-test.0 (Failed)
        348 - kudu-tool-test.1 (Failed)
        451 - trace-test (Failed)

This is the same set of failures which are present previous to this patch.

The question is wether the above patch is required on intel macs as well? Then 
the ifdef should be adjusted accordingly.

My test machine has been:
macOS 12.2.1 (Monterey)
XCode 13.4.1
ARM


--
To view, visit http://gerrit.cloudera.org:8080/19882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie490b9b3aa7159a1642c07b6b7e642dfac2de55e
Gerrit-Change-Number: 19882
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <ale...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <greber...@gmail.com>
Gerrit-Comment-Date: Tue, 04 Jul 2023 15:32:03 +0000
Gerrit-HasComments: No