Skip to content

Fix MPIR_proctable structure visibility #4891

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 13, 2018

Conversation

jjhursey
Copy link
Member

@jjhursey jjhursey commented Mar 8, 2018

  • The MPIR_PROCDESC structure needs to be visible even in optimized
    builds so that debuggers can attach to mpirun and properly read the
    MPIR_proctable.
  • In the v2.0.x and v2.x series this structure resided in the orterun
    directory and included the CFLAGS fix included here. This code
    moved in the v3.x series and the CFLAGS did not move causing this
    issue.

@jjhursey
Copy link
Member Author

jjhursey commented Mar 8, 2018

@jsquyres Tagging you since you made the change 13 years ago to orterun (4c59058). And you love build systems. If there is a better way to do this let me know.

FYI @rhc54 this was the actual MPIR issue that we were seeing.

@jjhursey
Copy link
Member Author

jjhursey commented Mar 8, 2018

Ref #4892

@rhc54
Copy link
Contributor

rhc54 commented Mar 8, 2018

looks ok to me - thx!

# post-processed forms of the CFLAGS in the library targets down
# below.

CFLAGS = $(CFLAGS_WITHOUT_OPTFLAGS) $(DEBUGGER_CFLAGS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. It strikes me a bit of a bummer that we have to compile the entire orted without optimization flags.

Is there a way to segregate the needed symbols into a single, small .c file and compile only that file without optimization flags?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one option you could consider - if you get people to use "prun" for executing jobs under the DVM, then the debugger code and orted_submit stuff could go back into orterun. It would take a little work to move it, but not too horrible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, I made a proof of concept, and the -g flag only has to be passed to orte/orted/orted_submit.lo, orte/libopen-rte.la and orte/tools/orted/orted

the inline patch below addresses the last two ones, but I could not figure out how to pass that flag to orted_submit.lo (orte/orted/Makefile.am is included in orte/Makefile.am, so the "object" file is orted/orted_submit.lo and I do not know how to handle the / in Makefile.am

diff --git a/orte/Makefile.am b/orte/Makefile.am
index 6af81a2..c9d27bc 100644
--- a/orte/Makefile.am
+++ b/orte/Makefile.am
@@ -41,7 +41,7 @@ lib@ORTE_LIB_PREFIX@open_rte_la_LIBADD = \
        $(MCA_orte_FRAMEWORK_LIBS) \
        $(ORTE_TOP_BUILDDIR)/opal/lib@[email protected]
 lib@ORTE_LIB_PREFIX@open_rte_la_DEPENDENCIES = $(libopen_rte_la_LIBADD)
-lib@ORTE_LIB_PREFIX@open_rte_la_LDFLAGS = -version-info $(libopen_rte_so_version)
+lib@ORTE_LIB_PREFIX@open_rte_la_LDFLAGS = -version-info $(libopen_rte_so_version) $(DEBUGGER_CFLAGS)
 
 # included subdirectory Makefile.am's and appended-to variables
 headers =
diff --git a/orte/tools/orted/Makefile.am b/orte/tools/orted/Makefile.am
index 92211a3..32fe2bc 100644
--- a/orte/tools/orted/Makefile.am
+++ b/orte/tools/orted/Makefile.am
@@ -43,7 +43,7 @@ orted_SOURCES = orted.c
 #  nothing to -all-static in the Makefile.in
 #  nice for systems that don't have all the shared
 #  libraries on the computes
-orted_LDFLAGS =
+orted_LDFLAGS = $(DEBUGGER_CFLAGS)
 orted_LDADD = \
        $(top_builddir)/orte/lib@[email protected] \
        $(top_builddir)/opal/lib@[email protected]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use this trick to compile just orted_submit.c with debug symbols. Let me give that a try.

If we want to further narrow down how much of orted_submit.c is compiled this way, we can do that in another effort.

@jjhursey jjhursey force-pushed the fix/mpir-symbol-vis branch from f673e60 to f551bfc Compare March 10, 2018 02:14
 * The `MPIR_PROCDESC` structure needs to be visible even in optimized
   builds so that debuggers can attach to `mpirun` and properly read the
   `MPIR_proctable`.
 * In the v2.0.x and v2.x series this structure resided in the `orterun`
   directory and included the `CFLAGS` fix included here. This code
   moved in the v3.x series and the `CFLAGS` did not move causing this
   issue.
   - Instead of applying the debug `CFLAGS` globally to libopen-rte,
     only apply them to the `orted_submit.c` compile which contains the
     MPIR symbols.

Signed-off-by: Joshua Hursey <[email protected]>
@jjhursey jjhursey force-pushed the fix/mpir-symbol-vis branch from f551bfc to ccb4f43 Compare March 10, 2018 02:15
@jjhursey
Copy link
Member Author

jjhursey commented Mar 10, 2018

@ggouaillardet @jsquyres I changed the PR to isolate the debug symbols to just the compilation of orted_submit.c which contains the MPIR symbols. This patch works, and isolates the application of these CFLAGS. Let me know what you think.

I confirmed that this patch does fix the MPIR test case.

@ggouaillardet
Copy link
Contributor

@jjhursey very clever !

@jjhursey jjhursey merged commit ae1d318 into open-mpi:master Mar 13, 2018
@jjhursey jjhursey deleted the fix/mpir-symbol-vis branch March 13, 2018 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants