Conversation
karya0
force-pushed
the
dev/kaarya0/launch-refactor-prep-2
branch
2 times, most recently
from
a17de56 to
b431c50
Compare
karya0
force-pushed
the
dev/kaarya0/launch-refactor-prep-2
branch
from
b431c50 to
f4a00bf
Compare
|
@karya0 , I'll review this one after the 3.1.0 release. It would be good to add a stable release before doing a merge of the headers. |
karya0
force-pushed
the
dev/kaarya0/launch-refactor-prep-2
branch
3 times, most recently
from
d2f5f08 to
ee7abf4
Compare
karya0
force-pushed
the
dev/kaarya0/launch-refactor-prep-2
branch
from
ee7abf4 to
31c58ec
Compare
gc00
left a comment
gc00
left a comment
There was a problem hiding this comment.
Thanks for the very clear description of the PR. And I agree that unifying the two headers is a big win, now that MTCP is not really an independent submodule.
QUESTIONL Have you done a quick test for m32 mode to make sure that we still support that? If not, then one of us should do that, before we push this in.
TWO MORE QUESTIONS: Changing the header means that the newest DMTCP will be incompatible with any previous checkpoint image. Should we:
- Change some header to change the version number in the header? (Or did you already do that?
- And also, when we do a future release, does this means that we must update DMTCP to version 3.2.0, or is it acceptable to do 3.1.1? What is the general practice in hese cases?
REQUEST: As I wrote in teh review, I'd much prefer just adding -std=gnu11 to the CFLAGS directly i configure.ac, if needed. This is far better than sprreading the logic in about 4 or 5 different files. Since 'gnu11' is really a different topic anyway, could we move that commit into a separate, independent PR, where we can discuss it? That will allow us to do a quick review of the more important commit: unifying the DMTCP/MTCP header.
| @@ -28,6 +28,10 @@ AC_PROG_CPP | |||
| AC_CONFIG_MACRO_DIR([m4]) | |||
| AX_CXX_COMPILE_STDCXX([14], [noext], [mandatory]) | |||
|
|
|||
There was a problem hiding this comment.
If I understand this code, it will always try to append -std=gnu11, assuming success. But if the default C compiler of the user is already C11-compatible, then this just adds confusion. Shouldn't we be conditionally appending, instead?
And if I misunderstood this code, then let's add a comment.
| @@ -0,0 +1,50 @@ | |||
| # =========================================================================== | |||
There was a problem hiding this comment.
It's easy in configure.ac to append to CFLAGS. Adding a special m4 file seems to add obfuscation, and the code is no longer local to a single file.
Am I missing something here?
| @@ -112,7 +112,9 @@ dmtcplib_PROGRAMS = $(d_libdir)/libdmtcp.so$(EXEEXT) | |||
| @AARCH64_HOST_TRUE@am__append_2 = -latomic | |||
| subdir = src | |||
| ACLOCAL_M4 = $(top_srcdir)/aclocal.m4 | |||
There was a problem hiding this comment.
And see my note about the .m4 file.
| @@ -100,7 +100,9 @@ libdmtcp_PROGRAMS = $(d_libdir)/libdmtcp_alloc.so$(EXEEXT) \ | |||
| $(d_libdir)/libdmtcp_timer.so$(EXEEXT) | |||
| subdir = src/plugin | |||
| ACLOCAL_M4 = $(top_srcdir)/aclocal.m4 | |||
There was a problem hiding this comment.
Ditto. And it would be nice to set the CFLAGS variable at top level, instead of spreading out some distributed code for eaach directory.
| @@ -50,7 +50,7 @@ endif | |||
|
|
|||
| DMTCP_INCLUDE=${top_srcdir}/include | |||
|
|
|||
gc00
left a comment
gc00
left a comment
There was a problem hiding this comment.
I'm approving this in advance. Please see the two minor changes, requested.
karya0
force-pushed
the
dev/kaarya0/launch-refactor-prep-2
branch
from
31c58ec to
097ca6f
Compare
karya0
force-pushed
the
dev/kaarya0/launch-refactor-prep-2
branch
from
097ca6f to
b448562
Compare
2 participants
This struct merges and replaces the previously separate DMTCP and MTCP headers. This would allow us to refactor dmtcp_launch/restart for MANA usage.
This unified struct is written twice to the ckpt-image-- the first copy is read by dmtcp_restart while the second copy is read by mtcp_restart binary. As an added benefit,
[From conversation with @gc00 : This is done because the ckpt image may be compressed, and so read through a pipe. We cannot use lseek on a pipe. So, we copy an identical copy of the header, so that when reading again from mtcp_restart, we don't need to lseek back to the beginning of the original header. ]
it also simplifies the mtcp_restart logic of searching for ckpt-header inside the ckpt image.
As part of the unified struct declaration, I used anonymous structs/unions to ensure size compatibility across 64-bit, 32-bit, and mixed-mode computation in a clean manner. This is primarily why this PR also moves the codebase to C11 mode. Note that we are already at C++14 (and will soon be at C++17).