Skip to content

Commit

Permalink
KUDU-3374 Add support for M1 and macOS Monterey
Browse files Browse the repository at this point in the history
The macOS Monterey OS upgrade broke glog 0.3.5, moreover building Kudu
with LLVM 9 on Apple silicon is not really feasible. There has been
multiple non-merged patches submitted to tackle these issues:
Upgrade glog to 0.6.0 [1]
Fix codegen build on MacOS Monterey [2]
Fix building on Apple M1 [3]
Upgrade to LLVM 11 and IWYU 0.15 [4]
This patch squashes all of the above and provides the necessary
glue changes.

LLVM is updated to version 11 and glog to version 0.6.0 to fix the
initial build issues. Building the glog tests has to be turned off as
it causes linker error in TSAN build. The optional ZLIB dependency in
LLVM-IWYU is removed as the original issue -mentioned in the comments-
has been resolved, and this caused build issues on Unix. [5] Building
Kudu on different OS and compiler combinations resulted in various
errors. Using Clang from the new LLVM 11 thridparty build resulted in
consisted successful builds. Therefore the thirparty clang is used both
in the local cmake and the distributed test script. The homebrew prefix
is changed from /usr/local to /opt/homebrew on ARM macs. In places
where package locations are provided, and ARM alternative location has
been added. Added a patch to fix null pointer dereference in rapidjson.
[6] Added another patch containing assertions to a similar issue, to
fix suppress clang warnings in rapidjson. [7] Building tests in glog
has to be turned off as it causes linker error in tsan build. [8] With
the clang upgrade it now links against libatomic in TSAN builds. In
dist-test.py libatomic is added to the list of shipped libraries as it
was missing on the target machines. A couple of new TSAN race occurences
came up with LLVM regexes, added those to the sanitizer suppression
list. pstack_watcher uses gdb, pstack or gstack to examine a process.
None of those are available on ARM macs. Added lldb support, to have a
fallback on ARM macs. Finally libcurl needed to be upgraded to 7.80 as
the old version gave symbol errors during release build on M1 mac.

[1] Change-Id: I21abd1749fdfdcde412f5a2ca2245c42da20d4f3
[2] Change-Id: Idc5721cb2445303b2e79d08b547e746929c7486d
[3] Change-Id: I029a858d2da77cea84e7e6856b8a5ac02713152d
[4] Change-Id: Id9c32abe256978158617a4fe3a3c34e9bfd00fb2
[5] https://github.com/include-what-you-use/include-what-you-use/
    issues/539
[6] Tencent/rapidjson#727
[7] Tencent/rapidjson#757
[8] google/glog#54

Change-Id: I9877f95340b969308c317a6bac50665ff78e329e
  • Loading branch information
martongreber committed Jul 22, 2022
1 parent 8902ca6 commit b8608ca
Show file tree
Hide file tree
Showing 49 changed files with 352 additions and 711 deletions.
12 changes: 12 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,18 @@ if ("$ENV{NO_REBUILD_THIRDPARTY}" STREQUAL "")
endif()
endif()

# Kudu should be compiled with clang built in thirdparty directory.
# This should be set after the thirdparty packages are built.
# Because that is the process for building llvm/clang as well.
find_program(CCACHE_FOUND ccache)
if(CCACHE_FOUND)
set(CMAKE_C_COMPILER ${THIRDPARTY_DIR}/../build-support/ccache-clang/clang)
set(CMAKE_CXX_COMPILER ${THIRDPARTY_DIR}/../build-support/ccache-clang/clang++)
else()
set(CMAKE_C_COMPILER ${THIRDPARTY_DIR}/clang-toolchain/bin/clang)
set(CMAKE_CXX_COMPILER ${THIRDPARTY_DIR}/clang-toolchain/bin/clang++)
endif()

############################################################
# Compiler flags
############################################################
Expand Down
4 changes: 4 additions & 0 deletions build-support/dist_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,10 @@ def is_lib_whitelisted(lib):
# installed versions different from the dist_test image.
if "libcrypto" in lib or "libsasl2" in lib or "libssl" in lib:
return True
# After upgrading to LLVM 11, TSAN builds link libatomic.
# Since it was not present on the test machines, it is whitelisted.
if "libatomic" in lib:
return True
return False
return True

Expand Down
7 changes: 6 additions & 1 deletion build-support/iwyu/mappings/glog.imp
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,10 @@
{ symbol: [ "CHECK_GE", private, "<glog/logging.h>", public ] },
{ symbol: [ "CHECK_GT", private, "<glog/logging.h>", public ] },
{ symbol: [ "ErrnoLogMessage", private, "<glog/logging.h>", public ] },
{ symbol: [ "COMPACT_GOOGLE_LOG_0", private, "<glog/logging.h>", public ] }
{ symbol: [ "COMPACT_GOOGLE_LOG_0", private, "<glog/logging.h>", public ] },

# Upgrading from glog 0.3.5 to 0.6.0 the includes for log_severity.h and vlog_is_on.h
# are changed from #include "" to #include <> which seem to cause issues.
{ include: ["<glog/log_severity.h>", "public", "<glog/logging.h>", "public"] },
{ include: ["<glog/vlog_is_on.h>", "public", "<glog/logging.h>", "public"] }
]
1 change: 1 addition & 0 deletions build-support/jenkins/build-and-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ elif [ "$BUILD_TYPE" = "TIDY" ]; then
BUILD_PYTHON3=0
BUILD_GRADLE=0
else
USE_CLANG=1
# Must be DEBUG or RELEASE
CMAKE_BUILD=$BUILD_TYPE
fi
Expand Down
9 changes: 9 additions & 0 deletions cmake_modules/FindGLog.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,18 @@ find_path(GLOG_INCLUDE_DIR glog/logging.h
# make sure we don't accidentally pick up a different version
NO_CMAKE_SYSTEM_PATH
NO_SYSTEM_ENVIRONMENT_PATH)

set(__CURRENT_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES})
if (APPLE)
set(CMAKE_FIND_LIBRARY_SUFFIXES ".dylib")
else()
set(CMAKE_FIND_LIBRARY_SUFFIXES ".so")
endif()
find_library(GLOG_SHARED_LIB glog
NO_CMAKE_SYSTEM_PATH
NO_SYSTEM_ENVIRONMENT_PATH)
set(CMAKE_FIND_LIBRARY_SUFFIXES ${__CURRENT_FIND_LIBRARY_SUFFIXES})
unset(__CURRENT_FIND_LIBRARY_SUFFIXES)
find_library(GLOG_STATIC_LIB libglog.a
NO_CMAKE_SYSTEM_PATH
NO_SYSTEM_ENVIRONMENT_PATH)
Expand Down
2 changes: 2 additions & 0 deletions cmake_modules/FindKerberosPrograms.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ foreach(bin ${bins})
/usr/sbin
# Homebrew install location.
/usr/local/opt/krb5/sbin
# Homebrew arm install location
/opt/homebrew/opt/krb5/sbin
# Macports install location.
/opt/local/sbin
# SLES
Expand Down
2 changes: 1 addition & 1 deletion src/kudu/codegen/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ if (APPLE)
# Clang does not know about by default.
set(PREFIXED_IR_INCLUDES
${PREFIXED_IR_INCLUDES}
-cxx-isystem "/Library/Developer/CommandLineTools/usr/include/c++/v1")
-cxx-isystem "${CMAKE_OSX_SYSROOT}")
endif()

# Get preprocessing definitions, which enable directives for glog and gtest.
Expand Down
6 changes: 3 additions & 3 deletions src/kudu/codegen/code_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -158,14 +158,14 @@ int DumpAsm(FuncPtr fptr, const TargetMachine& tm, std::ostream* out, int max_in
MCInst inst;
uint64_t size;
MCDisassembler::DecodeStatus stat =
disas->getInstruction(inst, size, mem_obj.slice(addr), addr, llvm::nulls(), llvm::nulls());
disas->getInstruction(inst, size, mem_obj.slice(addr), addr, llvm::nulls());
if (stat != MCDisassembler::Success) {
*out << "<ERROR at 0x" << std::hex << addr
<< " (absolute 0x" << (addr + base_addr) << ")"
<< ", skipping instruction>\n" << std::dec;
} else {
string annotations;
printer->printInst(&inst, os, annotations, subtarget_info);
printer->printInst(&inst, addr, annotations, subtarget_info, os);
os << " " << annotations << "\n";
// We need to check the opcode name for "RET" instead of comparing
// the opcode to llvm::ReturnInst::getOpcode() because the native
Expand All @@ -178,7 +178,7 @@ int DumpAsm(FuncPtr fptr, const TargetMachine& tm, std::ostream* out, int max_in
// LLVM RTTI, since subclassing an LLVM interface would require
// identical RTTI settings between LLVM and Kudu (see:
// http://llvm.org/docs/Packaging.html#c-features).
string opname = printer->getOpcodeName(inst.getOpcode());
string opname = printer->getOpcodeName(inst.getOpcode()).str();
std::transform(opname.begin(), opname.end(), opname.begin(), ::toupper);
if (opname.find("RET") != string::npos) return i + 1;
}
Expand Down
7 changes: 5 additions & 2 deletions src/kudu/codegen/module_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@
// for successful run-time operation of the code generator.
#include <glog/logging.h>
#include <llvm/ADT/StringMap.h>
#include <llvm/ADT/StringMapEntry.h>
#include <llvm/ADT/StringRef.h>
#include <llvm/ADT/ilist_iterator.h>
#include <llvm/ADT/iterator.h>
#include <llvm/ExecutionEngine/ExecutionEngine.h>
#include <llvm/ExecutionEngine/MCJIT.h> // IWYU pragma: keep
#include <llvm/IR/Attributes.h>
Expand Down Expand Up @@ -125,7 +128,7 @@ string ToString(const Module& m) {
// This method is needed for the implicit conversion from
// llvm::StringRef to std::string
string ToString(const Function* f) {
return f->getName();
return f->getName().str();
}

bool ModuleContains(const Module& m, const Function* fptr) {
Expand Down Expand Up @@ -379,7 +382,7 @@ TargetMachine* ModuleBuilder::GetTargetMachine() const {
unordered_set<string> ModuleBuilder::GetFunctionNames() const {
unordered_set<string> ret;
for (const JITFuture& fut : futures_) {
ret.insert(CHECK_NOTNULL(fut.llvm_f_)->getName());
ret.insert(CHECK_NOTNULL(fut.llvm_f_)->getName().str());
}
return ret;
}
Expand Down
5 changes: 3 additions & 2 deletions src/kudu/common/row_operations-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <memory>
#include <ostream>
#include <string>
#include <type_traits>
#include <vector>

#include <gflags/gflags_declare.h>
Expand Down Expand Up @@ -298,7 +299,7 @@ TEST_F(RowOperationsTest, SchemaFuzz) {
g_failing_case.server_schema = &server_schema;
g_failing_case.row = &row;
ASAN_SET_DEATH_CALLBACK(&DumpFailingCase);
google::InstallFailureFunction(&GlogFailure);
google::InstallFailureFunction(reinterpret_cast<google::logging_fail_func_t>(GlogFailure));

for (int i = 0; i < client_schema.num_columns(); i++) {
if (client_schema.column(i).is_nullable() &&
Expand Down Expand Up @@ -326,7 +327,7 @@ TEST_F(RowOperationsTest, SchemaFuzz) {

DoFuzzTest(server_schema, row, 100);
ASAN_SET_DEATH_CALLBACK(NULL);
google::InstallFailureFunction(&abort);
google::InstallFailureFunction(static_cast<google::logging_fail_func_t>(abort));
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/kudu/fs/dir_util-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,15 @@ TEST_F(KuduTest, Locking) {

// Note: we must use a death test here because file locking is only
// disallowed across processes, and death tests spawn child processes.
ASSERT_DEATH({
ASSERT_DEATH(({
DirInstanceMetadataFile second(env_, "", kType, kFileName);
CHECK_OK(second.LoadFromDisk());
CHECK_EQ(kUuid, second.uuid());
CHECK_OK(second.Lock());
}, "Could not lock");
}), "Could not lock");

ASSERT_OK(first.Unlock());
ASSERT_DEATH({
ASSERT_DEATH(({
DirInstanceMetadataFile second(env_, "", kType, kFileName);
CHECK_OK(second.LoadFromDisk());
CHECK_EQ(kUuid, second.uuid());
Expand All @@ -100,7 +100,7 @@ TEST_F(KuduTest, Locking) {
} else {
LOG(FATAL) << "Could not lock: " << s.ToString();
}
}, "Lock successfully acquired");
}), "Lock successfully acquired");
}

} // namespace fs
Expand Down
10 changes: 7 additions & 3 deletions src/kudu/gutil/atomicops-internals-macosx.h
Original file line number Diff line number Diff line change
Expand Up @@ -270,11 +270,15 @@ inline void NoBarrier_Store(volatile Atomic64* ptr, Atomic64 value) {
*ptr = value;
}

// Issue the x86 "pause" instruction, which tells the CPU that we
// are in a spinlock wait loop and should allow other hyperthreads
// to run, not speculate memory access, etc.
// Issue the x86 "pause" instruction (or "yield" on aarch64), which
// tells the CPU that we are in a spinlock wait loop and should allow
// other hyperthreads to run, not speculate memory access, etc.
inline void PauseCPU() {
#ifdef __aarch64__
__asm__ __volatile__("yield" : : : "memory");
#else
__asm__ __volatile__("pause" : : : "memory");
#endif
}

inline void Acquire_Store(volatile Atomic64 *ptr, Atomic64 value) {
Expand Down
2 changes: 1 addition & 1 deletion src/kudu/gutil/dynamic_annotations.c
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ void AnnotateBenignRace(const char *file, int line,
const char *description){}
void AnnotateBenignRaceSized(const char *file, int line,
const volatile void *mem,
long size,
size_t size,
const char *description) {}
void AnnotateMutexIsUsedAsCondVar(const char *file, int line,
const volatile void *mu){}
Expand Down
2 changes: 1 addition & 1 deletion src/kudu/gutil/dynamic_annotations.h
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ void AnnotateBenignRace(const char *file, int line,
const char *description);
void AnnotateBenignRaceSized(const char *file, int line,
const volatile void *address,
long size,
size_t size,
const char *description);
void AnnotateMutexIsUsedAsCondVar(const char *file, int line,
const volatile void *mu);
Expand Down
2 changes: 1 addition & 1 deletion src/kudu/ranger/ranger_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ TAG_FLAG(ranger_logtostdout, advanced);
TAG_FLAG(ranger_logtostdout, evolving);

DECLARE_int32(max_log_files);
DECLARE_int32(max_log_size);
DECLARE_uint32(max_log_size);
DECLARE_string(log_dir);

METRIC_DEFINE_histogram(server, ranger_subprocess_execution_time_ms,
Expand Down
2 changes: 2 additions & 0 deletions src/kudu/security/test/mini_kdc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ Status GetBinaryPath(const string& binary, string* path) {
static const vector<string> kCommonLocations = {
"/usr/local/opt/krb5/sbin", // Homebrew
"/usr/local/opt/krb5/bin", // Homebrew
"/opt/homebrew/opt/krb5/sbin", // Homebrew arm
"/opt/homebrew/opt/krb5/bin", // Homebrew arm
"/opt/local/sbin", // Macports
"/opt/local/bin", // Macports
"/usr/lib/mit/sbin", // SLES
Expand Down
5 changes: 3 additions & 2 deletions src/kudu/server/diagnostics_log.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,15 @@

#include "kudu/server/diagnostics_log.h"

#include <cstddef>

#include <cstdint>
#include <functional>
#include <memory>
#include <optional>
#include <ostream>
#include <queue>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>

Expand Down Expand Up @@ -67,7 +68,7 @@ namespace google {
// symbol name to "out". The symbol name is demangled if possible
// (supports symbols generated by GCC 3.x or newer). Otherwise,
// returns false.
bool Symbolize(void *pc, char *out, int out_size);
bool Symbolize(void *pc, char *out, size_t out_size);
}

DEFINE_int32(diagnostics_log_stack_traces_interval_ms, 60000,
Expand Down
3 changes: 2 additions & 1 deletion src/kudu/server/pprof_path_handlers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <cstdint>
#include <cstdlib>
#include <fstream>
#include <functional>
#include <string>
#include <unordered_map>
#include <utility>
Expand Down Expand Up @@ -66,7 +67,7 @@ namespace google {
// symbol name to "out". The symbol name is demangled if possible
// (supports symbols generated by GCC 3.x or newer). Otherwise,
// returns false.
bool Symbolize(void *pc, char *out, int out_size);
bool Symbolize(void *pc, char *out, size_t out_size);
}

namespace kudu {
Expand Down
4 changes: 3 additions & 1 deletion src/kudu/tablet/concurrent_btree.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,9 @@ inline void PrefetchMemory(const T *addr) {
int size = std::min<int>(sizeof(T), 4 * CACHELINE_SIZE);

for (int i = 0; i < size; i += CACHELINE_SIZE) {
prefetch(reinterpret_cast<const char *>(addr) + i, PREFETCH_HINT_T0);
if (addr) {
prefetch(reinterpret_cast<const char *>(addr) + i, PREFETCH_HINT_T0);
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/kudu/util/async_logger.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ void AsyncLogger::Stop() {
void AsyncLogger::Write(bool force_flush,
time_t timestamp,
const char* message,
int message_len) {
size_t message_len) {
{
MutexLock l(lock_);
DCHECK_EQ(state_, RUNNING);
Expand Down
2 changes: 1 addition & 1 deletion src/kudu/util/async_logger.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ class AsyncLogger : public google::base::Logger {
void Write(bool force_flush,
time_t timestamp,
const char* message,
int message_len) override;
size_t message_len) override;

// Flush any buffered messages.
void Flush() override;
Expand Down
2 changes: 1 addition & 1 deletion src/kudu/util/debug-util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ extern int GetStackTrace(void** result, int max_depth, int skip_count);
// symbol name to "out". The symbol name is demangled if possible
// (supports symbols generated by GCC 3.x or newer). Otherwise,
// returns false.
bool Symbolize(void *pc, char *out, int out_size);
bool Symbolize(void *pc, char *out, size_t out_size);

namespace glog_internal_namespace_ {
extern void DumpStackTraceToString(string *s);
Expand Down
6 changes: 3 additions & 3 deletions src/kudu/util/debug/trace_logging.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ class TraceGLog {
class TraceLogSink : public google::LogSink {
public:
explicit TraceLogSink(const char* category) : category_(category) {}
void send(google::LogSeverity severity, const char* full_filename,
void send(google::LogSeverity severity, const char* /*full_filename*/,
const char* base_filename, int line,
const struct ::tm* tm_time, const char* message,
const ::google::LogMessageTime& logmsgtime, const char* message,
size_t message_len) override {
// Rather than calling TRACE_EVENT_INSTANT here, we have to do it from
// the destructor. This is because glog holds its internal mutex while
Expand All @@ -116,7 +116,7 @@ class TraceGLog {
// we defer the tracing until the google::LogMessage has destructed and the
// glog lock is available again.
str_ = ToString(severity, base_filename, line,
tm_time, message, message_len);
logmsgtime, message, message_len);
}
virtual ~TraceLogSink() {
TRACE_EVENT_INSTANT1(category_, "vlog", TRACE_EVENT_SCOPE_THREAD,
Expand Down
2 changes: 1 addition & 1 deletion src/kudu/util/flags.cc
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ DECLARE_bool(logtostderr);
TAG_FLAG(logtostderr, stable);
TAG_FLAG(logtostderr, runtime);

DECLARE_int32(max_log_size);
DECLARE_uint32(max_log_size);
TAG_FLAG(max_log_size, stable);
TAG_FLAG(max_log_size, runtime);

Expand Down
7 changes: 4 additions & 3 deletions src/kudu/util/logging-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

#include <glog/logging.h>
#include <gmock/gmock-matchers.h>
#include <gtest/gtest-matchers.h>
#include <gtest/gtest.h>

#include "kudu/gutil/strings/substitute.h"
Expand Down Expand Up @@ -108,7 +109,7 @@ class CountingLogger : public google::base::Logger {
void Write(bool force_flush,
time_t /*timestamp*/,
const char* /*message*/,
int /*message_len*/) override {
size_t /*message_len*/) override {
message_count_++;
if (force_flush) {
Flush();
Expand Down Expand Up @@ -222,9 +223,9 @@ TEST(LoggingTest, TestRedactionIllustrateUsage) {
ASSERT_EQ("public=abc, private=def", KUDU_DISABLE_REDACTION(SomeComplexStringify("abc", "def")));

// Or we can execute an entire scope with redaction disabled.
KUDU_DISABLE_REDACTION({
KUDU_DISABLE_REDACTION(({
ASSERT_EQ("public=abc, private=def", SomeComplexStringify("abc", "def"));
});
}));
}


Expand Down
Loading

0 comments on commit b8608ca

Please sign in to comment.