Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19185 )
Change subject: IMPALA-11694: Convert gutil atomic ops to C++ atomics ...................................................................... Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/19185/7/be/src/gutil/atomicops.h File be/src/gutil/atomicops.h: http://gerrit.cloudera.org:8080/#/c/19185/7/be/src/gutil/atomicops.h@41 PS7, Line 41: typedef int64_t Atomic64; Over on the Kudu equivalent of this change (https://gerrit.cloudera.org/#/c/19882/), Marton tried to build on Mac M1. It turns out that on Mac M1 intptr_t and int64_t are different types (long vs long long). Both are 64-bit, but that difference leads to problems. Marton has more in his comment on that review. It looks like I made a mistake when I did the original porting of the Chromium code. The Chromium code has this logic: https://github.com/chromium/chromium/blob/main/base/atomicops.h#L61-L69 // 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 IS_NACL is a Chrome-specific thing, but it looks like keeping logic like this should fix the Mac M1 thing (and doesn't cause a problem for Linux): // 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__) typedef int64_t Atomic64; #else typedef intptr_t Atomic64; #endif We don't support 32-bit platforms, but keeping the __ILP32__ condition keeps it closer to the Chrome code. http://gerrit.cloudera.org:8080/#/c/19185/7/be/src/gutil/atomicops.h@50 PS7, Line 50: typedef int64_t Atomic64; Same thing here -- To view, visit http://gerrit.cloudera.org:8080/19185 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d1e3f4e71988a6c464071c1cd8bdebce622e4b8 Gerrit-Change-Number: 19185 Gerrit-PatchSet: 7 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Sun, 09 Jul 2023 03:52:26 +0000 Gerrit-HasComments: Yes
