Skip to content

Remove asserts that create a segfault in debug mode#121

Open
gvallee wants to merge 12 commits into
masterfrom
debug_mode_assert
Open

Remove asserts that create a segfault in debug mode#121
gvallee wants to merge 12 commits into
masterfrom
debug_mode_assert

Conversation

@gvallee
Copy link
Copy Markdown
Owner

@gvallee gvallee commented Feb 17, 2021

No description provided.

@gvallee gvallee added the WIP Work in progress label Feb 17, 2021
cjlegg added 11 commits February 18, 2021 03:42
@cjlegg
gitignore update (hide JL stuff including results
4ddaaaa 
Signed-off-by: Cyrus James Legg <j.legg.17@ucl.ac.uk>
@cjlegg
set DEBUG=1 in config.h for both alltoall and alltoallv
8c167c1 
to reproduce the error

Signed-off-by: Cyrus James Legg <j.legg.17@ucl.ac.uk>
@cjlegg
add in a build script for context of
a03d446 
 compile errors fixed in subsequent commits

Signed-off-by: Cyrus James Legg <j.legg.17@ucl.ac.uk>
@cjlegg
gcc 8.1.3 want to see a null parameter in macros using varagrs
c75faf3 
Signed-off-by: Cyrus James Legg <j.legg.17@ucl.ac.uk>
@cjlegg
more adding a explicit NULL to macro calls
247b036 
Signed-off-by: Cyrus James Legg <j.legg.17@ucl.ac.uk>
@cjlegg
more adding a explicit NULL to macro calls
742ed10 
Signed-off-by: Cyrus James Legg <j.legg.17@ucl.ac.uk>
@cjlegg
fix references to local and global ranks in a debug print
0df5514 
Signed-off-by: Cyrus James Legg <j.legg.17@ucl.ac.uk>
@cjlegg
more adding a explicit NULL to macro calls
976c8d3 
Signed-off-by: Cyrus James Legg <j.legg.17@ucl.ac.uk>
@cjlegg
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 <j.legg.17@ucl.ac.uk>
@cjlegg
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 <j.legg.17@ucl.ac.uk>
@cjlegg
compile and run scripts added for context
1c4b57e 
Signed-off-by: Cyrus James Legg <j.legg.17@ucl.ac.uk>
gvallee
gvallee commented Feb 18, 2021
View reviewed changes
Copy link
Copy Markdown
Owner Author

@gvallee gvallee left a comment

Choose a reason for hiding this comment

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

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

Comment thread 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)
Copy link
Copy Markdown
Owner Author

@gvallee gvallee Feb 18, 2021

Choose a reason for hiding this comment

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

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.

Comment thread src/alltoallv/config.h
#define _COLLECTIVE_PROFILER_ALLTOALLV_CONFIG_H

#define DEBUG (0)
#define DEBUG (1)
Copy link
Copy Markdown
Owner Author

@gvallee gvallee Feb 18, 2021

Choose a reason for hiding this comment

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

Why is debug active by default?

Comment thread src/alltoall/mpi_alltoall.c

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);
Copy link
Copy Markdown
Owner Author

@gvallee gvallee Feb 18, 2021

Choose a reason for hiding this comment

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

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.

Comment thread src/alltoall/build-scripts/build-at-hpcac.sh
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
Copy link
Copy Markdown
Owner Author

@gvallee gvallee Feb 18, 2021

Choose a reason for hiding this comment

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

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

Comment thread examples/batch-run-alltoall-hpcx-2-7-0-counts-variants.sh
# 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
Copy link
Copy Markdown
Owner Author

@gvallee gvallee Feb 18, 2021

Choose a reason for hiding this comment

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

Should that file ultimately be in the repo?

Comment thread .gitignore


examples/alltoallv_dt_c
src/alltoall/examples/alltoall
Copy link
Copy Markdown
Owner Author

@gvallee gvallee Feb 18, 2021

Choose a reason for hiding this comment

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

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

Comment thread .gitignore
examples/alltoallv_dt_c
src/alltoall/examples/alltoall

# JL stuff including results files
Copy link
Copy Markdown
Owner Author

@gvallee gvallee Feb 18, 2021

Choose a reason for hiding this comment

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

It is either generic or should not be in the repo

Comment thread src/alltoall/config.h
#define _COLLECTIVE_PROFILER_ALLTOALL_CONFIG_H

#define DEBUG (0)
#define DEBUG (1)
Copy link
Copy Markdown
Owner Author

@gvallee gvallee Feb 18, 2021

Choose a reason for hiding this comment

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

Why do we activate the debug mode by default?

Comment thread src/alltoallv/mpi_alltoallv.c
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)
Copy link
Copy Markdown
Owner Author

@gvallee gvallee Feb 18, 2021

Choose a reason for hiding this comment

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

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

Comment thread src/alltoallv/mpi_alltoallv.c
int comm_size;
int i, j;
int localrank;
// int localrank; // not assigned in this function - fixing
Copy link
Copy Markdown
Owner Author

@gvallee gvallee Feb 18, 2021

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown
Owner Author

gvallee commented Feb 18, 2021

Fix #122

@gvallee
Copy link
Copy Markdown
Owner Author

gvallee commented Feb 18, 2021

We need to fix #123 first.

@gvallee gvallee added wontfix This will not be worked on and removed WIP Work in progress labels Feb 19, 2021
@gvallee
Copy link
Copy Markdown
Owner Author

gvallee commented Feb 19, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

wontfix This will not be worked on

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gvallee @cjlegg