port/mmap: fix Clang -Wnontrivial-memcall error on macOS#429
Conversation
ti-chi-bot
Bot
added
contribution
|
Hi @liyishuai. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
the
first-time-contributor
|
Welcome @liyishuai! |
ti-chi-bot
Bot
added
the
size/XS
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe move assignment operator for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
liyishuai
force-pushed
the
fix/macos-clang-nontrivial-memcall
branch
from
63d62a4 to
cd2a4b8
Compare
ti-chi-bot
Bot
added
dco-signoff: yes
There was a problem hiding this comment.
Pull request overview
Fixes macOS (Clang 16+) build failures caused by -Werror,-Wnontrivial-memcall by removing the memcpy-based move assignment implementation for MemMapping and replacing it with member-wise moves.
Changes:
- Remove
memcpy+ placement-newmove-assignment pattern fromMemMapping. - Implement move assignment via
std::exchangeon the mapping’s owned members. - Drop now-unused headers (
<cstring>,<new>).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
port/mmap.cc (1)
43-48: Member access after explicit destructor call is technically UB; consider placement-new or inline cleanup.After
this->~MemMapping()at Line 43, the lifetime of*thishas ended. Per [basic.life], usingthisto access non-static data members (Lines 44–48) is undefined behavior, even though it works in practice here because the members are trivial scalar types. The original memcpy-based code avoided this by re-starting object lifetime via placement-new.Two clean options:
♻️ Option A — restart lifetime via placement-new (most idiomatic, re-adds ``)
MemMapping& MemMapping::operator=(MemMapping&& other) noexcept { if (&other == this) { return *this; } this->~MemMapping(); - addr_ = std::exchange(other.addr_, nullptr); - length_ = std::exchange(other.length_, 0); -#ifdef OS_WIN - page_file_handle_ = std::exchange(other.page_file_handle_, NULL); -#endif + ::new (this) MemMapping(std::move(other)); return *this; }♻️ Option B — release resources without calling the destructor, then move members (keeps `` removed)
MemMapping& MemMapping::operator=(MemMapping&& other) noexcept { if (&other == this) { return *this; } - this->~MemMapping(); + // Release currently owned resources without ending *this's lifetime. +#ifdef OS_WIN + if (addr_ != nullptr) { + (void)::UnmapViewOfFile(addr_); + } + if (page_file_handle_ != NULL) { + (void)::CloseHandle(page_file_handle_); + } +#else + if (addr_ != nullptr) { + auto status = munmap(addr_, length_); + assert(status == 0); + (void)status; + } +#endif addr_ = std::exchange(other.addr_, nullptr); length_ = std::exchange(other.length_, 0); `#ifdef` OS_WIN page_file_handle_ = std::exchange(other.page_file_handle_, NULL); `#endif` return *this; }Either option avoids the post-destructor member access. Option A is the smaller diff and most closely matches the original intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@port/mmap.cc` around lines 43 - 48, Calling this->~MemMapping() ends the object's lifetime and then accessing members (addr_, length_, page_file_handle_) is UB; restore lifetime before touching members by either (A) performing a placement-new of MemMapping at this to reinitialize the object and then assign std::exchange(other.addr_, nullptr)/std::exchange(other.length_, 0)/std::exchange(other.page_file_handle_, NULL), or (B) avoid calling the destructor entirely and instead call the destructor logic that releases resources (e.g., a private cleanup method) without ending the object's lifetime, then move the members from other into addr_, length_, and page_file_handle_; update MemMapping's move-or-swap implementation accordingly so no member is accessed after lifetime ends.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@port/mmap.cc`:
- Around line 43-48: Calling this->~MemMapping() ends the object's lifetime and
then accessing members (addr_, length_, page_file_handle_) is UB; restore
lifetime before touching members by either (A) performing a placement-new of
MemMapping at this to reinitialize the object and then assign
std::exchange(other.addr_, nullptr)/std::exchange(other.length_,
0)/std::exchange(other.page_file_handle_, NULL), or (B) avoid calling the
destructor entirely and instead call the destructor logic that releases
resources (e.g., a private cleanup method) without ending the object's lifetime,
then move the members from other into addr_, length_, and page_file_handle_;
update MemMapping's move-or-swap implementation accordingly so no member is
accessed after lifetime ends.
Summary:
fix the following error showing up in continuous tests:
```
Makefile:186: Warning: Compiling in debug mode. Don't use the resulting binary in production
port/mmap.cc:46:15: error: first argument in call to 'memcpy' is a pointer to non-trivially copyable type 'rocksdb::MemMapping' [-Werror,-Wnontrivial-memcall]
46 | std::memcpy(this, &other, sizeof(*this));
| ^
port/mmap.cc:46:15: note: explicitly cast the pointer to silence this warning
46 | std::memcpy(this, &other, sizeof(*this));
| ^
| (void*)
1 error generated.
make: *** [Makefile:2580: port/mmap.o] Error 1
make: *** Waiting for unfinished jobs....
```
Pull Request resolved: facebook#13864
Test Plan: `make USE_CLANG=1 j=150 check` with https://github.com/facebook/rocksdb/blob/13f054febb26100184eeefaac11877d735d45ac2/build_tools/build_detect_platform#L61-L70 commented out.
Reviewed By: mszeszko-meta
Differential Revision: D80033441
Pulled By: cbi42
fbshipit-source-id: b2330eea71fe28243236b75128ec6f3f1e971873
Signed-off-by: Yishuai Li <yishuai.li@pingcap.com>
liyishuai
force-pushed
the
fix/macos-clang-nontrivial-memcall
branch
from
cd2a4b8 to
f67c665
Compare
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Connor1996, v01dstar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
@liyishuai: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
ti-chi-bot
Bot
added
ok-to-test
Problem
Clang 16+ enables
-Wnontrivial-memcall, which rejectsmemcpyon pointers to non-trivially copyable types.MemMappinghas a user-provided destructor, making it non-trivially copyable. The move assignment operator usedmemcpy(this, &other, sizeof(*this)), which triggers-Werror,-Wnontrivial-memcallon macOS and causes a build failure:Fix
Replace the
memcpy+ placement-newpattern with explicitstd::exchangemember assignments — the correct C++ idiom for move semantics on a type with managed resources. Also removes the now-unused<cstring>and<new>includes.Test
Build passes on macOS with Apple Clang (tested via tikv/titan).
🤖 Generated with Claude Code
Summary by CodeRabbit