From 34529c353cdb57797f166571ff2b76dc10784f64 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Fri, 4 Jun 2021 20:19:19 -0700 Subject: [PATCH 1/5] Try to fix arm build on clang --- bindings/c/CMakeLists.txt | 11 ++ bindings/c/generate_asm.py | 7 +- .../c/test/unit/unit_tests_version_510.cpp | 118 ++++++++++++++++++ 3 files changed, 133 insertions(+), 3 deletions(-) create mode 100644 bindings/c/test/unit/unit_tests_version_510.cpp diff --git a/bindings/c/CMakeLists.txt b/bindings/c/CMakeLists.txt index 0807806f7e..a75518f760 100644 --- a/bindings/c/CMakeLists.txt +++ b/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 $ @CLUSTER_FILE@ fdb) + add_fdbclient_test( + NAME fdb_c_unit_tests_version_510 + COMMAND $ + @CLUSTER_FILE@ + fdb) add_fdbclient_test( NAME fdb_c_external_client_unit_tests COMMAND $ diff --git a/bindings/c/generate_asm.py b/bindings/c/generate_asm.py index b23166f2e1..32f3232df0 100755 --- a/bindings/c/generate_asm.py +++ b/bindings/c/generate_asm.py @@ -75,9 +75,10 @@ def write_unix_asm(asmfile, functions, prefix): asmfile.write("\n.globl %s%s\n" % (prefix, f)) asmfile.write("%s%s:\n" % (prefix, f)) 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)) diff --git a/bindings/c/test/unit/unit_tests_version_510.cpp b/bindings/c/test/unit/unit_tests_version_510.cpp new file mode 100644 index 0000000000..7e41760855 --- /dev/null +++ b/bindings/c/test/unit/unit_tests_version_510.cpp @@ -0,0 +1,118 @@ +/* + * unit_tests_header_520.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 + +#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 + +#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; +} From 311da4b07a7e5e5cc9293d4cdc54420beaab937f Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Sat, 5 Jun 2021 13:41:36 -0700 Subject: [PATCH 2/5] Explain requirements for fdb_c.g.S implementations --- bindings/c/generate_asm.py | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/bindings/c/generate_asm.py b/bindings/c/generate_asm.py index 32f3232df0..45fd3d7956 100755 --- a/bindings/c/generate_asm.py +++ b/bindings/c/generate_asm.py @@ -74,9 +74,39 @@ 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 + # 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(); } + # [anoyes@docker build]$ 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("\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, #:got_lo12:fdb_api_ptr_%s]\n" % (f)) asmfile.write("\tldr x8, [x8]\n") asmfile.write("\tbr x8\n") else: From cd5c0481ccbe5c146e8100b5477c98871edf3787 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Sat, 5 Jun 2021 14:05:29 -0700 Subject: [PATCH 3/5] Use linker script for external workloads This fixes an issue on Arm with lld: ld.lld: error: relocation R_AARCH64_PREL64 cannot be used against symbol OPENSSL_armcap_P; recompile with -fPIC I think the problem was that lld thought that the shared object might need to interpose OPENSSL_armcap_P at runtime, although honestly I'm not too sure about all this linker stuff. --- bindings/c/CMakeLists.txt | 4 ++++ bindings/c/external_workload.map | 7 +++++++ bindings/java/CMakeLists.txt | 5 +++++ 3 files changed, 16 insertions(+) create mode 100644 bindings/c/external_workload.map diff --git a/bindings/c/CMakeLists.txt b/bindings/c/CMakeLists.txt index a75518f760..561ab8d740 100644 --- a/bindings/c/CMakeLists.txt +++ b/bindings/c/CMakeLists.txt @@ -169,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) diff --git a/bindings/c/external_workload.map b/bindings/c/external_workload.map new file mode 100644 index 0000000000..effca03447 --- /dev/null +++ b/bindings/c/external_workload.map @@ -0,0 +1,7 @@ +{ + global: + workloadFactory; + local: + *; +}; + diff --git a/bindings/java/CMakeLists.txt b/bindings/java/CMakeLists.txt index 09012cdf97..eb89a9de25 100644 --- a/bindings/java/CMakeLists.txt +++ b/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) From 0beb548e99c242d9b2f3802e31510c2363685e79 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Sat, 5 Jun 2021 17:10:41 -0700 Subject: [PATCH 4/5] Improve comments --- bindings/c/generate_asm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bindings/c/generate_asm.py b/bindings/c/generate_asm.py index 45fd3d7956..2f1628fa00 100755 --- a/bindings/c/generate_asm.py +++ b/bindings/c/generate_asm.py @@ -81,7 +81,7 @@ def write_unix_asm(asmfile, functions, prefix): # # 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 - # pass arguments in the arm calling convention we must not use x0-x7 + # 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 @@ -91,7 +91,7 @@ def write_unix_asm(asmfile, functions, prefix): # typedef int (*function)(); # extern function f; # int g() { return f(); } - # [anoyes@docker build]$ cc -S -O3 -fPIC test.c && grep -A 10 '^g:' test.[sS] + # $ cc -S -O3 -fPIC test.c && grep -A 10 '^g:' test.[sS] # g: # .LFB0: # .cfi_startproc From 5d2d4622f6bcb79dcc7cd011143781cb26bf3dba Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Mon, 7 Jun 2021 09:40:26 -0700 Subject: [PATCH 5/5] Update bindings/c/test/unit/unit_tests_version_510.cpp --- bindings/c/test/unit/unit_tests_version_510.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bindings/c/test/unit/unit_tests_version_510.cpp b/bindings/c/test/unit/unit_tests_version_510.cpp index 7e41760855..fbc4143011 100644 --- a/bindings/c/test/unit/unit_tests_version_510.cpp +++ b/bindings/c/test/unit/unit_tests_version_510.cpp @@ -1,5 +1,5 @@ /* - * unit_tests_header_520.cpp + * unit_tests_header_510.cpp * * This source file is part of the FoundationDB open source project *