April 2014 - devel - mailman3.cels.anl.gov

Re: [mpich-devel] progress in flushall
by Jeff Hammond 30 Apr '14

30 Apr '14

On Wed, Apr 30, 2014 at 3:58 AM, Balaji, Pavan <balaji(a)anl.gov> wrote: > > On Apr 29, 2014, at 10:20 PM, Jeff Hammond <jeff.science(a)gmail.com> wrote: >> Yeah, I know that's what MPICH gives me. My point is that is >> undesirable. All sync operations should make progress, no matter >> what. > > They are not required to make progress from the spec perspective. It’s a bad assumption to make that Win_flush will always make progress. I think it’s OK for it to be mostly a no-op when there are no operations posted. If you need progress, why don’t you use test or something similar? I want flushall to make progress because there might be incoming RMA that won't be processed otherwise. While today MPICH makes progress on all Ch3 ops in Barrier, if that ever changes, ARMCI_Barrier is going to deadlock because it does win_flush_all then mpi_barrier. If flush_all doesn't process incoming RMA, the initiators cannot return from win_flush_all and will never proceed to barrier, while the processes that haven't initiated any RMA will be in mpi_barrier not make progress on RMA. If win_unlock_all behaves like win_flush_all and win_free doesn't make progress, then I can get the same deadlock scenario in ARMCI_Free. I'm already adding Iprobe for progress inside of ARMCI-MPI, but it seems silly to have to test for p2p to get progress in RMA and for RMA sync ops to not guarantee progress. I'd much prefer the design where p2p and rma progress are decoupled and my RMA sync ops are burdened with progress on p2p and vice versa. Best, Jeff -- Jeff Hammond jeff.science(a)gmail.com

1 0

Re: [mpich-devel] Suboptimal MPI_Allreduce() for intercommunicators
by Rajeev Thakur 30 Apr '14

30 Apr '14

It was probably done that way to avoid the need for memory allocation, but can you add a ticket in Trac and we will look at it. (For example, on one side of the intercommunicator sendbuf=1 byte, recvbuf=1 gigabyte; on the other side, sendbuf=1 gigabyte, recvbuf=1 byte) Rajeev On Apr 30, 2014, at 4:44 AM, Lisandro Dalcin <dalcinl(a)gmail.com> wrote: > The implementation of Allreduce for intercommunicator > (MPIR_Allreduce_inter in src/mpi/coll/allreduce.c) uses more or less > the following algorithm (this is Python code I'm using to test this > issue) > > def allreduce_inter_mpich(obj, op, comm, tag, localcomm, low_group): > zero = 0 > if comm.rank == 0: > root = MPI.ROOT > else: > root = MPI.PROC_NULL > if low_group: > ignore = reduce_inter(obj, op, zero, comm, tag, localcomm) > result = reduce_inter(obj, op, root, comm, tag, localcomm) > else: > result = reduce_inter(obj, op, root, comm, tag, localcomm) > ignore = reduce_inter(obj, op, zero, comm, tag, localcomm) > return localcomm.bcast(result, 0) > > > However, while the broadcasts at each group overlap, the calls to > reduce_inter() introduce serialization. A much better implementation > would be: > > def allreduce_inter_dalcinl(obj, op, comm, tag, localcomm): > result = reduce_binomial(obj, op, 0, localcomm, tag) > if comm.rank == 0: > result = comm.sendrecv(result, 0, tag, None, 0, tag) > return localcomm.bcast(result, 0) > > i.e, perform (overlaped) reductions in the local groups, exchange > results between local and remote rank 0, and (overlaped) broadcast in > the local groups. > > I'm ataching a test Python script (I do not expect you to run it :-), > but perhaps you want to see the code). I'm defining a reduce operation > that artificially sleeps 1 second. Running this code in 8 cores in my > desktop clearly shows the issue with the MPICH implementation: > > $ mpiexec -n 8 python test-reduce.py > [mpich] time: min=4.003491e+00 max=4.003569e+00 > [dalcinl] time: min=2.002367e+00 max=2.002456e+00 > > What do you think? Am I right? Or perhaps I'm missing something obvious? > > > -- > Lisandro Dalcin > --------------- > CIMEC (UNL/CONICET) > Predio CONICET-Santa Fe > Colectora RN 168 Km 472, Paraje El Pozo > 3000 Santa Fe, Argentina > Tel: +54-342-4511594 (ext 1016) > Tel/Fax: +54-342-4511169 > <test-allreduce.py>_______________________________________________ > To manage subscription options or unsubscribe: > https://lists.mpich.org/mailman/listinfo/devel

1 0

Suboptimal MPI_Allreduce() for intercommunicators
by Lisandro Dalcin 30 Apr '14

30 Apr '14

The implementation of Allreduce for intercommunicator (MPIR_Allreduce_inter in src/mpi/coll/allreduce.c) uses more or less the following algorithm (this is Python code I'm using to test this issue) def allreduce_inter_mpich(obj, op, comm, tag, localcomm, low_group): zero = 0 if comm.rank == 0: root = MPI.ROOT else: root = MPI.PROC_NULL if low_group: ignore = reduce_inter(obj, op, zero, comm, tag, localcomm) result = reduce_inter(obj, op, root, comm, tag, localcomm) else: result = reduce_inter(obj, op, root, comm, tag, localcomm) ignore = reduce_inter(obj, op, zero, comm, tag, localcomm) return localcomm.bcast(result, 0) However, while the broadcasts at each group overlap, the calls to reduce_inter() introduce serialization. A much better implementation would be: def allreduce_inter_dalcinl(obj, op, comm, tag, localcomm): result = reduce_binomial(obj, op, 0, localcomm, tag) if comm.rank == 0: result = comm.sendrecv(result, 0, tag, None, 0, tag) return localcomm.bcast(result, 0) i.e, perform (overlaped) reductions in the local groups, exchange results between local and remote rank 0, and (overlaped) broadcast in the local groups. I'm ataching a test Python script (I do not expect you to run it :-), but perhaps you want to see the code). I'm defining a reduce operation that artificially sleeps 1 second. Running this code in 8 cores in my desktop clearly shows the issue with the MPICH implementation: $ mpiexec -n 8 python test-reduce.py [mpich] time: min=4.003491e+00 max=4.003569e+00 [dalcinl] time: min=2.002367e+00 max=2.002456e+00 What do you think? Am I right? Or perhaps I'm missing something obvious? -- Lisandro Dalcin --------------- CIMEC (UNL/CONICET) Predio CONICET-Santa Fe Colectora RN 168 Km 472, Paraje El Pozo 3000 Santa Fe, Argentina Tel: +54-342-4511594 (ext 1016) Tel/Fax: +54-342-4511169

1 0

Re: [mpich-devel] progress in flushall
by Balaji, Pavan 30 Apr '14

30 Apr '14

On Apr 29, 2014, at 10:20 PM, Jeff Hammond <jeff.science(a)gmail.com> wrote: > Yeah, I know that's what MPICH gives me. My point is that is > undesirable. All sync operations should make progress, no matter > what. They are not required to make progress from the spec perspective. It’s a bad assumption to make that Win_flush will always make progress. I think it’s OK for it to be mostly a no-op when there are no operations posted. If you need progress, why don’t you use test or something similar? — Pavan

1 0

Re: [mpich-devel] progress in flushall
by Jeff Hammond 29 Apr '14

29 Apr '14

Yeah, I know that's what MPICH gives me. My point is that is undesirable. All sync operations should make progress, no matter what. Best, Jeff On Tue, Apr 29, 2014 at 10:17 PM, Balaji, Pavan <balaji(a)anl.gov> wrote: > Jeff, > > If there are no local RMA operations, Win_flush will essentially be a no-op, so no it’ll not make any progress. > > — Pavan > > On Apr 29, 2014, at 12:18 PM, Jeff Hammond <jeff.science(a)gmail.com> wrote: > >> win_flush_all mentions poking the progress engine for async progress >> but it won't actually do this if a given process has not initiated any >> RMA operations. This defeats the purpose of using win_flush_all to >> drive progress locally. >> >> Maybe I am misreading the code, but I'd sleep better at night if I was >> absolutely certain that win_flush_all hit the progress at least once. >> >> Thanks, >> >> Jeff >> >> (the following excerpts are easily found in src/mpid/ch3/src/ch3u_rma_sync.c) >> >> int MPIDI_Win_flush_all(MPID_Win *win_ptr) >> { >> int mpi_errno = MPI_SUCCESS; >> int i; >> MPIDI_STATE_DECL(MPIDI_STATE_MPIDI_WIN_FLUSH_ALL); >> >> MPIDI_RMA_FUNC_ENTER(MPIDI_STATE_MPIDI_WIN_FLUSH_ALL); >> >> MPIU_ERR_CHKANDJUMP(win_ptr->epoch_state != MPIDI_EPOCH_LOCK && >> win_ptr->epoch_state != MPIDI_EPOCH_LOCK_ALL, >> mpi_errno, MPI_ERR_RMA_SYNC, "**rmasync"); >> >> /* FIXME: Performance -- we should not process the ops separately. >> * Ideally, we should be able to use the same infrastructure that's used by >> * active target to complete all operations. */ >> >> /* Note: Local RMA calls don't poke the progress engine. This routine >> * should poke the progress engine when the local target is flushed to help >> * make asynchronous progress. Currently this is handled by Win_flush(). >> */ >> for (i = 0; i < MPIR_Comm_size(win_ptr->comm_ptr); i++) { >> if (MPIDI_CH3I_RMA_Ops_head(&win_ptr->targets[i].rma_ops_list) == NULL) >> continue; >> if (win_ptr->targets[i].remote_lock_state != MPIDI_CH3_WIN_LOCK_NONE) { >> mpi_errno = win_ptr->RMAFns.Win_flush(i, win_ptr); >> if (mpi_errno != MPI_SUCCESS) { MPIU_ERR_POP(mpi_errno); } >> } >> } >> >> >> int MPIDI_Win_flush(int rank, MPID_Win *win_ptr) >> { >> int mpi_errno = MPI_SUCCESS; >> int wait_for_rma_done_pkt = 0; >> MPIDI_RMA_Op_t *rma_op; >> MPIDI_STATE_DECL(MPID_STATE_MPIDI_WIN_FLUSH); >> >> MPIDI_RMA_FUNC_ENTER(MPID_STATE_MPIDI_WIN_FLUSH); >> >> MPIU_ERR_CHKANDJUMP(win_ptr->epoch_state != MPIDI_EPOCH_LOCK && >> win_ptr->epoch_state != MPIDI_EPOCH_LOCK_ALL, >> mpi_errno, MPI_ERR_RMA_SYNC, "**rmasync"); >> >> /* Check if win_lock was called */ >> MPIU_ERR_CHKANDJUMP(win_ptr->targets[rank].remote_lock_state == >> MPIDI_CH3_WIN_LOCK_NONE, >> mpi_errno, MPI_ERR_RMA_SYNC, "**rmasync"); >> >> /* Local flush: ops are performed immediately on the local process */ >> if (rank == win_ptr->comm_ptr->rank) { >> MPIU_Assert(win_ptr->targets[rank].remote_lock_state == >> MPIDI_CH3_WIN_LOCK_GRANTED); >> MPIU_Assert(MPIDI_CH3I_RMA_Ops_isempty(&win_ptr->targets[rank].rma_ops_list)); >> >> /* If flush is used as a part of polling for incoming data, we can >> * deadlock, since local RMA calls never poke the progress engine. So, >> * make extra progress here to avoid this problem. */ >> mpi_errno = MPIDI_CH3_Progress_poke(); >> if (mpi_errno) MPIU_ERR_POP(mpi_errno); >> goto fn_exit; >> } >> >> /* NOTE: All flush and req-based operations are currently implemented in >> terms of MPIDI_Win_flush. When this changes, those operations will also >> need to insert this read/write memory fence for shared memory windows. */ >> >> if (win_ptr->shm_allocated == TRUE) { >> OPA_read_write_barrier(); >> } >> >> rma_op = MPIDI_CH3I_RMA_Ops_head(&win_ptr->targets[rank].rma_ops_list); >> >> /* If there is no activity at this target (e.g. lock-all was called, but we >> * haven't communicated with this target), don't do anything. */ >> if (win_ptr->targets[rank].remote_lock_state == MPIDI_CH3_WIN_LOCK_CALLED >> && rma_op == NULL) >> { >> goto fn_exit; >> } >> >> -- >> Jeff Hammond >> jeff.science(a)gmail.com >> _______________________________________________ >> To manage subscription options or unsubscribe: >> https://lists.mpich.org/mailman/listinfo/devel > > _______________________________________________ > To manage subscription options or unsubscribe: > https://lists.mpich.org/mailman/listinfo/devel -- Jeff Hammond jeff.science(a)gmail.com

1 0

Re: [mpich-devel] progress in flushall
by Balaji, Pavan 29 Apr '14

29 Apr '14

Jeff, If there are no local RMA operations, Win_flush will essentially be a no-op, so no it’ll not make any progress. — Pavan On Apr 29, 2014, at 12:18 PM, Jeff Hammond <jeff.science(a)gmail.com> wrote: > win_flush_all mentions poking the progress engine for async progress > but it won't actually do this if a given process has not initiated any > RMA operations. This defeats the purpose of using win_flush_all to > drive progress locally. > > Maybe I am misreading the code, but I'd sleep better at night if I was > absolutely certain that win_flush_all hit the progress at least once. > > Thanks, > > Jeff > > (the following excerpts are easily found in src/mpid/ch3/src/ch3u_rma_sync.c) > > int MPIDI_Win_flush_all(MPID_Win *win_ptr) > { > int mpi_errno = MPI_SUCCESS; > int i; > MPIDI_STATE_DECL(MPIDI_STATE_MPIDI_WIN_FLUSH_ALL); > > MPIDI_RMA_FUNC_ENTER(MPIDI_STATE_MPIDI_WIN_FLUSH_ALL); > > MPIU_ERR_CHKANDJUMP(win_ptr->epoch_state != MPIDI_EPOCH_LOCK && > win_ptr->epoch_state != MPIDI_EPOCH_LOCK_ALL, > mpi_errno, MPI_ERR_RMA_SYNC, "**rmasync"); > > /* FIXME: Performance -- we should not process the ops separately. > * Ideally, we should be able to use the same infrastructure that's used by > * active target to complete all operations. */ > > /* Note: Local RMA calls don't poke the progress engine. This routine > * should poke the progress engine when the local target is flushed to help > * make asynchronous progress. Currently this is handled by Win_flush(). > */ > for (i = 0; i < MPIR_Comm_size(win_ptr->comm_ptr); i++) { > if (MPIDI_CH3I_RMA_Ops_head(&win_ptr->targets[i].rma_ops_list) == NULL) > continue; > if (win_ptr->targets[i].remote_lock_state != MPIDI_CH3_WIN_LOCK_NONE) { > mpi_errno = win_ptr->RMAFns.Win_flush(i, win_ptr); > if (mpi_errno != MPI_SUCCESS) { MPIU_ERR_POP(mpi_errno); } > } > } > > > int MPIDI_Win_flush(int rank, MPID_Win *win_ptr) > { > int mpi_errno = MPI_SUCCESS; > int wait_for_rma_done_pkt = 0; > MPIDI_RMA_Op_t *rma_op; > MPIDI_STATE_DECL(MPID_STATE_MPIDI_WIN_FLUSH); > > MPIDI_RMA_FUNC_ENTER(MPID_STATE_MPIDI_WIN_FLUSH); > > MPIU_ERR_CHKANDJUMP(win_ptr->epoch_state != MPIDI_EPOCH_LOCK && > win_ptr->epoch_state != MPIDI_EPOCH_LOCK_ALL, > mpi_errno, MPI_ERR_RMA_SYNC, "**rmasync"); > > /* Check if win_lock was called */ > MPIU_ERR_CHKANDJUMP(win_ptr->targets[rank].remote_lock_state == > MPIDI_CH3_WIN_LOCK_NONE, > mpi_errno, MPI_ERR_RMA_SYNC, "**rmasync"); > > /* Local flush: ops are performed immediately on the local process */ > if (rank == win_ptr->comm_ptr->rank) { > MPIU_Assert(win_ptr->targets[rank].remote_lock_state == > MPIDI_CH3_WIN_LOCK_GRANTED); > MPIU_Assert(MPIDI_CH3I_RMA_Ops_isempty(&win_ptr->targets[rank].rma_ops_list)); > > /* If flush is used as a part of polling for incoming data, we can > * deadlock, since local RMA calls never poke the progress engine. So, > * make extra progress here to avoid this problem. */ > mpi_errno = MPIDI_CH3_Progress_poke(); > if (mpi_errno) MPIU_ERR_POP(mpi_errno); > goto fn_exit; > } > > /* NOTE: All flush and req-based operations are currently implemented in > terms of MPIDI_Win_flush. When this changes, those operations will also > need to insert this read/write memory fence for shared memory windows. */ > > if (win_ptr->shm_allocated == TRUE) { > OPA_read_write_barrier(); > } > > rma_op = MPIDI_CH3I_RMA_Ops_head(&win_ptr->targets[rank].rma_ops_list); > > /* If there is no activity at this target (e.g. lock-all was called, but we > * haven't communicated with this target), don't do anything. */ > if (win_ptr->targets[rank].remote_lock_state == MPIDI_CH3_WIN_LOCK_CALLED > && rma_op == NULL) > { > goto fn_exit; > } > > -- > Jeff Hammond > jeff.science(a)gmail.com > _______________________________________________ > To manage subscription options or unsubscribe: > https://lists.mpich.org/mailman/listinfo/devel

1 0

Re: [mpich-devel] Trivial patch for MPE's configure
by Balaji, Pavan 29 Apr '14

29 Apr '14

Thanks, Lisandro. devel(a)mpich.org is the right list for this. We’ll look through this patch. — Pavan On Apr 29, 2014, at 4:49 AM, Lisandro Dalcin <dalcinl(a)gmail.com> wrote: > Dear Pavan, I'm not sure if a direct email to you is OK, but I'm not > sure where to send MPE-related stuff. Should I just use the devel@ > mailing list? > > I'm attaching a trivial patch to configure.ac, please review and > commit. With this patch, I'm able to build MPE with mpich 3.0.4 as > packaged in Fedora 20. > > Automatic detection of fortran compilers is not working so well, I had > to configure like this to get it work (ignore the -fPIC flags, this is > a particular requeriment): > > $ ./configure --prefix=/home/devel/mpe CFLAGS=-fPIC FFLAGS=-fPIC > LDFLAGS=-fPIC MPI_CC=mpicc MPI_F77=mpif77 F77=gfortran > > Regards, > > -- > Lisandro Dalcin > --------------- > CIMEC (UNL/CONICET) > Predio CONICET-Santa Fe > Colectora RN 168 Km 472, Paraje El Pozo > 3000 Santa Fe, Argentina > Tel: +54-342-4511594 (ext 1016) > Tel/Fax: +54-342-4511169 > <mpe-configure.diff>

1 0

progress in flushall
by Jeff Hammond 29 Apr '14

29 Apr '14

win_flush_all mentions poking the progress engine for async progress but it won't actually do this if a given process has not initiated any RMA operations. This defeats the purpose of using win_flush_all to drive progress locally. Maybe I am misreading the code, but I'd sleep better at night if I was absolutely certain that win_flush_all hit the progress at least once. Thanks, Jeff (the following excerpts are easily found in src/mpid/ch3/src/ch3u_rma_sync.c) int MPIDI_Win_flush_all(MPID_Win *win_ptr) { int mpi_errno = MPI_SUCCESS; int i; MPIDI_STATE_DECL(MPIDI_STATE_MPIDI_WIN_FLUSH_ALL); MPIDI_RMA_FUNC_ENTER(MPIDI_STATE_MPIDI_WIN_FLUSH_ALL); MPIU_ERR_CHKANDJUMP(win_ptr->epoch_state != MPIDI_EPOCH_LOCK && win_ptr->epoch_state != MPIDI_EPOCH_LOCK_ALL, mpi_errno, MPI_ERR_RMA_SYNC, "**rmasync"); /* FIXME: Performance -- we should not process the ops separately. * Ideally, we should be able to use the same infrastructure that's used by * active target to complete all operations. */ /* Note: Local RMA calls don't poke the progress engine. This routine * should poke the progress engine when the local target is flushed to help * make asynchronous progress. Currently this is handled by Win_flush(). */ for (i = 0; i < MPIR_Comm_size(win_ptr->comm_ptr); i++) { if (MPIDI_CH3I_RMA_Ops_head(&win_ptr->targets[i].rma_ops_list) == NULL) continue; if (win_ptr->targets[i].remote_lock_state != MPIDI_CH3_WIN_LOCK_NONE) { mpi_errno = win_ptr->RMAFns.Win_flush(i, win_ptr); if (mpi_errno != MPI_SUCCESS) { MPIU_ERR_POP(mpi_errno); } } } int MPIDI_Win_flush(int rank, MPID_Win *win_ptr) { int mpi_errno = MPI_SUCCESS; int wait_for_rma_done_pkt = 0; MPIDI_RMA_Op_t *rma_op; MPIDI_STATE_DECL(MPID_STATE_MPIDI_WIN_FLUSH); MPIDI_RMA_FUNC_ENTER(MPID_STATE_MPIDI_WIN_FLUSH); MPIU_ERR_CHKANDJUMP(win_ptr->epoch_state != MPIDI_EPOCH_LOCK && win_ptr->epoch_state != MPIDI_EPOCH_LOCK_ALL, mpi_errno, MPI_ERR_RMA_SYNC, "**rmasync"); /* Check if win_lock was called */ MPIU_ERR_CHKANDJUMP(win_ptr->targets[rank].remote_lock_state == MPIDI_CH3_WIN_LOCK_NONE, mpi_errno, MPI_ERR_RMA_SYNC, "**rmasync"); /* Local flush: ops are performed immediately on the local process */ if (rank == win_ptr->comm_ptr->rank) { MPIU_Assert(win_ptr->targets[rank].remote_lock_state == MPIDI_CH3_WIN_LOCK_GRANTED); MPIU_Assert(MPIDI_CH3I_RMA_Ops_isempty(&win_ptr->targets[rank].rma_ops_list)); /* If flush is used as a part of polling for incoming data, we can * deadlock, since local RMA calls never poke the progress engine. So, * make extra progress here to avoid this problem. */ mpi_errno = MPIDI_CH3_Progress_poke(); if (mpi_errno) MPIU_ERR_POP(mpi_errno); goto fn_exit; } /* NOTE: All flush and req-based operations are currently implemented in terms of MPIDI_Win_flush. When this changes, those operations will also need to insert this read/write memory fence for shared memory windows. */ if (win_ptr->shm_allocated == TRUE) { OPA_read_write_barrier(); } rma_op = MPIDI_CH3I_RMA_Ops_head(&win_ptr->targets[rank].rma_ops_list); /* If there is no activity at this target (e.g. lock-all was called, but we * haven't communicated with this target), don't do anything. */ if (win_ptr->targets[rank].remote_lock_state == MPIDI_CH3_WIN_LOCK_CALLED && rma_op == NULL) { goto fn_exit; } -- Jeff Hammond jeff.science(a)gmail.com

1 0

Re: [mpich-devel] Issue with MPI_Error_string() for user-defined codes/clases
by Rob Latham 24 Apr '14

24 Apr '14

Hey guys this is turning into a real bikeshed. The patch is good the way it is. There's really no good universal way to pretty up the way C deals with strings and as Larry Baker says a lot of it comes down to personal style preferences. I'm just waiting for Jenkins to finish building it on all platforms before pushing to master. something terrible happened to jenkins last evening... ==rob On 04/24/2014 02:11 AM, Larry Baker wrote: > Lisandro, > > You are correct about the C string initializer syntax. I was not paying such close attention to your patch as to pass along to you my feelings that C mixes up integers (especially the chameleon 0) as other types. To me, "" is more communicative of the purpose. I do not have the full text of your comments about the changes you made to the original patch in front of me. I thought you mentioned changing a "" initializer to { 0 }. > > Though it is trivial, as I recall, the array initializer syntax also specifies that unspecified elements are initialized with zeros. If that is true, then { 0 } is not exactly the same as "", since { 0 } requires zero-initializing the entire char array whereas "" requires zero-initializing only the first element. I'm not sure an intelligent compiler would do anything different than push a block of zeros on the stack in either case. > > Take my comments as a friendly communication. It is not necessary for you to change the trac ticket on my behalf. > > Cordially, > > Larry Baker > US Geological Survey > 650-329-5608 > baker(a)usgs.gov > > > > > On Apr 23, 2014, at 11:34 PM, Lisandro Dalcin wrote: > >> On 23 April 2014 23:11, Larry Baker <baker(a)usgs.gov> wrote: >>> Lisandro, >>> >>> As a matter of personal taste, I prefer that empty strings be used as >>> initializers for C strings. One of the things I dislike extremely about C >>> (and, one of the reasons I think it is so easy in C to write bad code) is >>> its mixing of integers and pointers and integers and chars. Even though { 0 >>> } is identical to { "" } when initializing C strings, I find the latter more >>> informative. I know the C standard specifies that strings are terminated by >>> the null character, which is elsewhere specified to be a byte with all bits >>> equal to zero. However, "" is the literal empty string, whereas 0 (or '\0') >>> is the underlying implementation of the empty string. >> >> Are you talking about this single line:? >> >> static const char empty_error_string[1] = { 0 }; >> >> I guess you are proposing to change it to: >> >> static const char empty_error_string[1] = ""; >> >> Please note {""} is an array of strings. Perhaps you meant {'\0'}, >> i.e. the null char literal? >> >> Please confirm your concerns were related to that single line of my >> patch, so I can update the trac ticket. >> >> >> -- >> Lisandro Dalcin >> --------------- >> CIMEC (UNL/CONICET) >> Predio CONICET-Santa Fe >> Colectora RN 168 Km 472, Paraje El Pozo >> 3000 Santa Fe, Argentina >> Tel: +54-342-4511594 (ext 1016) >> Tel/Fax: +54-342-4511169 > -- Rob Latham Mathematics and Computer Science Division Argonne National Lab, IL USA

1 0

Re: [mpich-devel] Issue with MPI_Error_string() for user-defined codes/clases
by Larry Baker 24 Apr '14

24 Apr '14

Lisandro, You are correct about the C string initializer syntax. I was not paying such close attention to your patch as to pass along to you my feelings that C mixes up integers (especially the chameleon 0) as other types. To me, "" is more communicative of the purpose. I do not have the full text of your comments about the changes you made to the original patch in front of me. I thought you mentioned changing a "" initializer to { 0 }. Though it is trivial, as I recall, the array initializer syntax also specifies that unspecified elements are initialized with zeros. If that is true, then { 0 } is not exactly the same as "", since { 0 } requires zero-initializing the entire char array whereas "" requires zero-initializing only the first element. I'm not sure an intelligent compiler would do anything different than push a block of zeros on the stack in either case. Take my comments as a friendly communication. It is not necessary for you to change the trac ticket on my behalf. Cordially, Larry Baker US Geological Survey 650-329-5608 baker(a)usgs.gov On Apr 23, 2014, at 11:34 PM, Lisandro Dalcin wrote: > On 23 April 2014 23:11, Larry Baker <baker(a)usgs.gov> wrote: >> Lisandro, >> >> As a matter of personal taste, I prefer that empty strings be used as >> initializers for C strings. One of the things I dislike extremely about C >> (and, one of the reasons I think it is so easy in C to write bad code) is >> its mixing of integers and pointers and integers and chars. Even though { 0 >> } is identical to { "" } when initializing C strings, I find the latter more >> informative. I know the C standard specifies that strings are terminated by >> the null character, which is elsewhere specified to be a byte with all bits >> equal to zero. However, "" is the literal empty string, whereas 0 (or '\0') >> is the underlying implementation of the empty string. > > Are you talking about this single line:? > > static const char empty_error_string[1] = { 0 }; > > I guess you are proposing to change it to: > > static const char empty_error_string[1] = ""; > > Please note {""} is an array of strings. Perhaps you meant {'\0'}, > i.e. the null char literal? > > Please confirm your concerns were related to that single line of my > patch, so I can update the trac ticket. > > > -- > Lisandro Dalcin > --------------- > CIMEC (UNL/CONICET) > Predio CONICET-Santa Fe > Colectora RN 168 Km 472, Paraje El Pozo > 3000 Santa Fe, Argentina > Tel: +54-342-4511594 (ext 1016) > Tel/Fax: +54-342-4511169

1 0