Conversation
WalkthroughAdds a configure option --enable-prgname-prefix / --disable-prgname-prefix that controls emitting ENABLE_PRGNAME_PREFIX into generated config headers; introduces ENV_VAR_DISABLE_PRGNAME_PREFIX and conditional compile/runtime guards so the "DMTCP:" process-name prefixing in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer (runs configure)
participant Configure as configure/configure.ac
participant Build as make
participant Compile as C++ Compiler
participant Runtime as Process (binary)
Dev->>Configure: run configure [--enable-prgname-prefix|--disable-prgname-prefix]
alt enabled (default yes)
Configure->>Configure: set use_prgname_prefix = yes
Configure->>Configure: emit -DENABLE_PRGNAME_PREFIX=1 into confdefs.h
else disabled
Configure->>Configure: leave ENABLE_PRGNAME_PREFIX undefined
end
Configure->>Build: generate Makefiles + config headers
Build->>Compile: compile sources (include config.h)
alt ENABLE_PRGNAME_PREFIX defined at compile
Compile->>Runtime: build with prefixing code included
Note right of Runtime: runtime checks ENV_VAR_DISABLE_PRGNAME_PREFIX
alt env var not set
Runtime->>Runtime: apply "DMTCP:" prefix via prctl (threadlist)
else env var set
Runtime->>Runtime: skip prefixing
end
else not defined
Compile->>Runtime: prefixing code omitted
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/threadlist.cpp (1)
662-673: Critical: Inverted preprocessor logic breaks intended functionality.The conditional compilation logic is inverted. When
NO_DMTCP_PRGNAME_PREFIXis defined, the code currently adds the prefix, but according to the PR description and the comment inconfig.h.in(line 111: "Do not use 'DMTCP:' prefix"), it should skip adding the prefix.The condition on line 662 should be
#ifndefinstead of#ifdefto match the intended behavior.Apply this diff to fix the inverted logic:
-# ifdef NO_DMTCP_PRGNAME_PREFIX +# ifndef NO_DMTCP_PRGNAME_PREFIX if (!Util::strStartsWith(curThread->procname, DMTCP_PRGNAME_PREFIX)) { // Add the "DMTCP:" prefix. string newName = string(DMTCP_PRGNAME_PREFIX) + curThread->procname;
🧹 Nitpick comments (2)
include/config.h.in (1)
54-54: Minor: Quote style inconsistency in comment.The comment changed from single quotes
'atomic'to mixed quotes`atomic'. Consider using consistent quote styling throughout the file.configure (1)
1539-1540: Tighten help text for clarity (optional)Consider wording: “Omit the 'DMTCP:' prefix from process names shown by ps/top (defines NO_DMTCP_PRGNAME_PREFIX). Default: disabled.” This makes scope and default explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
configure(3 hunks)configure.ac(1 hunks)include/config.h.in(3 hunks)src/threadlist.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
include/config.h.in (2)
111-112: LGTM: New macro definition is correctly configured.The new
NO_DMTCP_PRGNAME_PREFIXmacro is properly defined and documented for the pedantic build option. The comment clearly describes its purpose for CUDA checkpoint scenarios.
138-141: LGTM: Updated standard reference.The change from C89 to C90 is accurate—both refer to the same ANSI C standard, but C90 is the more commonly used designation.
configure (1)
849-849: Option recognized by parser — looks good
enable_pedanticis correctly registered in the option parser list. No issues.configure.ac (1)
232-241:--enable-pedantichandling looks solid.Help text, default, and
AC_DEFINEwiring are consistent with the rest of the configure script. No issues spotted.
|
@gc00 Should we make this configuration at compile time or runtime, like using a environment variable? |
gc00
force-pushed
the
pedantic-no-prefix
branch
from
c81b866 to
f2ce01f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
configure (1)
8079-8092: Define wiring OK; verify negative-sense usage and header placeholderThe define injection is correct. Please verify across the tree that:
include/config.h.incontains#undef NO_DMTCP_PRGNAME_PREFIX.- Call sites only add the "DMTCP:" prefix when the macro is NOT defined.
Optionally, add a runtime escape hatch (env var) to override compile-time behavior if needed per reviewer feedback.
Run from repo root:
#!/usr/bin/env bash set -euo pipefail echo "1) Placeholder in config.h.in" rg -n --hidden --no-ignore -C2 '\bNO_DMTCP_PRGNAME_PREFIX\b' include/config.h.in || { echo "Missing #undef NO_DMTCP_PRGNAME_PREFIX in include/config.h.in"; exit 1; } echo "2) Usage sense in src/" rg -n -C3 '(NO_DMTCP_PRGNAME_PREFIX|DMTCP[_A-Z]*PRGNAME[_A-Z]*PREFIX|DMTCP:)' src include || true
🧹 Nitpick comments (1)
configure (1)
1539-1541: Clarify flag name/help; consider alias and runtime override“pedantic” doesn’t convey “suppress DMTCP: prefix”. Suggest:
- Rename or add alias:
--disable-prgname-prefix(preferred UX).- Expand help text to say “omit 'DMTCP:' from process comm (ps(1))” and mention CUDA use-case.
- Optionally support an env var override (e.g.,
DMTCP_NO_PRGNAME_PREFIX=1) to allow runtime control without rebuilds.These are source changes in configure.ac; the generated configure will follow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
configure(3 hunks)configure.ac(1 hunks)include/config.h.in(3 hunks)src/threadlist.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- include/config.h.in
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
src/threadlist.cpp (1)
662-673: Conditional compilation is correctly implemented—no additional changes needed.The
#ifndef NO_DMTCP_PRGNAME_PREFIXguard at lines 662-673 properly excludes the prefix-addition logic when the macro is defined. Verification confirms thatDMTCP_PRGNAME_PREFIXis defined only once (line 56) and referenced only in lines 663 and 665, both within the conditional block. Theprctl(PR_SET_NAME)call at line 674 will execute with the original (unprefixed)procnamewhen the flag is set, which is the intended behavior.configure.ac (1)
232-241: The option name "pedantic" is misleading, but the design choice merits clarification rather than revision.The review correctly identifies that "pedantic" doesn't convey the option's actual purpose. However, examining the codebase reveals this is an intentional design choice consistent with DMTCP's build system:
Naming concern is valid: The option controls CUDA checkpoint compatibility (whether to suppress the "DMTCP:" prefix in process names). The term "pedantic" obscures this purpose.
Compile-time vs runtime is a valid discussion point: DMTCP extensively uses runtime environment variables (DMTCP_TMPDIR, DMTCP_RESTART_PAUSE, DMTCP_GDB_ATTACH_ON_RESTART, etc.), so the suggestion to support runtime configuration via an env var has merit. However, all AC_ARG_ENABLE options in configure.ac are compile-time toggles, making the current approach consistent.
Placement is deliberate: The option is in the "EXPERTS ONLY" section, suggesting specialized use. This explains the non-obvious naming—it's not meant for general users.
The review's suggestions to rename it or support runtime configuration are reasonable improvements but represent design preferences rather than functional issues. The current implementation is correct and follows the project's established patterns.
configure (1)
849-849: Option registered with parser — looks good
enable_pedanticis correctly added to the recognized options set, avoiding failures withenable_option_checking=fatal.
gc00
force-pushed
the
pedantic-no-prefix
branch
from
f2ce01f to
dbf404a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/threadlist.cpp (1)
662-673: Critical: Macro name mismatch prevents feature from working.The conditional compilation checks for
PRGNAME_PREFIX(line 662), butconfigure.acdefinesENABLE_PRGNAME_PREFIX(line 241). This mismatch means the prefixing code will never be compiled, even when--enable-prgname-prefixis used.Apply this diff to fix the macro name:
-# ifdef PRGNAME_PREFIX +# ifdef ENABLE_PRGNAME_PREFIX if (!Util::strStartsWith(curThread->procname, DMTCP_PRGNAME_PREFIX)) { // Add the "DMTCP:" prefix. string newName = string(DMTCP_PRGNAME_PREFIX) + curThread->procname;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
configure(3 hunks)configure.ac(1 hunks)include/config.h.in(3 hunks)src/threadlist.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- include/config.h.in
- configure
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
configure.ac (1)
232-243: Note: PR title and description are inconsistent with implementation.The PR title mentions
--enable-pedantic, but the actual change implements--enable-prgname-prefix. Additionally, the PR description states "defines NO_DMTCP_PRGNAME_PREFIX" but the code definesENABLE_PRGNAME_PREFIX. Please update the PR title and description to accurately reflect the implementation.
gc00
force-pushed
the
pedantic-no-prefix
branch
from
dbf404a to
805ae9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
configure.ac (1)
236-237: Environment variable reference already flagged.This has already been identified in a previous review comment. The help text mentions
DMTCP_DISABLE_PRGNAME_PREFIXas an environment variable alternative, but no runtime environment variable handling is implemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
configure(3 hunks)configure.ac(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- configure
gc00
force-pushed
the
pedantic-no-prefix
branch
2 times, most recently
from
44aacf4 to
cde4510
Compare
|
@xuyao0127 , I changed this PR as we agreed: --disable-prgname-prefix and env. var. DMTCP_DISABLE_PRGNAME_PREFIX now exist. Could you review this again? Thanks. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/threadlist.cpp (1)
635-685: Bug: env-var guard breaks restart path when prefix is disabledThe change from:
if (!restoreInProgress) { ... } else { ... JASSERT(Thread_UpdateState(curThread, ST_RUNNING, ST_SUSPENDED)); ThreadList::waitForAllRestored(curThread); }to:
if (!restoreInProgress) { ... } else if (getenv(ENV_VAR_DISABLE_PRGNAME_PREFIX) == NULL) { ... JASSERT(Thread_UpdateState(curThread, ST_RUNNING, ST_SUSPENDED)); ThreadList::waitForAllRestored(curThread); }means that when
restoreInProgressis true andDMTCP_DISABLE_PRGNAME_PREFIXis set, the wholeelse ifblock is skipped:
Thread_UpdateState(curThread, ST_RUNNING, ST_SUSPENDED)is never called.ThreadList::waitForAllRestored(curThread)is never called.That changes the core restart protocol for threads that happen to run with the env var set, not just the process-name prefixing behavior, and is very likely incorrect/unstable.
The env-var should guard only the prefixing logic, not the whole “restore” branch. A minimal fix is to restore the original
elseand move the env-var test inside the Linux/feature guard:- if (!restoreInProgress) { + if (!restoreInProgress) { ... - JASSERT(DmtcpRWLockUnlock(&threadResumeLock) == 0); - } else if (getenv(ENV_VAR_DISABLE_PRGNAME_PREFIX) == NULL) { - // If the user defined DMTCP_DISABLE_PRGNAME_PREFIX, skip this prefix. -#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 11) -# ifdef ENABLE_PRGNAME_PREFIX - if (!Util::strStartsWith(curThread->procname, DMTCP_PRGNAME_PREFIX)) { - ... - } -# endif - JASSERT(prctl(PR_SET_NAME, curThread->procname) != -1 || errno == EINVAL) - (curThread->procname) (JASSERT_ERRNO) - .Text("prctl(PR_SET_NAME, ...) failed"); -#endif // if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 11) - JASSERT(Thread_UpdateState(curThread, ST_RUNNING, ST_SUSPENDED)); - /* Else restoreinprog >= 1; This stuff executes to do a restart */ - ThreadList::waitForAllRestored(curThread); - } + JASSERT(DmtcpRWLockUnlock(&threadResumeLock) == 0); + } else { +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 11) +# ifdef ENABLE_PRGNAME_PREFIX + if (getenv(ENV_VAR_DISABLE_PRGNAME_PREFIX) == NULL && + !Util::strStartsWith(curThread->procname, DMTCP_PRGNAME_PREFIX)) { + // Add the "DMTCP:" prefix. + string newName = string(DMTCP_PRGNAME_PREFIX) + curThread->procname; + strncpy(curThread->procname, newName.c_str(), + sizeof(curThread->procname)); + curThread->procname[sizeof(curThread->procname) - 1] = '\0'; + } +# endif + JASSERT(prctl(PR_SET_NAME, curThread->procname) != -1 || errno == EINVAL) + (curThread->procname) (JASSERT_ERRNO) + .Text("prctl(PR_SET_NAME, ...) failed"); +#endif // if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 11) + + JASSERT(Thread_UpdateState(curThread, ST_RUNNING, ST_SUSPENDED)); + /* Else restoreinprog >= 1; This stuff executes to do a restart */ + ThreadList::waitForAllRestored(curThread); + }This keeps the restart handshake intact for all cases while still allowing both compile‑time (
ENABLE_PRGNAME_PREFIX) and runtime (DMTCP_DISABLE_PRGNAME_PREFIX) control over the prefixing.
🧹 Nitpick comments (3)
src/constants.h (1)
107-112: Consider whether the new env var should be “protected”
ENV_VAR_DISABLE_PRGNAME_PREFIXis added but not included inENV_VARS_ALL, which is supposed to track all “protected” DMTCP env vars. Please double‑check whether this variable should:
- be preserved/propagated the same way as other DMTCP_* env vars (in which case it likely belongs in
ENV_VARS_ALL), or- remain user‑only and not be treated as protected.
If you intend it to control behavior across restarts and child processes, adding it to
ENV_VARS_ALLmay be appropriate.src/threadlist.cpp (1)
57-58: Optional: guardDMTCP_PRGNAME_PREFIXwith the feature macroWhen
ENABLE_PRGNAME_PREFIXis not defined (compile‑time prefix disabled), the global:static const char *DMTCP_PRGNAME_PREFIX = "DMTCP:";becomes unused. Depending on your compiler flags (especially with pedantic / -Wextra / -Werror), this may trigger unused-variable warnings.
To avoid that, you could wrap the definition:
- static const char *DMTCP_PRGNAME_PREFIX = "DMTCP:"; + #ifdef ENABLE_PRGNAME_PREFIX + static const char *DMTCP_PRGNAME_PREFIX = "DMTCP:"; + #endifNot mandatory, but it keeps pedantic builds quieter.
configure (1)
1539-1544: Help text is clear; consider also mentioning the enable alias.You list
--disable-prgname-prefixand the env var. For completeness, consider noting that--enable-prgname-prefixis the counterpart and is enabled by default. This avoids ambiguity in./configure --help.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
configure(3 hunks)configure.ac(1 hunks)include/config.h.in(3 hunks)src/constants.h(1 hunks)src/threadlist.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- configure.ac
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
include/config.h.in (3)
24-26: New feature macro wiring looks consistentIntroducing
ENABLE_PRGNAME_PREFIXhere matches the usage pattern elsewhere (#ifdef ENABLE_PRGNAME_PREFIX), so the autotools flow should propagate this flag cleanly. No issues from this file’s side.
57-58: Doc tweak foratomiclibrary is fineUsing backticks around
atomicclarifies that it refers to the library name; no functional impact.
138-141: STDC_HEADERS comment clarification is reasonableThe updated wording about C90 and backward compatibility is clearer and still matches how autotools uses
STDC_HEADERS. Nothing else needed here.src/threadlist.cpp (1)
16-16: Includingconstants.hhere is appropriatePulling in
constants.hto useENV_VAR_DISABLE_PRGNAME_PREFIXis consistent with how other env‑var constants are handled. No concerns.configure (2)
849-856: Option registered correctly (default “on”).Adding
enable_prgname_prefixtoac_user_optsis consistent with using--enable/--disableflags and matches the desired default behavior.
8082-8095: All verification checks passed; macro gating and runtime integration are correct.✓
#undef ENABLE_PRGNAME_PREFIXplaceholder present in include/config.h.in:25
✓ No references to oldNO_DMTCP_PRGNAME_PREFIXmacro
✓ENV_VAR_DISABLE_PRGNAME_PREFIXconstant defined in src/constants.h:110
✓ Runtime logic in src/threadlist.cpp:661–667 correctly checks env var and applies#ifdef ENABLE_PRGNAME_PREFIXguardPositive-sense semantics are consistent throughout: enabled by default, disabled only when
DMTCP_DISABLE_PRGNAME_PREFIXis set.
gc00
force-pushed
the
pedantic-no-prefix
branch
from
cde4510 to
204d9ae
Compare
gc00
changed the title
| fi | ||
|
|
||
|
|
||
| # Check whether --enable-prgname-prefix was given. |
There was a problem hiding this comment.
In line 1539 you defined --disable-prgname-prefix, but here you are checking --enable-prgname-prefix. Is this the correct logic?
gc00
force-pushed
the
pedantic-no-prefix
branch
from
204d9ae to
de8932d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
configure (1)
1539-1543: Polish help text for clarity (default + env var usage).
- State default explicitly, e.g., “Default: enabled.”
- Show env var usage form, e.g.,
DMTCP_DISABLE_PRGNAME_PREFIX=1 dmtcp_launch ….
This reduces ambiguity for users skimming--help.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
configure(3 hunks)configure.ac(1 hunks)include/config.h.in(3 hunks)src/constants.h(1 hunks)src/threadlist.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/threadlist.cpp
- src/constants.h
- configure.ac
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
include/config.h.in (1)
24-26: LGTM! Default behavior verified as correct.The
ENABLE_PRGNAME_PREFIXmacro is properly configured to be enabled by default. The configure.ac setsuse_prgname_prefix=yesas the default, and the macro is defined when this condition is true. Users can disable it with--disable-prgname-prefixas intended.configure (2)
849-849: Option recognized by parser — looks good.Adding
enable_prgname_prefixtoac_user_optsensures--disable-prgname-prefixis accepted and setsenable_prgname_prefix=noper Autoconf semantics.
8082-8094: Verification complete; all requirements satisfied.The implementation is correct:
- ✓ Placeholder
#undef ENABLE_PRGNAME_PREFIXpresent in include/config.h.in- ✓ Call sites gate on
#ifdef ENABLE_PRGNAME_PREFIX(src/threadlist.cpp:664)- ✓ Runtime check for
DMTCP_DISABLE_PRGNAME_PREFIXenv var implemented (src/threadlist.cpp:665)- ✓ No stale negative-sense macro found
gc00
force-pushed
the
pedantic-no-prefix
branch
2 times, most recently
from
66233b1 to
771babe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
configure.ac (1)
232-244: Configure option and macro definition for PRGNAME prefix look correct; help text could be a bit clearer.
- The
AC_ARG_ENABLE([prgname-prefix])block with defaultuse_prgname_prefix=yesand theAC_DEFINE([ENABLE_PRGNAME_PREFIX], ...)guard are consistent with the newENABLE_PRGNAME_PREFIXmacro ininclude/config.h.inand the intended behavior.- Referencing
DMTCP_DISABLE_PRGNAME_PREFIXin the help text aligns with the runtime env-var mechanism added elsewhere in the PR.If you want to polish the UX, consider tweaking the help string to explicitly say this is a compile-time toggle (
--enable/--disable-prgname-prefix, default enabled) and thatDMTCP_DISABLE_PRGNAME_PREFIXis the runtime alternative, so the distinction is obvious from./configure --help.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
configure(3 hunks)configure.ac(1 hunks)include/config.h.in(3 hunks)src/constants.h(1 hunks)src/threadlist.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/threadlist.cpp
- configure
- src/constants.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
include/config.h.in (3)
24-26: ENABLE_PRGNAME_PREFIX macro wiring looks consistent.The new
ENABLE_PRGNAME_PREFIXentry matches the autoconf definition inconfigure.acand clearly documents the feature; no issues here.
57-58: HAVE_LIBATOMIC comment is clear and accurate.The updated wording for
HAVE_LIBATOMICcorrectly describes the-latomicdependency and is consistent with its usage later inconfigure.ac.
138-141: STDC_HEADERS comment clarification is fine.Updating the description to reference C90 and note backward compatibility is purely documentation and improves precision without affecting behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
include/config.h.in (1)
24-26: Clarify the comment to explain the feature's relationship to CUDA checkpoint.The comment "(for CUDA ckpt)" is ambiguous. Based on the PR description, CUDA checkpoint API requires the comm field without the prefix, which is why the disable option was added. Consider revising to clarify:
-/* Enable 'DMTCP:' prefix in comm field of 'ps' (for CUDA ckpt) */ +/* Enable 'DMTCP:' prefix in comm field of 'ps'. Disable for CUDA ckpt API. */Or more explicitly:
-/* Enable 'DMTCP:' prefix in comm field of 'ps' (for CUDA ckpt) */ +/* Enable 'DMTCP:' prefix in comm field shown by 'ps' (enabled by default; use --disable-prgname-prefix or DMTCP_DISABLE_PRGNAME_PREFIX for CUDA ckpt) */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
configure(3 hunks)configure.ac(1 hunks)include/config.h.in(3 hunks)src/constants.h(1 hunks)src/threadlist.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/threadlist.cpp
- configure
- src/constants.h
- configure.ac
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
include/config.h.in (2)
57-57: Incidental formatting change from autoheader regeneration.The backticks around 'atomic' appear to be from regenerating this file with autoheader, rather than a manual change for this PR. This is expected for auto-generated files.
138-141: Incidental documentation improvement from autoheader regeneration.The updated comment (C89→C90 and backward compatibility note) reflects changes in modern autoconf/autoheader versions. This is expected when regenerating config.h.in and is unrelated to the PR's prgname-prefix feature.
* PRGNAME_PREFIX macro created in config.h; The 'DMTCP:' prefix will
be prepended to the comm field in 'ps' on restarted processes _only_
if this macro is defined (default).
* This is needed for the CUDA ckpt API, which uses the 'comm' field
on restart
* This has the same effect as: ./configure --disable-prgname-prefix
gc00
force-pushed
the
pedantic-no-prefix
branch
from
771babe to
3c005f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
configure (1)
1539-1543: Help text okay; minor UX polish suggestion.The disable help reads well. Consider clarifying the env var usage, e.g., “set DMTCP_DISABLE_PRGNAME_PREFIX=1” to make the expected value explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
configure(3 hunks)configure.ac(1 hunks)include/config.h.in(3 hunks)src/constants.h(1 hunks)src/threadlist.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/constants.h
- src/threadlist.cpp
- include/config.h.in
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
configure (2)
849-849: Option registered correctly; supports both --enable/--disable forms.Adding
enable_prgname_prefixtoac_user_optslets Autoconf accept--disable-prgname-prefixby settingenable_prgname_prefix=no, and--enable-prgname-prefix[=yes/no]likewise. LGTM.
8082-8095: Header placeholder and macro usage verified correctly.All verification points passed:
#undef ENABLE_PRGNAME_PREFIXpresent in include/config.h.in (line 25)- Call site correctly uses
#ifdef ENABLE_PRGNAME_PREFIXin src/threadlist.cpp (line 664)- Runtime gate properly checks
ENV_VAR_DISABLE_PRGNAME_PREFIXenvironment variable (src/threadlist.cpp:665, src/constants.h:110)- No stale macro name references found
- No problematic numeric
#if ENABLE_PRGNAME_PREFIXchecks presentThe implementation is consistent and ready.
configure.ac (1)
232-243: Configure knob matches runtime escape hatch.The new
--disable-prgname-prefixoption cleanly mirrors theDMTCP_DISABLE_PRGNAME_PREFIXruntime toggle and follows the existing Autoconf patterns. Looks good to me.
2 participants
Summary by CodeRabbit
New Features
Documentation