[mpich-discuss] [PATCH v2] test: add attrdeleteget, MPI_Attr_get called from delete_fn

[Jed Brown]

Fab Tillier <ftillier at microsoft.com> writes:

>>+int main(int argc,char **argv)
>>+{
>>+  MPI_Comm scomm;
>>+
>>+  MTest_Init(&argc,&argv);
>>+  MPI_Comm_split(MPI_COMM_WORLD,1,0,&scomm);
>>+  MPI_Keyval_create(MPI_NULL_COPY_FN,delete_fn,&key,(void*)0);
>>+  MPI_Attr_put(scomm,key,a);
>>+  MPI_Comm_free(&scomm);
>
> The communicator handle will become invalid at some point after you
> call MPI_Comm_free (but before the call returns).  I didn't see in the
> standard any mention of whether using the communicator in the destroy
> callback is acceptable.  

Why pass the communicator as an argument to delete_fn if the MPI
implementation is allowed to have already made it invalid?

Since delete_fn is called by MPI_Comm_free, any use of the first
argument passed to delete_fn would be erroneous.  IMO, the presence of
the argument and lack of any comments stating that it should never be
used imply that it was intended to be valid.  It is valid with all other
MPI implementations that we've used (most of them).

> There's the usual vague notion of the call to MPI_Comm_free being
> erroneous if the delete callback fails.

Yes, the test relies on that.  The problem is MPI_Attr_get returning an
error when called from delete_fn, presumably due to eager invalidation
of the communicator.

>>+  MPI_Finalize();
>>+  return 0;
>>+}
>>+
>>+int delete_fn(MPI_Comm comm,int keyval,void *attr_val,void *extra_state)
>>+{
>>+  int   flg;
>>+  void  *ptr;
>>+
>>+  return MPI_Attr_get(comm,key,&ptr,&flg);
>
> Why aren't you using attr_val and extra_state here, rather than
> calling MPI_Attr_get?  Is this just a symptom of the test case being
> reduced, and the original code attempts to access a different
> attribute on that communicator?

It's a consequence of reducing, but PETSc can restructure (by using
separate destructors for each of the two relevant keyvals) so that it is
not necessary to call MPI_Attr_get in a delete_fn.  I'll make that
change, but I think it is worth fixing in MS-MPI as well.