Remove asserts that create a segfault in debug mode #121

gvallee · 2021-02-17T18:35:02Z

No description provided.

Signed-off-by: Cyrus James Legg <[email protected]>

to reproduce the error Signed-off-by: Cyrus James Legg <[email protected]>

compile errors fixed in subsequent commits Signed-off-by: Cyrus James Legg <[email protected]>

Signed-off-by: Cyrus James Legg <[email protected]>

but problem is that even for that logger->f is not yet initialsed Signed-off-by: Cyrus James Legg <[email protected]>

… untila _commit_data at the end of sampling final fix will be by deleting those permanetly or intiallising earlier Signed-off-by: Cyrus James Legg <[email protected]>

Signed-off-by: Cyrus James Legg <[email protected]>

gvallee

It seems the debug mode is quite broken and i do not believe this is a clean way to address the problem.

gvallee · 2021-02-18T18:49:56Z

src/alltoall/mpi_alltoall.c

-#if DEBUG
-	fprintf(logger->f, "new entry: %d --> %d --- %d\n", size, newNode->size, newNode->count);
-#endif
+// Temporary solution?? - logger->f is not yet initialised (which is done in _commit_data)


I really do not like that type of comments: it is either a todo and then we need to open an issue or at least clearly say so (i myself do not always open issue for all todos) or dead code in which case it should be deleted.

gvallee · 2021-02-18T18:50:15Z

src/alltoallv/config.h

@@ -7,7 +7,7 @@
 #ifndef _COLLECTIVE_PROFILER_ALLTOALLV_CONFIG_H
 #define _COLLECTIVE_PROFILER_ALLTOALLV_CONFIG_H

-#define DEBUG (0)
+#define DEBUG (1)


Why is debug active by default?

gvallee · 2021-02-18T18:51:27Z

src/alltoall/mpi_alltoall.c

@@ -91,8 +91,8 @@ static bool same_call_counters(avSRCountNode_t *call_data, int *send_counts, int
 	int rank, count_num;
 	int *_counts;

-	DEBUG_ALLTOALL_PROFILING("Comparing data with existing data...\n");
-	DEBUG_ALLTOALL_PROFILING("-> Comparing send counts...\n");
+	DEBUG_ALLTOALL_PROFILING("Comparing data with existing data...\n", NULL);


Another option (which i usually do) is create a DEBUG_ALLTOALL_PROFILING_NOARGS macro. I personally find it to keep the code cleaner but it is clearly a personal preference.

gvallee · 2021-02-18T18:51:53Z

src/alltoall/build-scripts/build-at-hpcac.sh

@@ -4,7 +4,7 @@
 module purge
 module load gcc/8.3.1 hpcx/2.7.0

-PROJECT_ROOT=/global/home/users/cyrusl/placement/expt0066/alltoall_profiling
+PROJECT_ROOT=/global/home/users/cyrusl/placement/expt0070/alltoall_profiling


Is that file ultimately really need to be in the repo?

gvallee · 2021-02-18T18:52:16Z

examples/batch-run-alltoall-hpcx-2-7-0-counts-variants.sh

@@ -28,7 +28,7 @@ THIS_SCRIPT_DIR=$(dirname "$THIS_SCRIPT")

 # environment and modules and some paths etc. for the job 
 # /global/home/users/cyrusl/placement/expt0060/OSU/osu-micro-benchmarks-5.6.3/install/libexec/osu-micro-benchmarks/mpi/collective
-export PROJECT_ROOT=/global/home/users/cyrusl/placement/expt0066
+export PROJECT_ROOT=/global/home/users/cyrusl/placement/expt0070


Should that file ultimately be in the repo?

gvallee · 2021-02-18T18:52:55Z

.gitignore

+
+
+examples/alltoallv_dt_c
+src/alltoall/examples/alltoall


You should not have a example directory in src/alltoall.

gvallee · 2021-02-18T18:53:18Z

.gitignore

+examples/alltoallv_dt_c
+src/alltoall/examples/alltoall
+
+# JL stuff including results files


It is either generic or should not be in the repo

gvallee · 2021-02-18T18:54:00Z

src/alltoall/config.h

@@ -7,7 +7,7 @@
 #ifndef _COLLECTIVE_PROFILER_ALLTOALL_CONFIG_H
 #define _COLLECTIVE_PROFILER_ALLTOALL_CONFIG_H

-#define DEBUG (0)
+#define DEBUG (1)


Why do we activate the debug mode by default?

gvallee · 2021-02-18T18:56:10Z

src/alltoallv/mpi_alltoallv.c

 	int ret;
 	bool need_profile = true;
 	int my_comm_rank;

 	MPI_Comm_size(comm, &comm_size);
-	MPI_Comm_rank(comm, &my_comm_rank);
-
+	MPI_Comm_rank(comm, &my_comm_rank);  // "Determines the rank of the calling process in the communicator." (var names confusing? localrank could fit this)


this is following best practices, there is nothing confusing. A process as a rank on a communicator, so my_comm_rank.

gvallee · 2021-02-18T18:57:20Z

src/alltoallv/mpi_alltoallv.c

@@ -1169,14 +1171,13 @@ int _mpi_alltoallv(const void *sendbuf, const int *sendcounts, const int *sdispl
 {
 	int comm_size;
 	int i, j;
-	int localrank;
+	// int localrank;  // not assigned in this function - fixing


I has nothing to do with the PR so while i am not against it, it is likely to increase the risk of having conflicts with the current intrusive PR we have.

gvallee · 2021-02-18T18:59:05Z

Fix #122

gvallee · 2021-02-18T19:00:52Z

We need to fix #123 first.

gvallee · 2021-02-19T15:44:21Z

This PR won't be merged, identified problems will be addressed one at a time in separate PRs. I leave the PR open for now for reference.

Remove asserts that create a segfault in debug mode

815194f

gvallee added the WIP Work in progress label Feb 17, 2021

cjlegg added 11 commits February 18, 2021 03:42

gitignore update (hide JL stuff including results

4ddaaaa

Signed-off-by: Cyrus James Legg <[email protected]>

set DEBUG=1 in config.h for both alltoall and alltoallv

8c167c1

to reproduce the error Signed-off-by: Cyrus James Legg <[email protected]>

add in a build script for context of

a03d446

compile errors fixed in subsequent commits Signed-off-by: Cyrus James Legg <[email protected]>

gcc 8.1.3 want to see a null parameter in macros using varagrs

c75faf3

Signed-off-by: Cyrus James Legg <[email protected]>

more adding a explicit NULL to macro calls

247b036

Signed-off-by: Cyrus James Legg <[email protected]>

more adding a explicit NULL to macro calls

742ed10

Signed-off-by: Cyrus James Legg <[email protected]>

fix references to local and global ranks in a debug print

0df5514

Signed-off-by: Cyrus James Legg <[email protected]>

more adding a explicit NULL to macro calls

976c8d3

Signed-off-by: Cyrus James Legg <[email protected]>

try to mask out failing refernce to logger-f to ENABLE_RAW_DATA case

9e1a43e

but problem is that even for that logger->f is not yet initialsed Signed-off-by: Cyrus James Legg <[email protected]>

comment out trace prints to logger->f, since logger-f not initialised…

7c17876

… untila _commit_data at the end of sampling final fix will be by deleting those permanetly or intiallising earlier Signed-off-by: Cyrus James Legg <[email protected]>

compile and run scripts added for context

1c4b57e

Signed-off-by: Cyrus James Legg <[email protected]>

gvallee commented Feb 18, 2021

View reviewed changes

gvallee added wontfix This will not be worked on and removed WIP Work in progress labels Feb 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove asserts that create a segfault in debug mode #121

Remove asserts that create a segfault in debug mode #121

gvallee commented Feb 17, 2021

gvallee left a comment

gvallee Feb 18, 2021

gvallee Feb 18, 2021

gvallee Feb 18, 2021

gvallee Feb 18, 2021

gvallee Feb 18, 2021

gvallee Feb 18, 2021

gvallee Feb 18, 2021

gvallee Feb 18, 2021

gvallee Feb 18, 2021

gvallee Feb 18, 2021

gvallee commented Feb 18, 2021

gvallee commented Feb 18, 2021

gvallee commented Feb 19, 2021



		examples/alltoallv_dt_c
		src/alltoall/examples/alltoall

Remove asserts that create a segfault in debug mode #121

Are you sure you want to change the base?

Remove asserts that create a segfault in debug mode #121

Conversation

gvallee commented Feb 17, 2021

gvallee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvallee commented Feb 18, 2021

gvallee commented Feb 18, 2021

gvallee commented Feb 19, 2021