Bump protobuf version and add Abseil as compiler dependency. (p4lang#…

…4463) * Bump protobuf version. Fix some missed dependencies here and there * Grab abseil using FetchContent * Disable unity builds for Abseil * Try to bumpprotobuf version in bazel * Pull abseil * Address review comments * Fix UB in json parser that remained from GMP times * Ensure sanitizer options are propagated down to dependencies (protobuf, abseil, etc.) * Some readme refines * See if we can workaround ubsan + int128 bug * Clarify ubsan issue workaround * Fix typo in README.md Co-authored-by: Vladimír Štill <[email protected]> --------- Co-authored-by: Vladimír Štill <[email protected]>
komaljai · Feb 26, 2024 · 6b9d005 · 6b9d005
1 parent c533f06
commit 6b9d005
Show file tree

Hide file tree

Showing 10 changed files with 121 additions and 73 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -23,6 +23,10 @@ endif()
 
 project (P4C)
 
+# set (CMAKE_CXX_EXTENSIONS OFF) # prefer using -std=c++17 rather than -std=gnu++17
+set (CMAKE_CXX_STANDARD 17)
+set (CMAKE_CXX_STANDARD_REQUIRED ON)
+
 set (CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake)
 set (P4C_RUNTIME_OUTPUT_DIRECTORY bin)
 set (P4C_LIBRARY_OUTPUT_DIRECTORY lib)
@@ -147,10 +151,36 @@ if(BUILD_STATIC_RELEASE_SANS_GLIBC)
   add_definitions(-DP4C_STATIC_BUILD)
 endif()
 
+# Ensure we enable sanitizers before fetching dependencies so the correct flags
+# are propagated below
+if (ENABLE_SANITIZERS)
+  append("-fsanitize=undefined,address" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+  # ubsan may generate 128-bit multiplication with explicit overflow check. This
+  # is compiled down to _muloti4 libcall that is provided by compiler-rr but not
+  # by libgcc
+  # So, on Linux where clang uses libgcc by default we'd need to switch to
+  # compiler-rt, though we'd pull unwinder from libgcc.
+  # See https://bugs.llvm.org/show_bug.cgi?id=16404 for more information
+  if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang" AND UNIX AND NOT APPLE)
+    append("--rtlib=compiler-rt --unwindlib=libgcc" CMAKE_EXE_LINKER_FLAGS)
+  endif()
+endif ()
+
+if (BUILD_AUTO_VAR_INIT_PATTERN)
+  add_cxx_compiler_option  ("-ftrivial-auto-var-init=pattern")
+endif ()
+
 # Required tools and libraries.
 find_package (PythonInterp 3 REQUIRED)
 find_package (FLEX 2.0 REQUIRED)
 find_package (BISON 3.0 REQUIRED)
+# If we enable GTest, install GoogleTest with the appropriate options.
+if (ENABLE_GTESTS)
+  include(GoogleTest)
+  p4c_obtain_googletest()
+endif ()
+include(Abseil)
+p4c_obtain_abseil()
 include(Protobuf)
 p4c_obtain_protobuf()
 
@@ -242,10 +272,6 @@ enable_testing ()
 #   set (CPACK_PACKAGE_VERSION_PATCH ${CPACK_PACKAGE_VERSION_PATCH}-${__P4C_VERSION_RC})
 # endif ()
 
-# set (CMAKE_CXX_EXTENSIONS OFF) # prefer using -std=c++17 rather than -std=gnu++17
-set (CMAKE_CXX_STANDARD 17)
-set (CMAKE_CXX_STANDARD_REQUIRED ON)
-
 add_cxx_compiler_option ("-Wall")
 add_cxx_compiler_option ("-Wextra")
 add_cxx_compiler_option ("-Wno-overloaded-virtual")
@@ -262,14 +288,6 @@ if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "Ap
   add_cxx_compiler_option ("-Wno-gnu-zero-variadic-macro-arguments")
 endif()
 
-if (ENABLE_SANITIZERS)
-  append("-fsanitize=undefined,address" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
-endif ()
-
-if (BUILD_AUTO_VAR_INIT_PATTERN)
-  add_cxx_compiler_option  ("-ftrivial-auto-var-init=pattern")
-endif ()
-
 if (ENABLE_WERROR)
   add_cxx_compiler_option  ("-Werror")
   if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
@@ -358,12 +376,6 @@ set (IR_GENERATOR ${IR_GENERATOR_DIRECTORY}/irgenerator)
 # Component libraries: must be defined before being used in the back end executables.
 set (P4C_LIBRARIES controlplane midend frontend ir p4ctoolkit ir-generated)
 
-# If we enable GTest, install GoogleTest with the appropriate options.
-if (ENABLE_GTESTS)
-  include(GoogleTest)
-  p4c_obtain_googletest()
-endif ()
-
 # The order of adding subdirectories matters because of target dependencies.
 add_subdirectory (tools/driver)
 add_subdirectory (lib)

diff --git a/README.md b/README.md
@@ -193,13 +193,15 @@ sudo dpkg -i /path/to/package.deb
        library. Default is ON.
      - `-DENABLE_GTESTS=ON|OFF`. Enable building and running GTest unit tests.
        Default is ON.
-     - `-DP4C_USE_PREINSTALLED_PROTOBUF=ON|OFF`. Try to find a system version of Protobuf instead of a CMake version.
+     - `-DP4C_USE_PREINSTALLED_ABSEIL=ON|OFF`. Try to find a system version of Abseil instead of a fetched one. Default is OFF.
+     - `-DP4C_USE_PREINSTALLED_PROTOBUF=ON|OFF`. Try to find a system version of Protobuf instead of a CMake version. Default is OFF.
+     - `-DENABLE_ABSEIL_STATIC=ON|OFF`. Enable the use of static abseil libraries. Default is ON. Only has an effect when `P4C_USE_PREINSTALLED_ABSEIL` is enabled.
      - `-DENABLE_PROTOBUF_STATIC=ON|OFF`. Enable the use of static protobuf libraries. Default is ON.
        Only has an effect when `P4C_USE_PREINSTALLED_PROTOBUF` is enabled.
      - `-DENABLE_MULTITHREAD=ON|OFF`. Use multithreading.  Default is
        OFF.
      - `-DBUILD_LINK_WITH_GOLD=ON|OFF`. Use Gold linker for build if available.
-     - `-DBUILD_LINK_WITH_LLD=ON|OFF`. Use LLD linker for build if available (overrides BUILD_LINK_WITH_GOLD).
+     - `-DBUILD_LINK_WITH_LLD=ON|OFF`. Use LLD linker for build if available (overrides `BUILD_LINK_WITH_GOLD`).
      - `-DENABLE_LTO=ON|OFF`. Use Link Time Optimization (LTO).  Default is OFF.
      - `-DENABLE_WERROR=ON|OFF`. Treat warnings as errors.  Default is OFF.
      - `-DCMAKE_UNITY_BUILD=ON|OFF `. Enable [unity builds](https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html) for faster compilation.  Default is OFF.
@@ -240,7 +242,7 @@ use them, but YMMV.
 
 - GNU Bison and Flex for the parser and lexical analyzer generators.
 
-- Google Protocol Buffers 3.0 or higher for control plane API generation
+- Google Protocol Buffers v3.25.3 or higher for control plane API generation
 
 - C++ boost library
 
@@ -274,16 +276,18 @@ For documentation building:
 
 `p4c` also depends on Google Protocol Buffers (Protobuf). `p4c` requires version
 3.0 or higher, so the packaged version provided in Ubuntu 20.04 **should**
-work. However, P4C typically installs its own version of Protobuf using CMake's Fetchcontent module
-(at the moment, 3.21.12). If you are experiencing issues with the Protobuf version shipped with your OS distribution, we recommend that to install Protobuf 3.21.12 from source. You can find instructions
-[here](https://github.com/protocolbuffers/protobuf/blob/v3.21.12/src/README.md).
-After cloning Protobuf and before you build, check-out version 3.21.12:
+work. However, P4C typically installs its own version of Protobuf using CMake's `FetchContent` module
+(at the moment, 3.25.3). If you are experiencing issues with the Protobuf version shipped with your OS distribution, we recommend that to install Protobuf 3.25.3 from source. You can find instructions
+[here](https://github.com/protocolbuffers/protobuf/blob/v3.25.3/src/README.md).
+After cloning Protobuf and before you build, check-out version 3.25.3:
 
-`git checkout v3.21.12`
+`git checkout v3.25.3`
 
 Please note that while all Protobuf versions newer than 3.0 should work for
 `p4c` itself, you may run into trouble with some extensions and other p4lang
-projects unless you install version 3.21.12.
+projects unless you install version 3.25.3.
+
+`p4c` also depends on Google Abseil library. This library is also a pre-requisite for Protobuf of any version newer than 3.21. Therefore the use of Protobuf of suitable version automatically fulfils Abseil dependency. P4C typically installs its own version of Abseil using CMake's `FetchContent` module (Abseil LTS 20240116.1 at the moment).
 
 ### CMake
 p4c requires a CMake version of at least 3.16.3 or higher. On older systems, a newer version of CMake can be installed using `pip3 install --user cmake==3.16.3`. We have a CI test on Ubuntu 18.04 that uses this option, but there is no guarantee that this will lead to a successful build.
@@ -348,7 +352,7 @@ Installing on macOS:
   ```
   Homebrew offers a `protobuf` formula. It installs version 3.2, which should
   work for p4c itself but may cause problems with some extensions. It's
-  preferable to use the version of Protobuf which is supplied with CMake's fetchcontent (3.21.12).
+  preferable to use the version of Protobuf which is supplied with CMake's fetchcontent (3.25.3).
 
   The `protobuf` formula requires the following CMake variables to be set,
   otherwise CMake does not find the libraries or fails in linking. It is likely

diff --git a/backends/dpdk/CMakeLists.txt b/backends/dpdk/CMakeLists.txt
@@ -40,9 +40,7 @@ add_custom_command(
 add_dependencies(dpdk_runtime_dir controlplane)
 
 add_library(dpdk_runtime STATIC ${DPDK_P4RUNTIME_INFO_GEN_SRCS})
-target_include_directories(dpdk_runtime
-  SYSTEM BEFORE PUBLIC ${Protobuf_INCLUDE_DIRS}
-)
+target_link_libraries(dpdk_runtime controlplane)
 
 # Silence various warnings as the root issue is out of our control, example https://github.com/protocolbuffers/protobuf/issues/7140
 set_source_files_properties(${DPDK_P4RUNTIME_INFO_GEN_SRCS} {$DPDK_P4RUNTIME_INFO_GEN_HDRS} PROPERTIES COMPILE_FLAGS "-Wno-unused-parameter -Wno-array-bounds -Wno-error")

diff --git a/backends/p4tools/common/CMakeLists.txt b/backends/p4tools/common/CMakeLists.txt
@@ -68,5 +68,5 @@ target_include_directories(
   SYSTEM BEFORE PUBLIC ${Protobuf_INCLUDE_DIRS}
 )
 
-add_dependencies(p4tools-control-plane controlplane)
+target_link_libraries(p4tools-control-plane controlplane)
 
diff --git a/backends/p4tools/modules/testgen/CMakeLists.txt b/backends/p4tools/modules/testgen/CMakeLists.txt
@@ -106,7 +106,7 @@ configure_file(register.h.in register.h)
 # The library.
 add_p4tools_library(testgen ${TESTGEN_SOURCES})
 # Make sure the testgen library and anything that links to it see both the Z3 and Inja includes.
-target_link_libraries(testgen PUBLIC p4tools-common inja)
+target_link_libraries(testgen PUBLIC p4tools-common ${Protobuf_LIBRARY} inja)
 if (TESTGEN_INCLUDES)
   target_include_directories(testgen ${TESTGEN_INCLUDES})
 endif()

diff --git a/bazel/p4c_deps.bzl b/bazel/p4c_deps.bzl
@@ -72,12 +72,19 @@ filegroup(
             strip_prefix = "googletest-1.13.0",
             sha256 = "ad7fdba11ea011c1d925b3289cf4af2c66a352e18d4c7264392fead75e919363",
         )
+    if not native.existing_rule("com_google_absl"):
+        http_archive(
+            name = "com_google_absl",
+            url = "https://github.com/abseil/abseil-cpp/releases/download/20240116.1/abseil-cpp-20240116.1.tar.gz",
+            strip_prefix = "abseil-cpp-20240116.1",
+            sha256 = "3c743204df78366ad2eaf236d6631d83f6bc928d1705dd0000b872e53b73dc6a",
+        )
     if not native.existing_rule("com_google_protobuf"):
         http_archive(
             name = "com_google_protobuf",
-            url = "https://github.com/protocolbuffers/protobuf/releases/download/v21.10/protobuf-all-21.10.tar.gz",
-            strip_prefix = "protobuf-21.10",
-            sha256 = "6fc9b6efc18acb2fd5fb3bcf981572539c3432600042b662a162c1226b362426",
+            url = "https://github.com/protocolbuffers/protobuf/releases/download/v25.3/protobuf-25.3.tar.gz",
+            strip_prefix = "protobuf-25.3",
+            sha256 = "d19643d265b978383352b3143f04c0641eea75a75235c111cc01a1350173180e",
         )
     if not native.existing_rule("rules_foreign_cc"):
         http_archive(

diff --git a/cmake/Abseil.cmake b/cmake/Abseil.cmake
@@ -0,0 +1,55 @@
+macro(p4c_obtain_abseil)
+  option(
+    P4C_USE_PREINSTALLED_ABSEIL
+    "Look for a preinstalled version of Abseil in the system instead of installing the library using FetchContent."
+    OFF
+  )
+
+  # If P4C_USE_PREINSTALLED_ABSEIL is ON just try to find a preinstalled version of Abseil.
+  if(P4C_USE_PREINSTALLED_ABSEIL)
+    if(ENABLE_ABSEIL_STATIC)
+      set(SAVED_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES})
+      set(CMAKE_FIND_LIBRARY_SUFFIXES .a)
+    endif()
+
+    set(CMAKE_FIND_PACKAGE_PREFER_CONFIG TRUE)
+    find_package(absl REQUIRED)
+
+    if(ENABLE_ABSEIL_STATIC)
+      set(CMAKE_FIND_LIBRARY_SUFFIXES ${SAVED_CMAKE_FIND_LIBRARY_SUFFIXES})
+    endif()
+  else()
+    set(P4C_ABSEIL_VERSION "20240116.1")
+    message("Fetching Abseil version ${P4C_ABSEIL_VERSION} for P4C...")
+
+    # Unity builds do not work for Abseil...
+    set(CMAKE_UNITY_BUILD_PREV ${CMAKE_UNITY_BUILD})
+    set(CMAKE_UNITY_BUILD OFF)
+
+    # Print out download state while setting up Abseil.
+    set(FETCHCONTENT_QUIET_PREV ${FETCHCONTENT_QUIET})
+    set(FETCHCONTENT_QUIET OFF)
+
+    set(ABSL_USE_EXTERNAL_GOOGLETEST ON)
+    set(ABSL_FIND_GOOGLETEST OFF)
+    set(ABSL_BUILD_TESTING OFF)
+    set(ABSL_ENABLE_INSTALL OFF)
+    set(ABSL_USE_SYSTEM_INCLUDES ON)
+    set(ABSL_PROPAGATE_CXX_STD ON)
+
+    FetchContent_Declare(
+      abseil
+      URL https://github.com/abseil/abseil-cpp/releases/download/${P4C_ABSEIL_VERSION}/abseil-cpp-${P4C_ABSEIL_VERSION}.tar.gz
+      URL_HASH SHA256=3c743204df78366ad2eaf236d6631d83f6bc928d1705dd0000b872e53b73dc6a
+      USES_TERMINAL_DOWNLOAD TRUE
+      GIT_PROGRESS TRUE
+    )
+    fetchcontent_makeavailable_but_exclude_install(abseil)
+
+    # Reset temporary variable modifications.
+    set(CMAKE_UNITY_BUILD ${CMAKE_UNITY_BUILD_PREV})
+    set(FETCHCONTENT_QUIET ${FETCHCONTENT_QUIET_PREV})
+  endif()
+
+  message("Done with setting up Abseil for P4C.")
+endmacro(p4c_obtain_abseil)
diff --git a/cmake/Protobuf.cmake b/cmake/Protobuf.cmake
@@ -29,10 +29,7 @@ macro(p4c_obtain_protobuf)
     # See https://github.com/p4lang/p4c/issues/4316
     set(Protobuf_LIBRARY "protobuf::libprotobuf")
   else()
-    # Google introduced another breaking change with protobuf 22.x by adding abseil as a new
-    # dependency. https://protobuf.dev/news/2022-08-03/#abseil-dep We do not want abseil, so we stay
-    # with 21.x for now.
-    set(P4C_PROTOBUF_VERSION "21.12")
+    set(P4C_PROTOBUF_VERSION "25.3")
     message("Fetching Protobuf version ${P4C_PROTOBUF_VERSION} for P4C...")
 
     # Unity builds do not work for Protobuf...
@@ -49,13 +46,15 @@ macro(p4c_obtain_protobuf)
     set(protobuf_BUILD_PROTOC_BINARIES ON CACHE BOOL "Build libprotoc and protoc compiler.")
     # Only ever build the static library. It is not safe to link with a local dynamic version.
     set(protobuf_BUILD_SHARED_LIBS OFF CACHE BOOL "Build Shared Libraries")
-    # This is necessary to be able to call FindPackage.
-    set(protobuf_INSTALL ON CACHE BOOL "Install Protobuf")
+    set(protobuf_INSTALL OFF CACHE BOOL "Install Protobuf")
+    set(protobuf_ABSL_PROVIDER "package" CACHE STRING "Use system-provided abseil")
+    set(protobuf_BUILD_EXPORT OFF)
+    set(utf8_range_ENABLE_INSTALL OFF)
 
     fetchcontent_declare(
       protobuf
-      URL https://github.com/protocolbuffers/protobuf/releases/download/v${P4C_PROTOBUF_VERSION}/protobuf-cpp-3.${P4C_PROTOBUF_VERSION}.tar.gz
-      URL_HASH SHA256=4eab9b524aa5913c6fffb20b2a8abf5ef7f95a80bc0701f3a6dbb4c607f73460
+      URL https://github.com/protocolbuffers/protobuf/releases/download/v${P4C_PROTOBUF_VERSION}/protobuf-${P4C_PROTOBUF_VERSION}.tar.gz
+      URL_HASH SHA256=d19643d265b978383352b3143f04c0641eea75a75235c111cc01a1350173180e
       USES_TERMINAL_DOWNLOAD TRUE
       GIT_PROGRESS TRUE
     )

diff --git a/lib/json.cpp b/lib/json.cpp
@@ -34,33 +34,9 @@ void IJson::dump() const { std::cout << toString(); }
 
 JsonValue *JsonValue::null = new JsonValue();
 
-big_int JsonValue::makeValue(long long v) {
-    if (v >= 0) {
-        return makeValue(static_cast<unsigned long long>(v));
-    } else {
-        // works for smallest long long value as well, because the bit
-        // representation for -(1 << 63) - as a long long - and for (1 << 63) -
-        // as an unsigned long long - is the same.
-        return -1 * makeValue(static_cast<unsigned long long>(-v));
-    }
-}
-
-big_int JsonValue::makeValue(unsigned long long v) {
-    big_int tmp = v;
-    return tmp;
-}
-
-// According to the GMP documentation
-// (https://gmplib.org/manual/C_002b_002b-Interface-Integers.html), mpz_class
-// cannot be constructed from a "long long". This means that on systems where
-// "long" != "long long", we cannot construct the "value" member directly from
-// parameter v. Instead, we use this suggested workaround:
-// https://stackoverflow.com/questions/6598265/convert-uint64-to-gmp-mpir-number
-// Because the "value" member is const, we use a helper (makeValue) to
-// initialize it.
-JsonValue::JsonValue(long long v) : tag(Kind::Number), value(makeValue(v)) {}
+JsonValue::JsonValue(long long v) : tag(Kind::Number), value(v) {}
 
-JsonValue::JsonValue(unsigned long long v) : tag(Kind::Number), value(makeValue(v)) {}
+JsonValue::JsonValue(unsigned long long v) : tag(Kind::Number), value(v) {}
 
 void JsonValue::serialize(std::ostream &out) const {
     switch (tag) {

diff --git a/lib/json.h b/lib/json.h
@@ -100,9 +100,6 @@ class JsonValue final : public IJson {
             throw std::logic_error("Incorrect constructor called");
     }
 
-    static big_int makeValue(long long v);
-    static big_int makeValue(unsigned long long v);
-
     const Kind tag;
     const big_int value = 0;
     const cstring str = nullptr;