Work around some new clang-analyze failures #9515

pdillinger · 2022-02-07T21:51:30Z

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

Test Plan: Was able to reproduce the reported errors locally and verify
they're fixed (except SetBGError). Otherwise, existing tests

Summary: ... seen only in internal clang-analyze runs. 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.) Fixed another false null deref report by adding an assert. Not in this PR: SetBGError returning address of a stack variable. Test Plan: Was able to reproduce the reported errors locally and verify they're fixed (except SetBGError). Otherwise, existing tests

facebook-github-bot · 2022-02-07T21:54:02Z

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

hx235 · 2022-02-07T22:54:43Z

util/status.cc

+std::unique_ptr<const char[]> Status::CopyState(const char* s) {
+  const size_t cch = std::strlen(s) + 1;  // +1 for the null terminator
+  char* rv = new char[cch];
+  std::strncpy(rv, s, cch);


For some reason, the existing code uses strncpy_s(result, cch, state, cch - 1); for OS_WIN (with or without _MSC_VER), are we dropping that concern and use std::strncpy(rv, s, cch); for OS_WIN based on the build-windows-x signals?

I looked at #4128, and

I don't think this code is going to trigger the gcc-8 warning

A static analyzer should easily be able to tell that 'new char[]' is returned, without understanding what strncpy returns.

¯\(ツ)/¯

I was more curious about why we previously separated OS_WIN into its own case case but anyway I think build_window_* run should give us enough signals for dropping the separation. And I agree with the new change shouldn't trigger gcc-8 warning mentioned in #4128.

👍

hx235 · 2022-02-07T22:55:07Z

third-party/gtest-1.8.1/fused-src/gtest/gtest.h

@@ -2442,6 +2443,10 @@ GTEST_API_ bool IsTrue(bool condition);

 // Defines scoped_ptr.

+// RocksDB: use unique_ptr to work around some clang-analyze false reports
+template <typename T>
+using scoped_ptr = std::unique_ptr<T>;


Do we also need to comment out the entire class scoped_ptr for clarifity?

It is commented out. For this third party code, I figured commenting out was better than removing (in case of future merge).

oh - I missed those cute /* */. Thanks!

hx235

Left two questions - otherwise LGTM. Thanks for the fix!

facebook-github-bot · 2022-02-08T00:04:28Z

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

facebook-github-bot · 2022-02-08T00:06:45Z

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

hx235 · 2022-02-08T00:40:29Z

db/error_handler.cc

@@ -251,6 +252,8 @@ void ErrorHandler::CancelErrorRecovery() {
 #endif
 }

+STATIC_AVOID_DESTRUCTION(const Status, kOkStatus){Status::OK()};


hx235

LGTM!

Summary: After facebook#9515 added a unique_ptr to Status, we see some warnings-as-error in some internal builds like this: ``` stderr: rocksdb/src/db/compaction/compaction_job.cc:2839:7: error: offset of on non-standard-layout type 'struct CompactionServiceResult' [-Werror,-Winvalid-offsetof] {offsetof(struct CompactionServiceResult, status), ^ ~~~~~~ ``` I see three potential solutions to resolving this: * Be more tricky about computing offsets, to bypass the warning, but this is basically invoking undefined behavior. * Migrate to something in place of offset, like a function mapping CompactionServiceResult* to Status* (for the `status` field). This might be required in the long term. * Use our new C++17 dependency to use offsetof in a well-defined way when the compiler allows it. From a comment on https://gist.github.com/graphitemaster/494f21190bb2c63c5516: A final note: in C++17, offsetof is conditionally supported, which means that you can use it on any type (not just standard layout types) and the compiler will error if it can't compile it correctly. That appears to be the best option if you can live with C++17 and don't need constexpr support. The C++17 semantics are confirmed on https://en.cppreference.com/w/cpp/types/offsetof, so we can suppress the warning as long as we accept that we might run into a compiler that rejects the code, and at that point we will find a solution, such as the more intrusive "migrate" solution above. Although this is currently only showing in our buck build, it will surely show up also with make and cmake, so I have updated those configurations as well. Test Plan: Tried out buck builds with platform009 and platform010 Reviewers: Subscribers: Tasks: Tags:

Summary: After #9515 added a unique_ptr to Status, we see some warnings-as-error in some internal builds like this: ``` stderr: rocksdb/src/db/compaction/compaction_job.cc:2839:7: error: offset of on non-standard-layout type 'struct CompactionServiceResult' [-Werror,-Winvalid-offsetof] {offsetof(struct CompactionServiceResult, status), ^ ~~~~~~ ``` I see three potential solutions to resolving this: * Expand our use of an idiom that works around the warning (see offset_of functions removed in this change, inspired by https://gist.github.com/graphitemaster/494f21190bb2c63c5516) However, this construction is invoking undefined behavior that assumes consistent layout with no compiler-introduced indirection. A compiler incompatible with our assumptions will likely compile the code and exhibit undefined behavior. * Migrate to something in place of offset, like a function mapping CompactionServiceResult* to Status* (for the `status` field). This might be required in the long term. * **Selected:** Use our new C++17 dependency to use offsetof in a well-defined way when the compiler allows it. From a comment on https://gist.github.com/graphitemaster/494f21190bb2c63c5516: > A final note: in C++17, offsetof is conditionally supported, which > means that you can use it on any type (not just standard layout > types) and the compiler will error if it can't compile it correctly. > That appears to be the best option if you can live with C++17 and > don't need constexpr support. The C++17 semantics are confirmed on https://en.cppreference.com/w/cpp/types/offsetof, so we can suppress the warning as long as we accept that we might run into a compiler that rejects the code, and at that point we will find a solution, such as the more intrusive "migrate" solution above. Although this is currently only showing in our buck build, it will surely show up also with make and cmake, so I have updated those configurations as well. Also in the buck build, -Wno-expansion-to-defined does not appear to be needed anymore (both current compiler configurations) so I removed it. Pull Request resolved: #9563 Test Plan: Tried out buck builds with both current compiler configurations Reviewed By: riversand963 Differential Revision: D34220931 Pulled By: pdillinger fbshipit-source-id: d39436008259bd1eaaa87c77be69fb2a5b559e1f

pdillinger requested a review from hx235 February 7, 2022 21:51

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

hx235 reviewed Feb 7, 2022

View reviewed changes

hx235 approved these changes Feb 7, 2022

View reviewed changes

pdillinger added 2 commits February 7, 2022 15:59

Also fix returning address of stack variable

17541e1

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

a8c0bb2

pdillinger requested a review from hx235 February 8, 2022 00:34

hx235 reviewed Feb 8, 2022

View reviewed changes

hx235 approved these changes Feb 8, 2022

View reviewed changes

facebook-github-bot closed this in 5cb137a Feb 8, 2022

pdillinger mentioned this pull request Feb 14, 2022

Use -Wno-invalid-offsetof instead of dangerous offset_of hack #9563

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Work around some new clang-analyze failures #9515

Work around some new clang-analyze failures #9515

pdillinger commented Feb 7, 2022 •

edited

Loading

facebook-github-bot commented Feb 7, 2022

hx235 Feb 7, 2022

pdillinger Feb 7, 2022

hx235 Feb 7, 2022

hx235 Feb 7, 2022

pdillinger Feb 7, 2022

hx235 Feb 7, 2022

hx235 left a comment •

edited

Loading

facebook-github-bot commented Feb 8, 2022

facebook-github-bot commented Feb 8, 2022

hx235 Feb 8, 2022

hx235 left a comment

Work around some new clang-analyze failures #9515

Work around some new clang-analyze failures #9515

Conversation

pdillinger commented Feb 7, 2022 • edited Loading

facebook-github-bot commented Feb 7, 2022

hx235 Feb 7, 2022

Choose a reason for hiding this comment

pdillinger Feb 7, 2022

Choose a reason for hiding this comment

hx235 Feb 7, 2022

Choose a reason for hiding this comment

hx235 Feb 7, 2022

Choose a reason for hiding this comment

pdillinger Feb 7, 2022

Choose a reason for hiding this comment

hx235 Feb 7, 2022

Choose a reason for hiding this comment

hx235 left a comment • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Feb 8, 2022

facebook-github-bot commented Feb 8, 2022

hx235 Feb 8, 2022

Choose a reason for hiding this comment

hx235 left a comment

Choose a reason for hiding this comment

pdillinger commented Feb 7, 2022 •

edited

Loading

hx235 left a comment •

edited

Loading