Joe McDonnell 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:
(1 comment)
> 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*>(¤t_)));
> ^~~~~~~~~~~~~~~~~~~~~~~~~~
>
> 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*>(¤t_),
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> 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
Thank you for trying that out. I noticed that I didn't include a piece of the
original Chromium code, and I think it would fix this. Chromium uses
AtomicWord64 == intptr_t for 64 bit platforms so that it matches AtomicWord, so
I made a mistake switching this to int64_t. I'm uploading a new rebased change.
http://gerrit.cloudera.org:8080/#/c/19882/1/src/kudu/gutil/atomicops.h
File src/kudu/gutil/atomicops.h:
http://gerrit.cloudera.org:8080/#/c/19882/1/src/kudu/gutil/atomicops.h@41
PS1, Line 41: typedef int64_t Atomic64;
I just noticed that on the original Chromium code has this:
// We need to be able to go between Atomic64 and AtomicWord implicitly. This
// means Atomic64 and AtomicWord should be the same type on 64-bit.
#if defined(__ILP32__) || BUILDFLAG(IS_NACL)
// NaCl's intptr_t is not actually 64-bits on 64-bit!
// http://code.google.com/p/nativeclient/issues/detail?id=1162
typedef int64_t Atomic64;
#else
typedef intptr_t Atomic64;
#endif
Thus, it would be using Atomic64=intptr_t for 64-bit platforms.
I will modify this code to use that same logic (without IS_NACL, which is a
chromium only thing).
--
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 <[email protected]>
Gerrit-Reviewer: Alexey Serbin <[email protected]>
Gerrit-Reviewer: Joe McDonnell <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <[email protected]>
Gerrit-Comment-Date: Sun, 09 Jul 2023 03:50:30 +0000
Gerrit-HasComments: Yes
