From df32d17de4a94028eba73bc4f9fdd37d4625083d Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Mon, 19 Sep 2022 13:27:43 -0500 Subject: [PATCH 1/3] Make it work on Clang 11 / libc++ 11. This is more broadly available, e.g. it's the default on Debian Bullseye. This also means we don't need to hard-code the full path to Clang anymore, which is pretty nice. --- .bazelrc | 6 ++---- README.md | 4 ++-- src/workerd/api/r2-rpc.c++ | 2 +- src/workerd/jsg/iterator.h | 10 ++++++++++ 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/.bazelrc b/.bazelrc index 137052ab28b..bfed6b98935 100644 --- a/.bazelrc +++ b/.bazelrc @@ -4,8 +4,8 @@ build --cxxopt='-stdlib=libc++' --host_cxxopt='-stdlib=libc++' build --linkopt='-stdlib=libc++' --host_linkopt='-stdlib=libc++' build --action_env=BAZEL_COMPILER=clang -build --action_env=CC=/usr/lib/llvm-15/bin/clang -build --action_env=CXX=/usr/lib/llvm-15/bin/clang++ +build --action_env=CC=clang +build --action_env=CXX=clang++ build --@capnp-cpp//src/kj:openssl=True --@capnp-cpp//src/kj:zlib=True --@capnp-cpp//src/kj:libdl=True @@ -15,7 +15,6 @@ build --cxxopt='-Wextra' --host_cxxopt='-Wextra' build --cxxopt='-Wno-strict-aliasing' --host_cxxopt='-Wno-strict-aliasing' build --cxxopt='-Wno-sign-compare' --host_cxxopt='-Wno-sign-compare' build --cxxopt='-Wno-unused-parameter' --host_cxxopt='-Wno-unused-parameter' -build --cxxopt='-Wno-deprecated-experimental-coroutine' --host_cxxopt='-Wno-deprecated-experimental-coroutine' build --cxxopt='-Wno-missing-field-initializers' --host_cxxopt='-Wno-missing-field-initializers' build --cxxopt='-Wno-ignored-qualifiers' --host_cxxopt='-Wno-ignored-qualifiers' @@ -23,4 +22,3 @@ build --cxxopt='-Wno-ignored-qualifiers' --host_cxxopt='-Wno-ignored-qualifiers' # TODO(cleanup): Can we disable warnings altogether from our dependencies, without disabling them # for workerd? build --cxxopt='-Wno-ambiguous-reversed-operator' --host_cxxopt='-Wno-ambiguous-reversed-operator' -build --copt='-Wno-deprecated-non-prototype' --host_copt='-Wno-deprecated-non-prototype' diff --git a/README.md b/README.md index 286f9ee5bf0..ffe0c850fe5 100644 --- a/README.md +++ b/README.md @@ -70,8 +70,8 @@ Windows users should run `workerd` under WSL2. To build `workerd`, you need: * [Bazel](https://bazel.build/) -* Clang 15 -* libc++ 15 +* Clang 11+ (e.g. package `clang` on Debian Bullseye) +* libc++ 11+ (e.g. packages `libc++-dev` and `libc++abi-dev` on Debian Bullseye) You may then build using: diff --git a/src/workerd/api/r2-rpc.c++ b/src/workerd/api/r2-rpc.c++ index f1bbc8e9ff2..56fb71caf8f 100644 --- a/src/workerd/api/r2-rpc.c++ +++ b/src/workerd/api/r2-rpc.c++ @@ -113,7 +113,7 @@ kj::Promise doR2HTTPGetRequest(kj::Own client, }); } - return result; + return kj::mv(result); } KJ_IF_MAYBE(m, response.headers->get(headerIds.cfBlobMetadataSize)) { diff --git a/src/workerd/jsg/iterator.h b/src/workerd/jsg/iterator.h index aba86285108..e4254df27ea 100644 --- a/src/workerd/jsg/iterator.h +++ b/src/workerd/jsg/iterator.h @@ -156,6 +156,7 @@ class GeneratorImpl { friend Generator; }; +#if _LIBCPP_VERSION >= 15000 template concept GeneratorCallback = std::invocable&> && std::same_as&>>; @@ -163,6 +164,15 @@ concept GeneratorCallback = std::invocable&> template concept AsyncGeneratorCallback = std::invocable&> && std::same_as, std::invoke_result_t&>>; +#else +// These don't work in libc++ 11. I haven't tried 12-14, but they do work in 15. So we'll just +// skip the extra type checking if we don't have libc++ 15 or newer. +template +concept GeneratorCallback = true; + +template +concept AsyncGeneratorCallback = true; +#endif template class Generator final { From 2e76bd999f362dae9285d7d86bc5289bfc5f60d6 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Mon, 19 Sep 2022 14:08:38 -0500 Subject: [PATCH 2/3] Use `capnp` to embed icudata, remove wd_cc_embed. wd_cc_embed doesn't seem to work on Mac and it was magic copy-pasta to start with so I'm not sure how to fix it. This approach is portable, but does result in a 57MB generated C++ file which is kinda gross. --- BUILD.bazel | 12 +++---- build/scripts/gen-embed.sh | 73 -------------------------------------- build/wd_cc_embed.bzl | 60 ------------------------------- icudata-embed.capnp | 13 +++++++ src/workerd/jsg/setup.c++ | 4 +-- 5 files changed, 20 insertions(+), 142 deletions(-) delete mode 100755 build/scripts/gen-embed.sh delete mode 100644 build/wd_cc_embed.bzl create mode 100644 icudata-embed.capnp diff --git a/BUILD.bazel b/BUILD.bazel index a00e5030f92..584aa842a0b 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -1,12 +1,10 @@ -load("//:build/wd_cc_embed.bzl", "wd_cc_embed") +load("@capnp-cpp//src/capnp:cc_capnp_library.bzl", "cc_capnp_library") -wd_cc_embed( +cc_capnp_library( name = "icudata-embed", - base_name = "icudata-embed", + srcs = ["icudata-embed.capnp"], strip_include_prefix = "", - defines = [ - "WORKERD_ICU_DATA_EMBED=EMBED_com_googlesource_chromium_icu_common_icudtl_dat" - ], - data = ["@com_googlesource_chromium_icu//:icudata"], visibility = ["//visibility:public"], + data = ["@com_googlesource_chromium_icu//:icudata"], + defines = [ "WORKERD_ICU_DATA_EMBED" ], ) diff --git a/build/scripts/gen-embed.sh b/build/scripts/gen-embed.sh deleted file mode 100755 index ff682598e14..00000000000 --- a/build/scripts/gen-embed.sh +++ /dev/null @@ -1,73 +0,0 @@ -#!/bin/bash -# Usage: gen-embed.sh [ ...] - -HEADER=$1 -shift -ASSEMBLY=$1 -shift -SRCS=( $@ ) - -generate_asm() { - SHA=$(sha256sum ${2}) - cat >>$ASSEMBLY << __EOF__ -// ${SHA} -.global ${1}_begin -.type ${1}_begin, @object -.align 8 - -${1}_begin: -.incbin "$2" - -.global ${1}_end -.type ${1}_end, @object -${1}_end: - -__EOF__ -} - -embed() { - INPUT=$1 - NAME=$2 - - # Construct the symbol name. We strip "src/" or "tmp/" from the front and use a different prefix. - SYMBOL_NAME=EMBED_$(tr ./- ___ <<< "${NAME#*/}") - - # Write header declarations. - cat >>$HEADER << __EOF__ -extern const ::kj::byte ${SYMBOL_NAME}_begin; -extern const ::kj::byte ${SYMBOL_NAME}_end; -#define ${SYMBOL_NAME} (::kj::ArrayPtr( \\ - &${SYMBOL_NAME}_begin, &${SYMBOL_NAME}_end)) - -__EOF__ - - # Write asm. - generate_asm $SYMBOL_NAME "$INPUT" -} - - -INCLUDE_GUARD=$(tr a-z./- A-Z___ <<< "$HEADER")_ - -cat >$HEADER << __EOF__ -#ifndef $INCLUDE_GUARD -#define $INCLUDE_GUARD -#include - -__EOF__ - -cat >$ASSEMBLY << __EOF__ -.section ".rodata" -__EOF__ - - -n=${#SRCS[@]} -for ((i=0; i>$HEADER << __EOF__ -#endif // $INCLUDE_GUARD -__EOF__ diff --git a/build/wd_cc_embed.bzl b/build/wd_cc_embed.bzl deleted file mode 100644 index a30e2ee3164..00000000000 --- a/build/wd_cc_embed.bzl +++ /dev/null @@ -1,60 +0,0 @@ -load("//:build/wd_cc_library.bzl", "wd_cc_library") - -def wd_cc_embed_name(file_name): - return "EMBED_" + file_name.replace(".", "_").replace("-", "_").replace("/", "_") - -def _ew_cc_embed_gen_impl(ctx): - # - args = [ctx.expand_location("$(execpath {})".format(l.name)) for l in ctx.attr.outs] - inputs = [] - for ds in ctx.attr.data: - for f in ds.files.to_list(): - inputs.append(f) - - # - args.append(f.path) - args.append(f.short_path) - - ctx.actions.run( - executable = ctx.executable._tool, - tools = ctx.attr._tool.files, - inputs = inputs, - outputs = ctx.outputs.outs, - arguments = args, - mnemonic = "GenEmbed", - ) - - return [DefaultInfo(files = depset(ctx.outputs.outs))] - -wd_cc_embed_gen = rule( - implementation = _ew_cc_embed_gen_impl, - attrs = { - "data": attr.label_list(allow_files = True, mandatory = True), - "outs": attr.output_list(), - "_tool": attr.label( - allow_single_file = True, - executable = True, - cfg = "exec", - default = "//:build/scripts/gen-embed.sh", - ), - }, -) - -def wd_cc_embed( - name, - base_name, - data, - **kwargs): - wd_cc_embed_gen( - name = name + "_gen", - data = data, - outs = [base_name + ".h", base_name + ".S"], - ) - - wd_cc_library( - name = name, - srcs = [base_name + ".S"], - hdrs = [base_name + ".h"], - textual_hdrs = data, - **kwargs - ) diff --git a/icudata-embed.capnp b/icudata-embed.capnp new file mode 100644 index 00000000000..1e437e2a7c7 --- /dev/null +++ b/icudata-embed.capnp @@ -0,0 +1,13 @@ +@0x9ed733e2dd15598b; +# This file is a hack to embed the ICU data file which libicu needs at runtime to do its thing. +# It's inconvenient to ship this file separately so we bake it into the binary. (V8's normal GN +# build can actually do this for us, but we use the Bazel build which doesn't have this option.) +# +# We use Cap'n Proto to do the embedding because it's conveniently cross-platform, but this is +# pretty ugly as it generates a 57MB C++ source file (containing a giant byte array literal). +# It would be nice to use some more direct way of embedding binary data into a symbol in the +# executable, but I couldn't figure out how to do this portably. (See the commit which introduced +# this file to see the old Linux-only solution, but it didn't work on Mac and I didn't want to +# figure it out.) + +const embeddedIcuDataFile :Data = embed "external/com_googlesource_chromium_icu/common/icudtl.dat"; diff --git a/src/workerd/jsg/setup.c++ b/src/workerd/jsg/setup.c++ index 5e5e5fc83a4..d8989975a7a 100644 --- a/src/workerd/jsg/setup.c++ +++ b/src/workerd/jsg/setup.c++ @@ -8,7 +8,7 @@ #include #ifdef WORKERD_ICU_DATA_EMBED -#include +#include #include #endif @@ -101,7 +101,7 @@ V8System::V8System(kj::Own platformParam, kj::ArrayPtr` // containing the data. UErrorCode err = U_ZERO_ERROR; - udata_setCommonData(WORKERD_ICU_DATA_EMBED.begin(), &err); + udata_setCommonData(EMBEDDED_ICU_DATA_FILE->begin(), &err); udata_setFileAccess(UDATA_ONLY_PACKAGES, &err); KJ_ASSERT(err == U_ZERO_ERROR); #else From bbff5bf77be96658bfe588f84f01821721c36914 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Mon, 19 Sep 2022 14:17:20 -0500 Subject: [PATCH 3/3] `DEBUG` is a macro on Apple platforms; avoid this name. --- src/workerd/api/trace.c++ | 2 +- src/workerd/io/worker-interface.capnp | 5 +++-- src/workerd/io/worker.c++ | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/workerd/api/trace.c++ b/src/workerd/api/trace.c++ index a5063218816..1c102393075 100644 --- a/src/workerd/api/trace.c++ +++ b/src/workerd/api/trace.c++ @@ -192,7 +192,7 @@ double TraceLog::getTimestamp() { kj::StringPtr TraceLog::getLevel() { switch (log.logLevel) { - case LogLevel::DEBUG: return "debug"_kj; + case LogLevel::DEBUG_: return "debug"_kj; case LogLevel::INFO: return "info"_kj; case LogLevel::LOG: return "log"_kj; case LogLevel::WARN: return "warn"_kj; diff --git a/src/workerd/io/worker-interface.capnp b/src/workerd/io/worker-interface.capnp index 1b412621c35..36c8cb864f6 100644 --- a/src/workerd/io/worker-interface.capnp +++ b/src/workerd/io/worker-interface.capnp @@ -4,7 +4,8 @@ @0xf7958855f6746344; -$import "/capnp/c++.capnp".namespace("workerd::rpc"); +using Cxx = import "/capnp/c++.capnp"; +$Cxx.namespace("workerd::rpc"); using import "/capnp/compat/http-over-capnp.capnp".HttpMethod; using import "/capnp/compat/http-over-capnp.capnp".HttpService; @@ -17,7 +18,7 @@ struct Trace @0x8e8d911203762d34 { logLevel @1 :Level; enum Level { - debug @0; + debug @0 $Cxx.name("debug_"); # avoid collision with macro on Apple platforms info @1; log @2; warn @3; diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index ca3da0ef677..ca8656f2616 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -624,7 +624,7 @@ struct Worker::Isolate::Impl { jsg::check(console.As()->Set(context, methodStr, f)); }; - setHandler("debug", LogLevel::DEBUG); + setHandler("debug", LogLevel::DEBUG_); setHandler("error", LogLevel::ERROR); setHandler("info", LogLevel::INFO); setHandler("log", LogLevel::LOG);