Merge pull request #4911 from sfc-gh-anoyes/anoyes/arm-fdb_c-clang

Fix clang build on Arm

bindings/c/CMakeLists.txt

@@ -78,6 +78,8 @@ if(NOT WIN32)
     test/unit/fdb_api.cpp
     test/unit/fdb_api.hpp)
 
+  set(UNIT_TEST_VERSION_510_SRCS test/unit/unit_tests_version_510.cpp)
+
   if(OPEN_FOR_IDE)
     add_library(fdb_c_performance_test OBJECT test/performance_test.c test/test.h)
     add_library(fdb_c_ryw_benchmark OBJECT test/ryw_benchmark.c test/test.h)
@@ -85,6 +87,7 @@ if(NOT WIN32)
     add_library(mako OBJECT ${MAKO_SRCS})
     add_library(fdb_c_setup_tests OBJECT test/unit/setup_tests.cpp)
     add_library(fdb_c_unit_tests OBJECT ${UNIT_TEST_SRCS})
+    add_library(fdb_c_unit_tests_version_510 OBJECT ${UNIT_TEST_VERSION_510_SRCS})
   else()
     add_executable(fdb_c_performance_test test/performance_test.c test/test.h)
     add_executable(fdb_c_ryw_benchmark test/ryw_benchmark.c test/test.h)
@@ -92,6 +95,7 @@ if(NOT WIN32)
     add_executable(mako ${MAKO_SRCS})
     add_executable(fdb_c_setup_tests test/unit/setup_tests.cpp)
     add_executable(fdb_c_unit_tests ${UNIT_TEST_SRCS})
+    add_executable(fdb_c_unit_tests_version_510 ${UNIT_TEST_VERSION_510_SRCS})
     strip_debug_symbols(fdb_c_performance_test)
     strip_debug_symbols(fdb_c_ryw_benchmark)
     strip_debug_symbols(fdb_c_txn_size_test)
@@ -104,8 +108,10 @@ if(NOT WIN32)
   add_dependencies(fdb_c_unit_tests doctest)
   target_include_directories(fdb_c_setup_tests PUBLIC ${DOCTEST_INCLUDE_DIR})
   target_include_directories(fdb_c_unit_tests PUBLIC ${DOCTEST_INCLUDE_DIR})
+  target_include_directories(fdb_c_unit_tests_version_510 PUBLIC ${DOCTEST_INCLUDE_DIR})
   target_link_libraries(fdb_c_setup_tests PRIVATE fdb_c Threads::Threads)
   target_link_libraries(fdb_c_unit_tests PRIVATE fdb_c Threads::Threads)
+  target_link_libraries(fdb_c_unit_tests_version_510 PRIVATE fdb_c Threads::Threads)
 
   # do not set RPATH for mako
   set_property(TARGET mako PROPERTY SKIP_BUILD_RPATH TRUE)
@@ -135,6 +141,11 @@ if(NOT WIN32)
     COMMAND $<TARGET_FILE:fdb_c_unit_tests>
             @CLUSTER_FILE@
             fdb)
+  add_fdbclient_test(
+    NAME fdb_c_unit_tests_version_510
+    COMMAND $<TARGET_FILE:fdb_c_unit_tests_version_510>
+            @CLUSTER_FILE@
+            fdb)
   add_fdbclient_test(
     NAME fdb_c_external_client_unit_tests
     COMMAND $<TARGET_FILE:fdb_c_unit_tests>
@@ -158,6 +169,10 @@ set_target_properties(c_workloads PROPERTIES
   LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/share/foundationdb")
 target_link_libraries(c_workloads PUBLIC fdb_c)
 
+if (NOT WIN32 AND NOT APPLE AND NOT OPEN_FOR_IDE)
+  target_link_options(c_workloads PRIVATE "LINKER:--version-script=${CMAKE_CURRENT_SOURCE_DIR}/external_workload.map,-z,nodelete")
+endif()
+
 # TODO: re-enable once the old vcxproj-based build system is removed.
 #generate_export_header(fdb_c EXPORT_MACRO_NAME "DLLEXPORT"
 #  EXPORT_FILE_NAME ${CMAKE_CURRENT_BINARY_DIR}/foundationdb/fdb_c_export.h)

bindings/c/external_workload.map

@@ -0,0 +1,7 @@
+{
+  global:
+    workloadFactory;
+  local:
+    *;
+};
+

bindings/c/generate_asm.py

@@ -74,10 +74,41 @@ def write_unix_asm(asmfile, functions, prefix):
     for f in functions:
         asmfile.write("\n.globl %s%s\n" % (prefix, f))
         asmfile.write("%s%s:\n" % (prefix, f))
+
+        # These assembly implementations of versioned fdb c api functions must have the following properties.
+        #
+        # 1. Don't require dynamic relocation.
+        #
+        # 2. Perform a tail-call to the function pointer that works for a
+        #    function with any number of arguments. For example, since registers x0-x7 are used
+        #    to pass arguments in the Arm calling convention we must not use x0-x7
+        #    here.
+        #
+        # You can compile this example c program to get a rough idea of how to
+        # load the extern symbol and make a tail call.
+        #
+        # $ cat test.c
+        # typedef int (*function)();
+        # extern function f;
+        # int g() { return f(); }
+        # $ cc -S -O3 -fPIC test.c && grep -A 10 '^g:' test.[sS]
+        # g:
+        # .LFB0:
+        # 	.cfi_startproc
+        # 	adrp	x0, :got:f
+        # 	ldr	x0, [x0, #:got_lo12:f]
+        # 	ldr	x0, [x0]
+        # 	br	x0
+        # 	.cfi_endproc
+        # .LFE0:
+        # 	.size	g, .-g
+        # 	.ident	"GCC: (GNU) 8.3.1 20190311 (Red Hat 8.3.1-3)"
+
         if platform == "linux-aarch64":
-            asmfile.write("\tldr x16, =fdb_api_ptr_%s\n" % (f))
-            asmfile.write("\tldr x16, [x16]\n")
-            asmfile.write("\tbr x16\n")
+            asmfile.write("\tadrp x8, :got:fdb_api_ptr_%s\n" % (f))
+            asmfile.write("\tldr x8, [x8, #:got_lo12:fdb_api_ptr_%s]\n" % (f))
+            asmfile.write("\tldr x8, [x8]\n")
+            asmfile.write("\tbr x8\n")
         else:
             asmfile.write(
                 "\tmov r11, qword ptr [%sfdb_api_ptr_%s@GOTPCREL+rip]\n" % (prefix, f))

bindings/c/test/unit/unit_tests_version_510.cpp

@@ -0,0 +1,118 @@
+/*
+ * unit_tests_header_510.cpp
+ *
+ * This source file is part of the FoundationDB open source project
+ *
+ * Copyright 2013-2021 Apple Inc. and the FoundationDB project authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// Unit tests for the FoundationDB C API, at api header version 510
+
+#include "fdb_c_options.g.h"
+#include <thread>
+
+#define FDB_API_VERSION 510
+static_assert(FDB_API_VERSION == 510, "Don't change this! This test intentionally tests an old api header version");
+
+#include <foundationdb/fdb_c.h>
+
+#define DOCTEST_CONFIG_IMPLEMENT
+#include "doctest.h"
+
+#include "flow/config.h"
+
+void fdb_check(fdb_error_t e) {
+	if (e) {
+		std::cerr << fdb_get_error(e) << std::endl;
+		std::abort();
+	}
+}
+
+std::string clusterFilePath;
+std::string prefix;
+
+FDBDatabase* db;
+
+struct Future {
+	FDBFuture* f = nullptr;
+	Future() = default;
+	explicit Future(FDBFuture* f) : f(f) {}
+	~Future() {
+		if (f)
+			fdb_future_destroy(f);
+	}
+};
+
+struct Transaction {
+	FDBTransaction* tr = nullptr;
+	Transaction() = default;
+	explicit Transaction(FDBTransaction* tr) : tr(tr) {}
+	~Transaction() {
+		if (tr)
+			fdb_transaction_destroy(tr);
+	}
+};
+
+// TODO add more tests. The motivation for this test for now is to test the
+// assembly code that handles emulating older api versions, but there's no
+// reason why this shouldn't also test api version 510 specific behavior.
+
+TEST_CASE("GRV") {
+	Transaction tr;
+	fdb_check(fdb_database_create_transaction(db, &tr.tr));
+	Future grv{ fdb_transaction_get_read_version(tr.tr) };
+	fdb_check(fdb_future_block_until_ready(grv.f));
+}
+
+int main(int argc, char** argv) {
+	if (argc < 3) {
+		std::cout << "Unit tests for the FoundationDB C API.\n"
+		          << "Usage: " << argv[0] << " /path/to/cluster_file key_prefix [doctest args]" << std::endl;
+		return 1;
+	}
+	fdb_check(fdb_select_api_version(FDB_API_VERSION));
+
+	doctest::Context context;
+	context.applyCommandLine(argc, argv);
+
+	fdb_check(fdb_setup_network());
+	std::thread network_thread{ &fdb_run_network };
+
+	{
+		FDBCluster* cluster;
+		Future clusterFuture{ fdb_create_cluster(argv[1]) };
+		fdb_check(fdb_future_block_until_ready(clusterFuture.f));
+		fdb_check(fdb_future_get_cluster(clusterFuture.f, &cluster));
+		Future databaseFuture{ fdb_cluster_create_database(cluster, (const uint8_t*)"DB", 2) };
+		fdb_check(fdb_future_block_until_ready(databaseFuture.f));
+		fdb_check(fdb_future_get_database(databaseFuture.f, &db));
+		fdb_cluster_destroy(cluster);
+	}
+
+	clusterFilePath = std::string(argv[1]);
+	prefix = argv[2];
+	int res = context.run();
+	fdb_database_destroy(db);
+
+	if (context.shouldExit()) {
+		fdb_check(fdb_stop_network());
+		network_thread.join();
+		return res;
+	}
+	fdb_check(fdb_stop_network());
+	network_thread.join();
+
+	return res;
+}

bindings/java/CMakeLists.txt

@@ -138,6 +138,11 @@ else()
   add_library(fdb_java SHARED fdbJNI.cpp)
   add_library(java_workloads SHARED JavaWorkload.cpp)
 endif()
+
+if (NOT WIN32 AND NOT APPLE AND NOT OPEN_FOR_IDE)
+  target_link_options(java_workloads PRIVATE "LINKER:--version-script=${CMAKE_SOURCE_DIR}/bindings/c/external_workload.map,-z,nodelete")
+endif()
+
 target_include_directories(fdb_java PRIVATE ${JNI_INCLUDE_DIRS})
 # libfdb_java.so is loaded by fdb-java.jar and doesn't need to depened on jvm shared libraries.
 target_link_libraries(fdb_java PRIVATE fdb_c)