Why not? I use asserts all the time in OMPI code. A quick grep in ompi/mca says I'm not alone. There are a whole bunch of places where I "know" a fact, such as a pointer never being NULL or consistency checks between two values. These don't need to run in production; I've theoretically tested the crap out of the code before then. But they are really useful when developing and debugging later.
Just my $0.02. Brian On 11/4/11 5:18 PM, "Jeff Squyres" <jsquy...@cisco.com> wrote: >+1 -- we shouldn't be using assert(). > > >On Nov 4, 2011, at 2:15 PM, Tim Mattox wrote: > >> I doubt you want to be using assert() here or anywhere in the >> OMPI library. If the rem_info.rem_qps pointer is NULL, I would >> think you want to do something other than just die. >> And, in a production build, if it was to ever be null, things would >> eventually crash right? The ompi library should strive to never >>crash... >> maybe print an error message and abort, but flat out crashing >> seems wrong. >> >> I've been away from active OMPI development for awhile now so I >> might be remembering this incorrectly, but isn't there some >> policy/SOP on not using assert in the OMPI library code? >> >> Please excuse me while I return to being a silent/lurking ghost >> of an OMPI developer. >> >> On Thu, Nov 3, 2011 at 8:15 PM, <cy...@osl.iu.edu> wrote: >>> Author: cyeoh >>> Date: 2011-11-03 20:15:08 EDT (Thu, 03 Nov 2011) >>> New Revision: 25431 >>> URL: https://svn.open-mpi.org/trac/ompi/changeset/25431 >>> >>> Log: >>> Removes pointless memmove which because of a previous memcpy will >>>always >>> have identical source and destination pointers. See #2871 >>> Plugs a couple of minor memory leaks related to remote qp info >>> >>> >>> Text files modified: >>> trunk/ompi/mca/btl/openib/btl_openib_endpoint.c | 3 >>>+++ >>> trunk/ompi/mca/btl/openib/connect/btl_openib_connect_oob.c | 22 >>>++++++++++++---------- >>> 2 files changed, 15 insertions(+), 10 deletions(-) >>> >>> Modified: trunk/ompi/mca/btl/openib/btl_openib_endpoint.c >>> >>>======================================================================== >>>====== >>> --- trunk/ompi/mca/btl/openib/btl_openib_endpoint.c (original) >>> +++ trunk/ompi/mca/btl/openib/btl_openib_endpoint.c 2011-11-03 >>>20:15:08 EDT (Thu, 03 Nov 2011) >>> @@ -452,6 +452,9 @@ >>> free(endpoint->qps); >>> endpoint->qps = NULL; >>> >>> + free(endpoint->rem_info.rem_qps); >>> + free(endpoint->rem_info.rem_srqs); >>> + >>> /* unregister xrc recv qp */ >>> #if HAVE_XRC >>> if (0 != endpoint->xrc_recv_qp_num) { >>> >>> Modified: trunk/ompi/mca/btl/openib/connect/btl_openib_connect_oob.c >>> >>>======================================================================== >>>====== >>> --- trunk/ompi/mca/btl/openib/connect/btl_openib_connect_oob.c >>>(original) >>> +++ trunk/ompi/mca/btl/openib/connect/btl_openib_connect_oob.c >>>2011-11-03 20:15:08 EDT (Thu, 03 Nov 2011) >>> @@ -272,19 +272,14 @@ >>> static int set_remote_info(mca_btl_base_endpoint_t* endpoint, >>> mca_btl_openib_rem_info_t* rem_info) >>> { >>> + /* Free up the memory pointed to by rem_qps before overwriting >>>the pointer >>> + in the following memcpy */ >>> + free(endpoint->rem_info.rem_qps); >>> + >>> /* copy the rem_info stuff */ >>> memcpy(&((mca_btl_openib_endpoint_t*) endpoint)->rem_info, >>> rem_info, sizeof(mca_btl_openib_rem_info_t)); >>> >>> - /* copy over the rem qp info */ >>> - /* per #2871, changed this from memcpy() to memmove() to handle >>> - * the case of overlapping (or same) src/dest addresses. >>> - * However, we still *should* figure out why the src and dest >>> - * addresses are sometimes the same. */ >>> - memmove(endpoint->rem_info.rem_qps, >>> - rem_info->rem_qps, sizeof(mca_btl_openib_rem_qp_info_t) * >>> - mca_btl_openib_component.num_qps); >>> - >>> BTL_VERBOSE(("Setting QP info, LID = %d", >>>endpoint->rem_info.rem_lid)); >>> return OMPI_SUCCESS; >>> } >>> @@ -671,7 +666,12 @@ >>> uint8_t message_type; >>> bool master; >>> >>> - /* start by unpacking data first so we know who is knocking at >>> + /* We later memcpy this whole structure. Make sure >>> + that all the parameters are initialized, especially >>> + the pointers */ >>> + memset(&rem_info,0, sizeof(rem_info)); >>> + >>> + /* start by unpacking data first so we know who is knocking at >>> our door */ >>> BTL_VERBOSE(("unpacking %d of %d\n", cnt, OPAL_UINT8)); >>> rc = opal_dss.unpack(buffer, &message_type, &cnt, OPAL_UINT8); >>> @@ -857,6 +857,7 @@ >>> to CONNECTING, and then reply with our QP >>> information */ >>> if (master) { >>> + assert(rem_info.rem_qps != NULL); >>> rc = reply_start_connect(ib_endpoint, &rem_info); >>> } else { >>> rc = >>>oob_module_start_connect(ib_endpoint->endpoint_local_cpc, >>> @@ -879,6 +880,7 @@ >>> break; >>> >>> case MCA_BTL_IB_CONNECTING : >>> + assert(rem_info.rem_qps != NULL); >>> set_remote_info(ib_endpoint, &rem_info); >>> if (OMPI_SUCCESS != (rc = qp_connect_all(ib_endpoint))) { >>> BTL_ERROR(("endpoint connect error: %d", rc)); >>> _______________________________________________ >>> svn-full mailing list >>> svn-f...@open-mpi.org >>> http://www.open-mpi.org/mailman/listinfo.cgi/svn-full >>> >> >> >> >> -- >> Tim Mattox, Ph.D. - http://homepage.mac.com/tmattox/ >> timat...@open-mpi.org || tmat...@gmail.com >> I'm a bright... http://www.the-brights.net/ >> >> _______________________________________________ >> devel mailing list >> de...@open-mpi.org >> http://www.open-mpi.org/mailman/listinfo.cgi/devel > > >-- >Jeff Squyres >jsquy...@cisco.com >For corporate legal information go to: >http://www.cisco.com/web/about/doing_business/legal/cri/ > > >_______________________________________________ >devel mailing list >de...@open-mpi.org >http://www.open-mpi.org/mailman/listinfo.cgi/devel > > -- Brian W. Barrett Dept. 1423: Scalable System Software Sandia National Laboratories
