Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 23 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughBumps vendored jemalloc to 5.3.1, switches submodule URL to the official jemalloc repo, expands configure-time options and checks, adds two new sized-free FFI symbols, updates unsafe raw value handling, and refactors macro-generated tests for control interface options and background-thread tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jemalloc-sys/configure/configure (1)
6951-7019:⚠️ Potential issue | 🟠 MajorAdd linkability validation to explicit
--with-cxx-stdlibselections.The explicit
libstdc++andlibcxxbranches append toLIBSwithout validating linkability, while the default branch performs a linkability test and rolls back on failure. This asymmetry allows an explicitly selected C++ stdlib to pass configure even if unavailable, deferring the failure to the link step. The same validation logic from the default branch should be applied to explicit selections.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jemalloc-sys/configure/configure` around lines 6951 - 7019, The libstdc++ and libcxx case arms append T_APPEND_V to LIBS without performing the linkability check that the default branch does; update the libstdc++ and libcxx branches in the with_cxx_stdlib case to mirror the default branch: set T_APPEND_V (e.g. -lstdc++ or -lc++), append it to LIBS temporarily (using the same SAVED_LIBS pattern), run the existing conftest/linkability routine (invoke ac_fn_c_try_link to set je_cv_libstdcxx), and if je_cv_libstdcxx is "no" restore SAVED_LIBS (SAVED_LIBS, LIBS, T_APPEND_V, je_cv_libstdcxx and ac_fn_c_try_link are the symbols to touch) so explicit selections fail at configure time like the default branch.
🧹 Nitpick comments (1)
jemalloc-ctl/src/lib.rs (1)
244-247: Also cover thebackground_threadupdate path here.
jemalloc-ctl/src/macros.rsnow skips the autogeneratedbackground_threadupdate test, so this module is the only remaining coverage for that API. Right now it never calls eitherbackground_thread::update()orbg_mib.update().Possible addition
background_thread::write(!bg_orig).unwrap(); assert_eq!(background_thread::read().unwrap(), !bg_orig); + let old = bg_mib.update(bg_orig).unwrap(); + assert_eq!(old, !bg_orig); + let old = background_thread::update(!bg_orig).unwrap(); + assert_eq!(old, bg_orig); bg_mib.write(bg_orig).unwrap(); assert_eq!(bg_mib.read().unwrap(), bg_orig);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jemalloc-ctl/src/lib.rs` around lines 244 - 247, The test currently toggles background_thread and bg_mib using write/read but never exercises their update paths; add calls to background_thread::update() and bg_mib.update() after the write/read assertions to verify the update API toggles the value and that subsequent read() returns the updated state. Specifically, after background_thread::write(!bg_orig) and assert_eq!(background_thread::read().unwrap(), !bg_orig), call background_thread::update().unwrap() and assert_eq!(background_thread::read().unwrap(), bg_orig); similarly after bg_mib.write(bg_orig) and assert_eq!(bg_mib.read().unwrap(), bg_orig), call bg_mib.update().unwrap() and assert_eq!(bg_mib.read().unwrap(), !bg_orig), ensuring you reference the background_thread::write/read/update and bg_mib.write/read/update symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jemalloc-ctl/src/lib.rs`:
- Around line 239-261: The test read_write_update currently mutates
process-global allocator state and doesn't always restore originals; snapshot
the original values from background_thread::mib().read() and
max_background_threads::mib().read() at the start of the test and ensure both
are restored on all exit paths (use a scoped cleanup/guard or defer) by calling
background_thread::write(original_bg) / bg_mib.write(original_bg) and
max_background_threads::write(original_mt) / mt_mib.write(original_mt) (and/or
their .update counterparts) so failures cannot leak modified state to other
tests.
In `@jemalloc-sys/configure/configure`:
- Around line 11718-11737: The platform check currently uses `uname -s` at the
top of the block (the line starting with if test `uname -s` = "Linux") which
inspects the build machine instead of the target; change that conditional to
check the cross-build target variable (use `${host_os}` or `${host}` as used
elsewhere) — e.g. replace the `uname -s` test with a test against `host_os`
(e.g. test "x${host_os}" = "xLinux") so the logic that sets
`enable_prof_frameptr` and the subsequent checks that reference
`enable_prof_frameptr`, `enable_prof`, `backtrace_method`, and `GCC` follow
target-platform semantics. Ensure the rest of the block remains unchanged.
- Around line 14828-14995: The linker probe blocks for sanitizers (the
-fsanitize=thread and UBSAN -fsanitize=undefined sections) currently roll back
CONFIGURE_LDFLAGS on link failure but do not restore CONFIGURE_CFLAGS and
CONFIGURE_CXXFLAGS, leaving instrumented objects without matching linker flags;
update the failure branches (where je_cv_ldflags_added is cleared) to also
restore CONFIGURE_CFLAGS to T_CONFIGURE_CFLAGS and CONFIGURE_CXXFLAGS to
T_CONFIGURE_CXXFLAGS (mirror how CONFIGURE_LDFLAGS is reset), and ensure the
same fix is applied in both the ThreadSanitizer and UBSan linker-probe blocks
(refer to symbols je_cv_cflags_added, je_cv_cxxflags_added, je_cv_ldflags_added,
T_CONFIGURE_CFLAGS, T_CONFIGURE_CXXFLAGS, and T_CONFIGURE_LDFLAGS).
---
Outside diff comments:
In `@jemalloc-sys/configure/configure`:
- Around line 6951-7019: The libstdc++ and libcxx case arms append T_APPEND_V to
LIBS without performing the linkability check that the default branch does;
update the libstdc++ and libcxx branches in the with_cxx_stdlib case to mirror
the default branch: set T_APPEND_V (e.g. -lstdc++ or -lc++), append it to LIBS
temporarily (using the same SAVED_LIBS pattern), run the existing
conftest/linkability routine (invoke ac_fn_c_try_link to set je_cv_libstdcxx),
and if je_cv_libstdcxx is "no" restore SAVED_LIBS (SAVED_LIBS, LIBS, T_APPEND_V,
je_cv_libstdcxx and ac_fn_c_try_link are the symbols to touch) so explicit
selections fail at configure time like the default branch.
---
Nitpick comments:
In `@jemalloc-ctl/src/lib.rs`:
- Around line 244-247: The test currently toggles background_thread and bg_mib
using write/read but never exercises their update paths; add calls to
background_thread::update() and bg_mib.update() after the write/read assertions
to verify the update API toggles the value and that subsequent read() returns
the updated state. Specifically, after background_thread::write(!bg_orig) and
assert_eq!(background_thread::read().unwrap(), !bg_orig), call
background_thread::update().unwrap() and
assert_eq!(background_thread::read().unwrap(), bg_orig); similarly after
bg_mib.write(bg_orig) and assert_eq!(bg_mib.read().unwrap(), bg_orig), call
bg_mib.update().unwrap() and assert_eq!(bg_mib.read().unwrap(), !bg_orig),
ensuring you reference the background_thread::write/read/update and
bg_mib.write/read/update symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da4a971c-55d0-4ecd-aad8-cc00ca8ab672
📒 Files selected for processing (7)
.gitmodulesjemalloc-ctl/src/lib.rsjemalloc-ctl/src/macros.rsjemalloc-sys/Cargo.tomljemalloc-sys/README.mdjemalloc-sys/configure/configurejemalloc-sys/jemalloc
| fn read_write_update() { | ||
| // -- background_thread -- | ||
| let bg_mib = background_thread::mib().unwrap(); | ||
| let bg_orig = bg_mib.read().unwrap(); | ||
|
|
||
| background_thread::write(!bg_orig).unwrap(); | ||
| assert_eq!(background_thread::read().unwrap(), !bg_orig); | ||
| bg_mib.write(bg_orig).unwrap(); | ||
| assert_eq!(bg_mib.read().unwrap(), bg_orig); | ||
|
|
||
| // -- max_background_threads -- | ||
| // jemalloc 5.3.1+ rejects writing 0 (EINVAL), so use 1. | ||
| let mt_mib = max_background_threads::mib().unwrap(); | ||
| let _ = max_background_threads::read().unwrap(); | ||
| let _ = mt_mib.read().unwrap(); | ||
| max_background_threads::write(1).unwrap(); | ||
| assert_eq!(max_background_threads::read().unwrap(), 1); | ||
| mt_mib.write(1).unwrap(); | ||
| assert_eq!(mt_mib.read().unwrap(), 1); | ||
| let old = max_background_threads::update(1).unwrap(); | ||
| assert_eq!(old, 1); | ||
| let old = mt_mib.update(1).unwrap(); | ||
| assert_eq!(old, 1); |
There was a problem hiding this comment.
Restore the original control values before this test exits.
background_thread_tests::read_write_update mutates process-global allocator state. max_background_threads is left at 1, and background_thread is only restored on the happy path, so a failure here can contaminate later tests in the same binary. Please snapshot both originals and restore them in scoped cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jemalloc-ctl/src/lib.rs` around lines 239 - 261, The test read_write_update
currently mutates process-global allocator state and doesn't always restore
originals; snapshot the original values from background_thread::mib().read() and
max_background_threads::mib().read() at the start of the test and ensure both
are restored on all exit paths (use a scoped cleanup/guard or defer) by calling
background_thread::write(original_bg) / bg_mib.write(original_bg) and
max_background_threads::write(original_mt) / mt_mib.write(original_mt) (and/or
their .update counterparts) so failures cannot leak modified state to other
tests.
| if test `uname -s` = "Linux" | ||
| then | ||
| # Check whether --enable-prof-frameptr was given. | ||
| if test "${enable_prof_frameptr+set}" = set; then : | ||
| enableval=$enable_prof_frameptr; if test "x$enable_prof_frameptr" = "xno" ; then | ||
| enable_prof_frameptr="0" | ||
| else | ||
| enable_prof_frameptr="1" | ||
| if test "x$enable_prof" = "x0" ; then | ||
| as_fn_error $? "--enable-prof-frameptr should only be used with --enable-prof" "$LINENO" 5 | ||
| fi | ||
| fi | ||
|
|
||
| else | ||
| enable_prof_frameptr="0" | ||
|
|
||
| fi | ||
|
|
||
| if test "x$backtrace_method" = "x" -a "x$enable_prof_frameptr" = "x1" \ | ||
| -a "x$GCC" = "xyes" ; then |
There was a problem hiding this comment.
Use host_os here instead of uname -s.
Line 11718 keys this block off the build machine, not the target. Cross builds can therefore disable --enable-prof-frameptr for a Linux target, or enable Linux-specific profiling config for a non-Linux target. This needs to follow ${host_os} / ${host} like the rest of the platform logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jemalloc-sys/configure/configure` around lines 11718 - 11737, The platform
check currently uses `uname -s` at the top of the block (the line starting with
if test `uname -s` = "Linux") which inspects the build machine instead of the
target; change that conditional to check the cross-build target variable (use
`${host_os}` or `${host}` as used elsewhere) — e.g. replace the `uname -s` test
with a test against `host_os` (e.g. test "x${host_os}" = "xLinux") so the logic
that sets `enable_prof_frameptr` and the subsequent checks that reference
`enable_prof_frameptr`, `enable_prof`, `backtrace_method`, and `GCC` follow
target-platform semantics. Ensure the rest of the block remains unchanged.
| if test "x$enable_tsan" = "x1" ; then | ||
|
|
||
| { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether compiler supports -fsanitize=thread" >&5 | ||
| $as_echo_n "checking whether compiler supports -fsanitize=thread... " >&6; } | ||
| T_CONFIGURE_CFLAGS="${CONFIGURE_CFLAGS}" | ||
| T_APPEND_V=-fsanitize=thread | ||
| if test "x${CONFIGURE_CFLAGS}" = "x" -o "x${T_APPEND_V}" = "x" ; then | ||
| CONFIGURE_CFLAGS="${CONFIGURE_CFLAGS}${T_APPEND_V}" | ||
| else | ||
| CONFIGURE_CFLAGS="${CONFIGURE_CFLAGS} ${T_APPEND_V}" | ||
| fi | ||
|
|
||
|
|
||
| if test "x${CONFIGURE_CFLAGS}" = "x" -o "x${SPECIFIED_CFLAGS}" = "x" ; then | ||
| CFLAGS="${CONFIGURE_CFLAGS}${SPECIFIED_CFLAGS}" | ||
| else | ||
| CFLAGS="${CONFIGURE_CFLAGS} ${SPECIFIED_CFLAGS}" | ||
| fi | ||
|
|
||
| cat confdefs.h - <<_ACEOF >conftest.$ac_ext | ||
| /* end confdefs.h. */ | ||
|
|
||
|
|
||
| int | ||
| main () | ||
| { | ||
|
|
||
| return 0; | ||
|
|
||
| ; | ||
| return 0; | ||
| } | ||
| _ACEOF | ||
| if ac_fn_c_try_compile "$LINENO"; then : | ||
| je_cv_cflags_added=-fsanitize=thread | ||
| { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5 | ||
| $as_echo "yes" >&6; } | ||
| else | ||
| je_cv_cflags_added= | ||
| { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 | ||
| $as_echo "no" >&6; } | ||
| CONFIGURE_CFLAGS="${T_CONFIGURE_CFLAGS}" | ||
|
|
||
| fi | ||
| rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext | ||
| if test "x${CONFIGURE_CFLAGS}" = "x" -o "x${SPECIFIED_CFLAGS}" = "x" ; then | ||
| CFLAGS="${CONFIGURE_CFLAGS}${SPECIFIED_CFLAGS}" | ||
| else | ||
| CFLAGS="${CONFIGURE_CFLAGS} ${SPECIFIED_CFLAGS}" | ||
| fi | ||
|
|
||
|
|
||
|
|
||
| { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether compiler supports -fsanitize=thread" >&5 | ||
| $as_echo_n "checking whether compiler supports -fsanitize=thread... " >&6; } | ||
| T_CONFIGURE_CXXFLAGS="${CONFIGURE_CXXFLAGS}" | ||
| T_APPEND_V=-fsanitize=thread | ||
| if test "x${CONFIGURE_CXXFLAGS}" = "x" -o "x${T_APPEND_V}" = "x" ; then | ||
| CONFIGURE_CXXFLAGS="${CONFIGURE_CXXFLAGS}${T_APPEND_V}" | ||
| else | ||
| CONFIGURE_CXXFLAGS="${CONFIGURE_CXXFLAGS} ${T_APPEND_V}" | ||
| fi | ||
|
|
||
|
|
||
| if test "x${CONFIGURE_CXXFLAGS}" = "x" -o "x${SPECIFIED_CXXFLAGS}" = "x" ; then | ||
| CXXFLAGS="${CONFIGURE_CXXFLAGS}${SPECIFIED_CXXFLAGS}" | ||
| else | ||
| CXXFLAGS="${CONFIGURE_CXXFLAGS} ${SPECIFIED_CXXFLAGS}" | ||
| fi | ||
|
|
||
| ac_ext=cpp | ||
| ac_cpp='$CXXCPP $CPPFLAGS' | ||
| ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5' | ||
| ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' | ||
| ac_compiler_gnu=$ac_cv_cxx_compiler_gnu | ||
|
|
||
| cat confdefs.h - <<_ACEOF >conftest.$ac_ext | ||
| /* end confdefs.h. */ | ||
|
|
||
| #include <sys/mman.h> | ||
|
|
||
| int | ||
| main () | ||
| { | ||
|
|
||
| mprotect((void *)0, 0, PROT_NONE); | ||
| return 0; | ||
|
|
||
| ; | ||
| return 0; | ||
| } | ||
| _ACEOF | ||
| if ac_fn_c_try_link "$LINENO"; then : | ||
| je_cv_mprotect=yes | ||
| if ac_fn_cxx_try_compile "$LINENO"; then : | ||
| je_cv_cxxflags_added=-fsanitize=thread | ||
| { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5 | ||
| $as_echo "yes" >&6; } | ||
| else | ||
| je_cv_mprotect=no | ||
| je_cv_cxxflags_added= | ||
| { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 | ||
| $as_echo "no" >&6; } | ||
| CONFIGURE_CXXFLAGS="${T_CONFIGURE_CXXFLAGS}" | ||
|
|
||
| fi | ||
| rm -f core conftest.err conftest.$ac_objext \ | ||
| conftest$ac_exeext conftest.$ac_ext | ||
| rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext | ||
| ac_ext=c | ||
| ac_cpp='$CPP $CPPFLAGS' | ||
| ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5' | ||
| ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' | ||
| ac_compiler_gnu=$ac_cv_c_compiler_gnu | ||
|
|
||
| if test "x${CONFIGURE_CXXFLAGS}" = "x" -o "x${SPECIFIED_CXXFLAGS}" = "x" ; then | ||
| CXXFLAGS="${CONFIGURE_CXXFLAGS}${SPECIFIED_CXXFLAGS}" | ||
| else | ||
| CXXFLAGS="${CONFIGURE_CXXFLAGS} ${SPECIFIED_CXXFLAGS}" | ||
| fi | ||
| { $as_echo "$as_me:${as_lineno-$LINENO}: result: $je_cv_mprotect" >&5 | ||
| $as_echo "$je_cv_mprotect" >&6; } | ||
|
|
||
| if test "x${je_cv_mprotect}" = "xyes" ; then | ||
|
|
||
| $as_echo "#define JEMALLOC_HAVE_MPROTECT " >>confdefs.h | ||
|
|
||
| { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether linker supports -fsanitize=thread" >&5 | ||
| $as_echo_n "checking whether linker supports -fsanitize=thread... " >&6; } | ||
| T_CONFIGURE_LDFLAGS="${CONFIGURE_LDFLAGS}" | ||
| T_APPEND_V=-fsanitize=thread | ||
| if test "x${CONFIGURE_LDFLAGS}" = "x" -o "x${T_APPEND_V}" = "x" ; then | ||
| CONFIGURE_LDFLAGS="${CONFIGURE_LDFLAGS}${T_APPEND_V}" | ||
| else | ||
| CONFIGURE_LDFLAGS="${CONFIGURE_LDFLAGS} ${T_APPEND_V}" | ||
| fi | ||
|
|
||
|
|
||
| { $as_echo "$as_me:${as_lineno-$LINENO}: checking for __builtin_clz" >&5 | ||
| $as_echo_n "checking for __builtin_clz... " >&6; } | ||
| if ${je_cv_builtin_clz+:} false; then : | ||
| $as_echo_n "(cached) " >&6 | ||
| if test "x${CONFIGURE_LDFLAGS}" = "x" -o "x${SPECIFIED_LDFLAGS}" = "x" ; then | ||
| LDFLAGS="${CONFIGURE_LDFLAGS}${SPECIFIED_LDFLAGS}" | ||
| else | ||
| cat confdefs.h - <<_ACEOF >conftest.$ac_ext | ||
| LDFLAGS="${CONFIGURE_LDFLAGS} ${SPECIFIED_LDFLAGS}" | ||
| fi | ||
|
|
||
| cat confdefs.h - <<_ACEOF >conftest.$ac_ext | ||
| /* end confdefs.h. */ | ||
|
|
||
|
|
||
| int | ||
| main () | ||
| { | ||
|
|
||
| { | ||
| unsigned x = 0; | ||
| int y = __builtin_clz(x); | ||
| } | ||
| { | ||
| unsigned long x = 0; | ||
| int y = __builtin_clzl(x); | ||
| } | ||
| { | ||
| unsigned long long x = 0; | ||
| int y = __builtin_clzll(x); | ||
| } | ||
| return 0; | ||
|
|
||
| ; | ||
| return 0; | ||
| } | ||
| _ACEOF | ||
| if ac_fn_c_try_link "$LINENO"; then : | ||
| je_cv_builtin_clz=yes | ||
| je_cv_ldflags_added=-fsanitize=thread | ||
| { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5 | ||
| $as_echo "yes" >&6; } | ||
| else | ||
| je_cv_builtin_clz=no | ||
| je_cv_ldflags_added= | ||
| { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 | ||
| $as_echo "no" >&6; } | ||
| CONFIGURE_LDFLAGS="${T_CONFIGURE_LDFLAGS}" | ||
|
|
||
| fi | ||
| rm -f core conftest.err conftest.$ac_objext \ | ||
| conftest$ac_exeext conftest.$ac_ext | ||
| if test "x${CONFIGURE_LDFLAGS}" = "x" -o "x${SPECIFIED_LDFLAGS}" = "x" ; then | ||
| LDFLAGS="${CONFIGURE_LDFLAGS}${SPECIFIED_LDFLAGS}" | ||
| else | ||
| LDFLAGS="${CONFIGURE_LDFLAGS} ${SPECIFIED_LDFLAGS}" | ||
| fi | ||
| { $as_echo "$as_me:${as_lineno-$LINENO}: result: $je_cv_builtin_clz" >&5 | ||
| $as_echo "$je_cv_builtin_clz" >&6; } | ||
|
|
||
| if test "x${je_cv_builtin_clz}" = "xyes" ; then | ||
|
|
||
| $as_echo "#define JEMALLOC_HAVE_BUILTIN_CLZ " >>confdefs.h | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the configure file
fd -t f "configure" jemalloc-sys/ | head -5Repository: tikv/jemallocator
Length of output: 93
🏁 Script executed:
# Get the file size to determine how to read it
wc -l jemalloc-sys/configure/configureRepository: tikv/jemallocator
Length of output: 99
🏁 Script executed:
# Extract the TSAN section (lines 14828-14995)
sed -n '14828,14995p' jemalloc-sys/configure/configure | cat -nRepository: tikv/jemallocator
Length of output: 5877
🏁 Script executed:
# Extract the section that "also applies to" (lines 14997-15177)
sed -n '14997,15177p' jemalloc-sys/configure/configure | cat -nRepository: tikv/jemallocator
Length of output: 6234
Sanitizer compile flags must be rolled back when the linker probe fails.
When the linker probe for -fsanitize=thread or -fsanitize=undefined fails, CONFIGURE_CFLAGS and CONFIGURE_CXXFLAGS are not reset. This leaves instrumented object files without matching runtime support at link time, causing late build failures instead of clean configure-time rejection. The code correctly rolls back CONFIGURE_LDFLAGS on linker failure (lines 156, 169), but must do the same for CONFIGURE_CFLAGS and CONFIGURE_CXXFLAGS.
Also applies to: 14997-15177 (UBSAN section)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jemalloc-sys/configure/configure` around lines 14828 - 14995, The linker
probe blocks for sanitizers (the -fsanitize=thread and UBSAN
-fsanitize=undefined sections) currently roll back CONFIGURE_LDFLAGS on link
failure but do not restore CONFIGURE_CFLAGS and CONFIGURE_CXXFLAGS, leaving
instrumented objects without matching linker flags; update the failure branches
(where je_cv_ldflags_added is cleared) to also restore CONFIGURE_CFLAGS to
T_CONFIGURE_CFLAGS and CONFIGURE_CXXFLAGS to T_CONFIGURE_CXXFLAGS (mirror how
CONFIGURE_LDFLAGS is reset), and ensure the same fix is applied in both the
ThreadSanitizer and UBSan linker-probe blocks (refer to symbols
je_cv_cflags_added, je_cv_cxxflags_added, je_cv_ldflags_added,
T_CONFIGURE_CFLAGS, T_CONFIGURE_CXXFLAGS, and T_CONFIGURE_LDFLAGS).
|
How about merge nightly back to main and then switch to 5.3.1 instead? |
Yes, I can rebase this commit onto the nightly head. I didn't realize there's a nightly branch which fixes some bugs and adapts new features early. (Actually I also encounter the broken update bug when making this PR.) |
oldp should not be the same as newp, otherwise jemalloc may write to oldp first and the new value will be lost. Also fix invalid test value for max_background_threads and epoch. Signed-off-by: Jay Lee <busyjaylee@gmail.com>
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
Signed-off-by: Jay Lee <busyjaylee@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
jemalloc-sys/src/lib.rs (1)
256-289: Keep allocator-override symbol pinning in sync with newly exported deallocators.With
free_sized/free_aligned_sizedadded here,set_up_statics(Line 974-987) should also pin these symbols underoverride_allocator_on_supported_platformsto preserve the file’s “force all allocator symbols visible to linker” guarantee.♻️ Proposed update in
set_up_staticsmod set_up_statics { use super::*; @@ #[used] static USED_FREE: unsafe extern "C" fn(*mut c_void) = free; + #[used] + static USED_FREE_SIZED: unsafe extern "C" fn(*mut c_void, usize) = free_sized; + #[used] + static USED_FREE_ALIGNED_SIZED: unsafe extern "C" fn(*mut c_void, usize, usize) = + free_aligned_sized; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jemalloc-sys/src/lib.rs` around lines 256 - 289, set_up_statics must also pin the newly exported deallocators so the allocator-override symbol set remains complete: add entries for free_sized and free_aligned_sized to the list of symbols registered under override_allocator_on_supported_platforms inside set_up_statics (the same place other allocator symbols are pinned) so the file’s “force all allocator symbols visible to linker” guarantee is preserved; reference the functions named free_sized and free_aligned_sized and the configuration block/function set_up_statics/override_allocator_on_supported_platforms when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jemalloc-ctl/src/raw.rs`:
- Around line 188-201: The update function should enforce the same T: Copy bound
as update_mib to prevent constructing non-Copy types from raw bytes; modify the
signature of pub unsafe fn update<T>(...) to pub unsafe fn update<T: Copy>(...)
and ensure any uses of T in the function (old_value, value, sizes) remain the
same so the unsafe mallctl call and the assert_eq!(old_len, mem::size_of::<T>())
still compile; this makes update consistent with update_mib and the function’s
safety contract.
In `@jemallocator/tests/ffi.rs`:
- Around line 26-29: The aligned allocation call violates its precondition and
lacks a null check; change the size passed to ffi::aligned_alloc from 127 to a
multiple of 16 (e.g., 128) and update the matching ffi::free_aligned_sized call
accordingly, and add null-pointer assertions for both allocations (after
ffi::malloc and after ffi::aligned_alloc) so the test checks ptr.is_null() (or
uses the file's existing assert pattern) before freeing; reference: ffi::malloc,
ffi::free_sized, ffi::aligned_alloc, ffi::free_aligned_sized.
---
Nitpick comments:
In `@jemalloc-sys/src/lib.rs`:
- Around line 256-289: set_up_statics must also pin the newly exported
deallocators so the allocator-override symbol set remains complete: add entries
for free_sized and free_aligned_sized to the list of symbols registered under
override_allocator_on_supported_platforms inside set_up_statics (the same place
other allocator symbols are pinned) so the file’s “force all allocator symbols
visible to linker” guarantee is preserved; reference the functions named
free_sized and free_aligned_sized and the configuration block/function
set_up_statics/override_allocator_on_supported_platforms when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 241e1c0c-cf5d-4b41-b774-61f4d24b3fe3
📒 Files selected for processing (11)
.github/workflows/main.yml.gitmodulesjemalloc-ctl/src/lib.rsjemalloc-ctl/src/macros.rsjemalloc-ctl/src/raw.rsjemalloc-sys/Cargo.tomljemalloc-sys/README.mdjemalloc-sys/configure/configurejemalloc-sys/jemallocjemalloc-sys/src/lib.rsjemallocator/tests/ffi.rs
✅ Files skipped from review due to trivial changes (6)
- jemalloc-sys/README.md
- .github/workflows/main.yml
- jemalloc-ctl/src/lib.rs
- jemalloc-sys/jemalloc
- .gitmodules
- jemalloc-sys/Cargo.toml
| pub unsafe fn update<T>(name: &[u8], mut value: T) -> Result<T> { | ||
| validate_name(name); | ||
|
|
||
| let mut len = mem::size_of::<T>(); | ||
| let mut old_len = mem::size_of::<T>(); | ||
| let mut old_value = MaybeUninit::<T>::uninit(); | ||
| cvt(tikv_jemalloc_sys::mallctl( | ||
| name as *const _ as *const c_char, | ||
| old_value.as_mut_ptr() as *mut _, | ||
| &mut old_len, | ||
| &mut value as *mut _ as *mut _, | ||
| &mut len, | ||
| &mut value as *mut _ as *mut _, | ||
| len, | ||
| mem::size_of::<T>(), | ||
| ))?; | ||
| assert_eq!(len, mem::size_of::<T>()); | ||
| Ok(value) | ||
| assert_eq!(old_len, mem::size_of::<T>()); | ||
| Ok(old_value.assume_init()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the two update signatures side-by-side.
sed -n '160,205p' jemalloc-ctl/src/raw.rs
# Scan for obvious explicit instantiations that would rely on a non-Copy T.
rg -nP --type=rust '\bupdate::<\s*(String|Vec<|Box<|PathBuf|CString)' .Repository: tikv/jemallocator
Length of output: 1770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all calls to the update() function
rg -nP --type=rust '\bupdate\(' jemalloc-ctl/src/ | head -20
# Also check for update::<T> generic instantiations with any type
rg -nP --type=rust 'update::<' . | head -20Repository: tikv/jemallocator
Length of output: 1742
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at the full keys.rs file to understand type parameters
cat -n jemalloc-ctl/src/keys.rsRepository: tikv/jemallocator
Length of output: 16436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the complete raw.rs file
wc -l jemalloc-ctl/src/raw.rs
# Then read it in sections
head -400 jemalloc-ctl/src/raw.rs | tail -250Repository: tikv/jemallocator
Length of output: 10115
Add the T: Copy bound to the string-path update function to match update_mib.
The update function on line 188 currently lacks the T: Copy bound that update_mib (line 164) enforces. Both functions perform the same unsafe operation via mallctl/mallctlbymib and share identical safety documentation regarding construction of arbitrary types from raw bytes. The two entry points should enforce the same bounds.
All current call sites pass Copy types (u32, u64, isize, usize, u8, and pointers), so adding this bound is not breaking.
Suggested fix
-pub unsafe fn update<T>(name: &[u8], mut value: T) -> Result<T> {
+pub unsafe fn update<T: Copy>(name: &[u8], mut value: T) -> Result<T> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub unsafe fn update<T>(name: &[u8], mut value: T) -> Result<T> { | |
| validate_name(name); | |
| let mut len = mem::size_of::<T>(); | |
| let mut old_len = mem::size_of::<T>(); | |
| let mut old_value = MaybeUninit::<T>::uninit(); | |
| cvt(tikv_jemalloc_sys::mallctl( | |
| name as *const _ as *const c_char, | |
| old_value.as_mut_ptr() as *mut _, | |
| &mut old_len, | |
| &mut value as *mut _ as *mut _, | |
| &mut len, | |
| &mut value as *mut _ as *mut _, | |
| len, | |
| mem::size_of::<T>(), | |
| ))?; | |
| assert_eq!(len, mem::size_of::<T>()); | |
| Ok(value) | |
| assert_eq!(old_len, mem::size_of::<T>()); | |
| Ok(old_value.assume_init()) | |
| pub unsafe fn update<T: Copy>(name: &[u8], mut value: T) -> Result<T> { | |
| validate_name(name); | |
| let mut old_len = mem::size_of::<T>(); | |
| let mut old_value = MaybeUninit::<T>::uninit(); | |
| cvt(tikv_jemalloc_sys::mallctl( | |
| name as *const _ as *const c_char, | |
| old_value.as_mut_ptr() as *mut _, | |
| &mut old_len, | |
| &mut value as *mut _ as *mut _, | |
| mem::size_of::<T>(), | |
| ))?; | |
| assert_eq!(old_len, mem::size_of::<T>()); | |
| Ok(old_value.assume_init()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jemalloc-ctl/src/raw.rs` around lines 188 - 201, The update function should
enforce the same T: Copy bound as update_mib to prevent constructing non-Copy
types from raw bytes; modify the signature of pub unsafe fn update<T>(...) to
pub unsafe fn update<T: Copy>(...) and ensure any uses of T in the function
(old_value, value, sizes) remain the same so the unsafe mallctl call and the
assert_eq!(old_len, mem::size_of::<T>()) still compile; this makes update
consistent with update_mib and the function’s safety contract.
| ptr = ffi::malloc(100); | ||
| ffi::free_sized(ptr, 100); | ||
| ptr = ffi::aligned_alloc(16, 127); | ||
| ffi::free_aligned_sized(ptr, 16, 127); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the contract in FFI docs and the violating test callsite.
rg -n "pub fn aligned_alloc|integral multiple of `alignment`|size is not an integral multiple" jemalloc-sys/src/lib.rs
rg -n "aligned_alloc\\(" jemallocator/tests/ffi.rsRepository: tikv/jemallocator
Length of output: 313
🏁 Script executed:
# Get broader context around the unsafe documentation and function signature
sed -n '195,205p' jemalloc-sys/src/lib.rs
# Check test patterns - are non-null assertions used elsewhere in this test file?
rg -B2 -A2 "assert.*is_null|is_null.*assert" jemallocator/tests/ffi.rs | head -30Repository: tikv/jemallocator
Length of output: 754
🏁 Script executed:
# View lines around the change to verify exact formatting and context
sed -n '20,35p' jemallocator/tests/ffi.rsRepository: tikv/jemallocator
Length of output: 515
Fix undefined behavior in aligned allocation test.
Line 28 calls ffi::aligned_alloc(16, 127), but the documented precondition requires size to be an integral multiple of alignment. Since 127 ÷ 16 = 7.9375, this violates the safety contract. Additionally, both new allocations lack null assertions unlike other allocation tests in this file.
Proposed fix
ptr = ffi::malloc(100);
+ assert!(!ptr.is_null());
ffi::free_sized(ptr, 100);
ptr = ffi::aligned_alloc(16, 128);
+ assert!(!ptr.is_null());
- ffi::free_aligned_sized(ptr, 16, 127);
+ ffi::free_aligned_sized(ptr, 16, 128);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ptr = ffi::malloc(100); | |
| ffi::free_sized(ptr, 100); | |
| ptr = ffi::aligned_alloc(16, 127); | |
| ffi::free_aligned_sized(ptr, 16, 127); | |
| ptr = ffi::malloc(100); | |
| assert!(!ptr.is_null()); | |
| ffi::free_sized(ptr, 100); | |
| ptr = ffi::aligned_alloc(16, 128); | |
| assert!(!ptr.is_null()); | |
| ffi::free_aligned_sized(ptr, 16, 128); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jemallocator/tests/ffi.rs` around lines 26 - 29, The aligned allocation call
violates its precondition and lacks a null check; change the size passed to
ffi::aligned_alloc from 127 to a multiple of 16 (e.g., 128) and update the
matching ffi::free_aligned_sized call accordingly, and add null-pointer
assertions for both allocations (after ffi::malloc and after ffi::aligned_alloc)
so the test checks ptr.is_null() (or uses the file's existing assert pattern)
before freeing; reference: ffi::malloc, ffi::free_sized, ffi::aligned_alloc,
ffi::free_aligned_sized.
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
|
When might you release this in a new release of tikv-jemalloc? Looking for a rough schedule or plan. ta |
3 participants
Merge the nightly branch and upgrade jemalloc to upstream 5.3.1.
Upstream v5.3.1 already includes the patches previously carried in tikv/jemalloc, so the submodule now points directly to https://github.com/jemalloc/jemalloc.
New functions or mallctl options introduced in 5.3.1 are not added yet. These can be added in follow-up PRs.
Summary by CodeRabbit
New Features
Chores