Require C++17 #9481

pdillinger · 2022-02-01T05:34:35Z

Summary: Drop support for some old compilers by requiring C++17 standard
(or higher). See #9388

First modification based on this is to remove some conditional compilation in slice.h (also
better for ODR)

Also in this PR:

Fix some Makefile formatting that seems to affect ASSERT_STATUS_CHECKED config in
some cases
Add c_test to NON_PARALLEL_TEST in Makefile
Fix a clang-analyze reported "potential leak" in lru_cache_test
Better "compatibility" definition of DEFINE_uint32 for old versions of gflags
Fix a linking problem with shared libraries in Makefile (./random_test: error while loading shared libraries: librocksdb.so.6.29: cannot open shared object file: No such file or directory)
Always set ROCKSDB_SUPPORT_THREAD_LOCAL and use thread_local (from C++11)
- TODO in later PR: clean up that obsolete flag
Fix a cosmetic typo in c.h (Typo on variable name #9488)

Test Plan: CircleCI config substantially updated.

Upgrade to latest Ubuntu images for each release
Generally prefer Ubuntu 20, but keep a couple Ubuntu 16 builds with oldest supported
compilers, to ensure compatibility
Remove .circleci/cat_ignore_eagain except for Ubuntu 16 builds, because this is to work
around a kernel bug that should not affect anything but Ubuntu 16.
Remove designated gcc-9 build, because the default linux build now uses GCC 9 from
Ubuntu 20.
Add some apt-key add to fix some apt "couldn't be verified" errors
Generally drop SKIP_LINK=1; work-around no longer needed
Generally add-apt-repository before apt-get update as manual testing indicated the
reverse might not work.

Travis:

Use gcc-7 by default (remove specific gcc-7 and gcc-4.8 builds)
TODO in later PR: fix s390x "Assembler messages: Error: invalid switch -march=z14" failure

AppVeyor:

Completely dropped because we are dropping VS2015 support and CircleCI covers
VS >= 2017

Also local testing with old gflags (out of necessity when using ROCKSDB_NO_FBCODE=1).

Summary: Drop support for some old compilers by requiring C++17 standard (or higher). See facebook#9388 Test Plan: CI config updated TODO: finish sanitizing CircleCI config

mrambacher

This generally looks good to me.

For the Linux boxes, I wonder if using "-j$(nproc)" might be a better paradigm. The parallelism seems to inconsistent.

mrambacher · 2022-02-01T13:57:38Z

examples/Makefile

-	$(CXX) $(CXXFLAGS) $@.cc -o$@ ../librocksdb.a -I../include -O2 -std=c++11 $(PLATFORM_LDFLAGS) $(PLATFORM_CXXFLAGS) $(EXEC_LDFLAGS)
+	$(CXX) $(CXXFLAGS) $@.cc -o$@ ../librocksdb.a -I../include -O2 -std=c++17 $(PLATFORM_LDFLAGS) $(PLATFORM_CXXFLAGS) $(EXEC_LDFLAGS)


What happens if the examples do not specify a --std? Won't it build with the default for the compiler? And if that is less than 17, what will happen?

If we are requiring that std be set and be at least 17, then that probably also needs to be updated on the README/build instructions. If 17 is the lowest we are supporting (and most/all compilers default to it or higher), then I do not know this line is necessary.

mrambacher · 2022-02-01T13:59:37Z

.circleci/config.yml

-      - run: (mkdir build && cd build && cmake -DWITH_GFLAGS=1 .. && make V=1 -j20 && ctest -j20) 2>&1 | .circleci/cat_ignore_eagain
+      - run: (mkdir build && cd build && cmake -DWITH_GFLAGS=1 .. && make V=1 -j20 && ctest -j20)


Can you please add a comment (could be in the PR and not the config file) as to why you are dropping the redirection everywhere?

pdillinger · 2022-02-01T20:08:09Z

I wonder if using "-j$(nproc)" might be a better paradigm

I'm not touching this at this time.

What happens if the examples do not specify a --std? Won't it build with the default for the compiler? And if that is less than 17, what will happen?

You'll get errors like undefined std::string_view

facebook-github-bot · 2022-02-02T17:46:00Z

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mrambacher · 2022-02-03T00:44:55Z

.circleci/config.yml

    resource_class: 2xlarge
    steps:
      - pre-steps
      - install-gflags
      - upgrade-cmake
-      - run: (mkdir build && cd build && cmake -DWITH_GFLAGS=1 .. && make V=1 -j20 && ctest -j20) 2>&1 | .circleci/cat_ignore_eagain
+      - run: (mkdir build && cd build && cmake -DWITH_GFLAGS=1 .. && make V=1 -j20 && ctest -j20)


Should this be -j 32? Every other use of 2xlarge used -j32

This PR is not about parallelism in CircleCI

mrambacher · 2022-02-03T00:45:07Z

.circleci/config.yml

@@ -338,7 +340,7 @@ jobs:
      - pre-steps
      - install-gflags
      - install-benchmark
-      - run: (mkdir build && cd build && cmake -DWITH_GFLAGS=1 -DWITH_BENCHMARK=1 .. && make V=1 -j20 && ctest -j20) 2>&1 | .circleci/cat_ignore_eagain
+      - run: (mkdir build && cd build && cmake -DWITH_GFLAGS=1 -DWITH_BENCHMARK=1 .. && make V=1 -j20 && ctest -j20)


This PR is not about parallelism in CircleCI

mrambacher · 2022-02-03T00:46:19Z

.circleci/config.yml

    resource_class: xlarge
    steps:
      - pre-steps
      - install-benchmark
-      - run: DEBUG_LEVEL=0 make microbench 2>&1 | .circleci/cat_ignore_eagain
+      - run: DEBUG_LEVEL=0 make microbench


No make parallelism?

This PR is not about parallelism in CircleCI

mrambacher · 2022-02-03T00:46:48Z

.circleci/config.yml

    resource_class: large
    steps:
      - pre-steps
      - install-gflags
      - run:
          name: "Build examples"
          command: |
-            OPT=-DTRAVIS V=1 make -j4 static_lib && cd examples && make -j4 | ../.circleci/cat_ignore_eagain
+            OPT=-DTRAVIS V=1 make -j4 static_lib && cd examples && make -j4


This PR is not about parallelism in CircleCI

mrambacher · 2022-02-03T13:23:30Z

CMakeLists.txt

-if(HAVE_THREAD_LOCAL)
-  add_definitions(-DROCKSDB_SUPPORT_THREAD_LOCAL)
-endif()
+# Thread-local is part of C++11 (TODO: clean up this flag)


C++17 or part of the C++ standard. (It is a bit confusing to say C++11 when we say we support 17 only.

C++11 is factually accurate. I'm pointing out that the check has been effectively obsolete for a long time.

mrambacher · 2022-02-03T13:24:08Z

INSTALL.md

@@ -50,7 +50,7 @@ to build a portable binary, add `PORTABLE=1` before your make commands, like thi
 ## Supported platforms

 * **Linux - Ubuntu**
-    * Upgrade your gcc to version at least 4.8 to get C++11 support.
+    * Upgrade your gcc to version at least 7 to get C++17 support.


Should this mention clang?

It's not the default compiler on Ubuntu

Understood that it is not the default compiler. Nowhere in INSTALL does it say we support clang (on any platform) and what the minimum clang version is. Can we add that somewhere?

It's near the top, "We do depend on newer gcc/clang with C++17 support (GCC >= 7, Clang >= 5)."

mrambacher · 2022-02-03T13:29:51Z

include/rocksdb/slice.h

@@ -44,11 +41,9 @@ class Slice {
  /* implicit */
  Slice(const std::string& s) : data_(s.data()), size_(s.size()) {}

-#ifdef __cpp_lib_string_view
  // Create a slice that refers to the same contents as "sv"
  /* implicit */
  Slice(std::string_view sv) : data_(sv.data()), size_(sv.size()) {}


Should this pass in a const std::string_view& ?

facebook-github-bot · 2022-02-03T16:49:13Z

@pdillinger has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-03T16:50:58Z

@pdillinger has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-04T18:04:53Z

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-02-04T19:59:59Z

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

This reverts commit fd3e0f4.

Summary: After facebook#9481, we are using newer default compiler for build-format-compatible CircleCI nightly job, which fails on building 2.2.fb.branch branch because it tries to use a pre-compiled libsnappy.a that is checked into the repo (!). This works around that by setting SNAPPY_LDFLAGS=-lsnappy, which is only understood by such old versions. Test Plan: Run check_format_compatible.sh on Ubuntu 20 AWS machine, watch nightly run

Summary: ... seen only in internal clang-analyze runs after #9481 * Mostly, this works around falsely reported leaks by using std::unique_ptr in some places where clang-analyze was getting confused. (I didn't see any changes in C++17 that could make our Status implementation leak memory.) * Also fixed SetBGError returning address of a stack variable. * Also fixed another false null deref report by adding an assert. Also, use SKIP_LINK=1 to speed up `make analyze` Pull Request resolved: #9515 Test Plan: Was able to reproduce the reported errors locally and verify they're fixed (except SetBGError). Otherwise, existing tests Reviewed By: hx235 Differential Revision: D34054630 Pulled By: pdillinger fbshipit-source-id: 38600ef3da75ddca307dff96b7a1a523c2885c2e

Summary: After #9481, we are using newer default compiler for build-format-compatible CircleCI nightly job, which fails on building 2.2.fb.branch branch because it tries to use a pre-compiled libsnappy.a that is checked into the repo (!). This works around that by setting SNAPPY_LDFLAGS=-lsnappy, which is only understood by such old versions. Pull Request resolved: #9517 Test Plan: Run check_format_compatible.sh on Ubuntu 20 AWS machine, watch nightly run Reviewed By: hx235 Differential Revision: D34055561 Pulled By: pdillinger fbshipit-source-id: 45f9d428dd082f026773bfa8d9dd4dad66fc9378

Require C++17

d69630b

Summary: Drop support for some old compilers by requiring C++17 standard (or higher). See facebook#9388 Test Plan: CI config updated TODO: finish sanitizing CircleCI config

facebook-github-bot added the CLA Signed label Feb 1, 2022

pdillinger added 3 commits January 31, 2022 22:02

Fixes

3cc9d55

Major CircleCI updates

bd0a6dc

Another fix

dbbdda7

mrambacher approved these changes Feb 1, 2022

View reviewed changes

pdillinger added 5 commits February 1, 2022 09:53

More fixes

8df4a63

Fix clang-10 (hopefully)

a89729b

Merge remote-tracking branch 'origin/main' into cxx17

8b646fb

More updates

ec51fe2

Merge remote-tracking branch 'origin/main' into cxx17

7354e77

pdillinger added 6 commits February 1, 2022 12:55

Try to fix Travis, clang analyzer FP

fb72271

Attempt fix Travis, AppVeyor, LIB_MODE=shared

f2dadba

Drop AppVeyor (because we are dropping VS2015)

866bf0c

Attempt^2 fix LIB_MODE=shared

90e7c89

Assume thread_local + typo in 9488

20e96db

Merge remote-tracking branch 'origin/main' into cxx17

e679a35

pdillinger marked this pull request as ready for review February 2, 2022 17:45

pdillinger requested a review from mrambacher February 2, 2022 17:45

mrambacher reviewed Feb 3, 2022

View reviewed changes

pdillinger added 2 commits February 3, 2022 08:24

Merge remote-tracking branch 'origin/main' into cxx17

b8c9452

Minor edits

f5fc954

facebook-github-bot closed this in fd3e0f4 Feb 5, 2022

hx235 added a commit to hx235/rocksdb that referenced this pull request Feb 7, 2022

Revert "Require C++17 (facebook#9481)"

d0f52de

This reverts commit fd3e0f4.

pdillinger mentioned this pull request Feb 7, 2022

Work around some new clang-analyze failures #9515

Closed

pdillinger mentioned this pull request Feb 7, 2022

Work around snappy linker issue with newer compilers #9517

Closed

This was referenced Feb 8, 2022

Travis s390x Makefile builds broken with gcc-7 #9524

Closed

Upgrade to Ubuntu 20.04 in our CircleCI config #9486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require C++17 #9481

Require C++17 #9481

pdillinger commented Feb 1, 2022 •

edited

Loading

mrambacher left a comment

mrambacher Feb 1, 2022

mrambacher Feb 1, 2022

pdillinger commented Feb 1, 2022

facebook-github-bot commented Feb 2, 2022

mrambacher Feb 3, 2022

pdillinger Feb 3, 2022

mrambacher Feb 3, 2022

pdillinger Feb 3, 2022

mrambacher Feb 3, 2022

pdillinger Feb 3, 2022

mrambacher Feb 3, 2022

pdillinger Feb 3, 2022

mrambacher Feb 3, 2022

pdillinger Feb 3, 2022

mrambacher Feb 3, 2022

pdillinger Feb 3, 2022

mrambacher Feb 3, 2022

pdillinger Feb 5, 2022

mrambacher Feb 3, 2022

pdillinger Feb 3, 2022

facebook-github-bot commented Feb 3, 2022

facebook-github-bot commented Feb 3, 2022

facebook-github-bot commented Feb 4, 2022

facebook-github-bot commented Feb 4, 2022

		$(CXX) $(CXXFLAGS) $@.cc -o$@ ../librocksdb.a -I../include -O2 -std=c++11 $(PLATFORM_LDFLAGS) $(PLATFORM_CXXFLAGS) $(EXEC_LDFLAGS)
		$(CXX) $(CXXFLAGS) $@.cc -o$@ ../librocksdb.a -I../include -O2 -std=c++17 $(PLATFORM_LDFLAGS) $(PLATFORM_CXXFLAGS) $(EXEC_LDFLAGS)

		- run: (mkdir build && cd build && cmake -DWITH_GFLAGS=1 .. && make V=1 -j20 && ctest -j20) 2>&1 \| .circleci/cat_ignore_eagain
		- run: (mkdir build && cd build && cmake -DWITH_GFLAGS=1 .. && make V=1 -j20 && ctest -j20)

Require C++17 #9481

Require C++17 #9481

Conversation

pdillinger commented Feb 1, 2022 • edited Loading

mrambacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdillinger commented Feb 1, 2022

facebook-github-bot commented Feb 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 3, 2022

facebook-github-bot commented Feb 3, 2022

facebook-github-bot commented Feb 4, 2022

facebook-github-bot commented Feb 4, 2022

pdillinger commented Feb 1, 2022 •

edited

Loading