Changed --mpi to --kernel-loader, updated comments#1216
Changed --mpi to --kernel-loader, updated comments#1216xuyao0127 merged 2 commits intodmtcp:mainfrom
Conversation
WalkthroughThe changes update the command-line interface and internal logic in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant dmtcp_launch
participant Kernel
participant UpperHalfProgram
User->>dmtcp_launch: Launch with --kernel-loader
dmtcp_launch->>dmtcp_launch: Parse arguments, set enableKernelLoader = true
dmtcp_launch->>Kernel: Set ADDR_NO_RANDOMIZE personality (if supported)
dmtcp_launch->>dmtcp_launch: Set UH_PRELOAD environment variable
dmtcp_launch->>UpperHalfProgram: Start with UH_PRELOAD
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/dmtcp_launch.cpp (1)
844-849: Fix inconsistent environment variable handling for 32-bit ELF.There are two issues in the 32-bit ELF handling:
- Line 844 unconditionally sets
LD_PRELOAD, which conflicts with kernel loader mode- Lines 845-846 and 847-849 use
preloadLibsinstead ofpreloadLibs32Apply this diff to fix the logic:
- setenv("LD_PRELOAD", preloadLibs32.c_str(), 1); - if (enableKernelLoader) { - setenv("UH_PRELOAD", preloadLibs.c_str(), 1); - } else { - setenv("LD_PRELOAD", preloadLibs.c_str(), 1); - } + if (enableKernelLoader) { + setenv("UH_PRELOAD", preloadLibs32.c_str(), 1); + } else { + setenv("LD_PRELOAD", preloadLibs32.c_str(), 1); + }
🧹 Nitpick comments (1)
src/dmtcp_launch.cpp (1)
499-500: Fix grammatical error in the comment.The comment has a subject-verb disagreement.
Apply this diff to fix the grammar:
- // Test and set env var for FSGSBASE. The env var is used by plugins that uses + // Test and set env var for FSGSBASE. The env var is used by plugins that use
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/dmtcp_launch.cpp(4 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)
src/dmtcp_launch.cpp (4)
140-144: LGTM! Clear and accurate usage documentation.The usage message clearly describes the new
--kernel-loaderoption's functionality and its effects on the environment setup.
185-186: LGTM! Variable declaration follows established patterns.The
enableKernelLoadervariable is properly initialized and follows the same naming convention as other plugin flags in the codebase.
321-326: LGTM! Argument processing is correctly implemented.The logic properly sets the
enableKernelLoaderflag and conditionally applies theADDR_NO_RANDOMIZEpersonality based on kernel version support. The conditional compilation is consistent with the header includes.
829-833: LGTM! Correct environment variable handling for the main case.The logic properly sets
UH_PRELOADwhen kernel loader mode is enabled, otherwise falls back toLD_PRELOAD.
xuyao0127
force-pushed
the
kernel-loader
branch
from
c1564c5 to
86568d1
Compare
gc00
left a comment
gc00
left a comment
There was a problem hiding this comment.
See my request for more information.
| @@ -836,11 +841,10 @@ setLDPreloadLibs(bool is32bitElf) | |||
| " ./configure --enable-m32 && make clean && make -j && " | |||
| "make install\n" | |||
| " ./configure && make clean && make -j && make install\n"); | |||
There was a problem hiding this comment.
I don't understand the use of preloadLibs32 instead of the original preloadLibs, below. I thought preloadLibs32 would be used only for the rare (almost obsolete) case of mixed 32-bit and 64-bit libraries. Perhaps if you added a comment or two here, this change would be clearer. Thanks.
There was a problem hiding this comment.
@xuyao0127 , Thanks for explaining this orally. LGTM.
2 participants
Both MANA and CRAC needs to set the
ADDR_NO_RANDOMIZEpersonality, and useUH_PRELOADenv var instead ofLD_PRELOAD. I changed the flag fordmtcp_launchfrom--mpito--kernel-loaderand updated related comments so that both MANA and CRAC can use the same--kernel-loaderflag.Summary by CodeRabbit
New Features
--kernel-loadercommand-line option for advanced plugin support.Improvements
UH_PRELOADwhen kernel loader mode is enabled, improving compatibility with certain plugins.