Hi everyone,
I'm using LAMMPS both as a library and with the "custom" fix approach (btw, great job: the plug in structure of the code is really great!) and I have noticed that the STUBS/mpi.h doesn't actually contains valid C code (which is not irrelevant, since the same file could be included in C code using the library.h interface...).
I'm not an expert, but I think that the (formally correct) definition:
struct MPI_Status {
int MPI_SOURCE;
};
should be changed to something like:
typedef struct __MPI_Status {
int MPI_SOURCE;
} MPI_Status;
in order for the subsequent code to conform both to the C standard and the MPI specification, for example to match the MPI_Recv call:
int MPI_Recv( void *buf, int count, MPI_Datatype datatype, int source, int tag, MPI_Comm comm, MPI_Status *status )
This doesn't affect the build with g++ (I checked) and works fine with gcc. Could this change be included in the next release (I didn't posted a patch since the modification doesn't seem to worth it.. of course i can do it, if it matters)?
Thank you for the attention, and for all the good work
Riccardo Di Meo
PS: I have also notice, at least in older versions (26Dec) some issues about the library interface that I could maybe try to resolve: Is there a "formal" procedure to provide patches and have them accepted?
Hi everyone,
ciao riccardo,
how is life in trieste?
I'm using LAMMPS both as a library and with the "custom" fix approach
(btw, great job: the plug in structure of the code is really great!) and
I have noticed that the STUBS/mpi.h doesn't actually contains valid C
code (which is not irrelevant, since the same file could be included in
C code using the library.h interface...).
I'm not an expert, but I think that the (formally correct) definition:
struct MPI_Status {
int MPI_SOURCE;
};
should be changed to something like:
typedef struct __MPI_Status {
int MPI_SOURCE;
} MPI_Status;
i'd say you are correct. however, you may also have noticed
that MPI_Status is only used in MPI APIs that cannot be used
with the Stubs code in the first place.
in order for the subsequent code to conform both to the C standard and
the MPI specification, for example to match the MPI_Recv call:
int MPI_Recv( void *buf, int count, MPI_Datatype datatype, int source, int tag, MPI_Comm comm, MPI_Status *status )
This doesn't affect the build with g++ (I checked) and works fine with
gcc. Could this change be included in the next release (I didn't posted
a patch since the modification doesn't seem to worth it.. of course i
can do it, if it matters)?
Thank you for the attention, and for all the good work
Riccardo Di Meo
PS: I have also notice, at least in older versions (26Dec) some issues
about the library interface that I could maybe try to resolve: Is there
a "formal" procedure to provide patches and have them accepted?
send an e-mail with an archive containing the patch and the
modified full sources to steve, and provide a detailed description
of what the change is doing and why.
that used to work fine for me in the past.
say hi to everybody in trieste!
cheers,
axel.
p.s.: our first story with the nvidia folks has finally come out.
have a look at: http://www.nvidia.com/object/io_1263443092139.html
or check out http://www.youtube.com/watch?v=0DhHUMrtmPc
i'm now curious to see what they do with the footage they
recorded at the ICTP school in december... 
Axel Kohlmeyer wrote:
Hi everyone,
ciao riccardo,
Hi Axel!
how is life in trieste?
full of surprises and excitement as usual, you know...
I'm using LAMMPS both as a library and with the "custom" fix approach (btw, great job: the plug in structure of the code is really great!) and I have noticed that the STUBS/mpi.h doesn't actually contains valid C code (which is not irrelevant, since the same file could be included in C code using the library.h interface...).
I'm not an expert, but I think that the (formally correct) definition:
struct MPI_Status {
int MPI_SOURCE;
};
should be changed to something like:
typedef struct __MPI_Status {
int MPI_SOURCE;
} MPI_Status;
i'd say you are correct. however, you may also have noticed
that MPI_Status is only used in MPI APIs that cannot be used
with the Stubs code in the first place.
I see you are right, however whether the functions are going to be actually used or not, the problem is that the header breaks C compilation, regardless how it's used: an alternative for users willing to bypass that, would be for them to have their own mpi.h file, just in order to compile code that uses both the MPI stubs and library.h, however it seems quite a convoluted solution (especially if one without side-effects can be provided).
in order for the subsequent code to conform both to the C standard and the MPI specification, for example to match the MPI_Recv call:
int MPI_Recv( void *buf, int count, MPI_Datatype datatype, int source, int tag, MPI_Comm comm, MPI_Status *status )
This doesn't affect the build with g++ (I checked) and works fine with gcc. Could this change be included in the next release (I didn't posted a patch since the modification doesn't seem to worth it.. of course i can do it, if it matters)?
Thank you for the attention, and for all the good work
Riccardo Di Meo
PS: I have also notice, at least in older versions (26Dec) some issues about the library interface that I could maybe try to resolve: Is there a "formal" procedure to provide patches and have them accepted?
send an e-mail with an archive containing the patch and the modified full sources to steve, and provide a detailed description
of what the change is doing and why.
that used to work fine for me in the past.
ok, thank you for the information!
say hi to everybody in trieste!
cheers,
axel.
I will: just send Shawn and Ben my regards too!
I hope you are doing fine in Philly, see you soon on the ML
Cheers,
RDM
p.s.: our first story with the nvidia folks has finally come out.
have a look at: http://www.nvidia.com/object/io_1263443092139.html
or check out http://www.youtube.com/watch?v=0DhHUMrtmPc
i'm now curious to see what they do with the footage they recorded at the ICTP school in december...
(me too 
)
Thanks!
>> I'm using LAMMPS both as a library and with the "custom" fix approach
>> (btw, great job: the plug in structure of the code is really great!) and
>> I have noticed that the STUBS/mpi.h doesn't actually contains valid C
>> code (which is not irrelevant, since the same file could be included in
>> C code using the library.h interface...).
>>
>> I'm not an expert, but I think that the (formally correct) definition:
>>
>> struct MPI_Status {
>>
>> int MPI_SOURCE;
>>
>> };
>>
>>
>> should be changed to something like:
>>
>> typedef struct __MPI_Status {
>>
>> int MPI_SOURCE;
>>
>> } MPI_Status;
>>
>
>
> i'd say you are correct. however, you may also have noticed
> that MPI_Status is only used in MPI APIs that cannot be used
> with the Stubs code in the first place.
on second thought, the code in mpi.h is correct c++ code.
it is only wrong in c semantics.
in c++ a struct is effectively a class with all public members
and automatically defines a new type.
I see you are right, however whether the functions are going to be
actually used or not, the problem is that the header breaks C
compilation, regardless how it's used: an alternative for users willing
to bypass that, would be for them to have their own mpi.h file, just in
order to compile code that uses both the MPI stubs and library.h,
however it seems quite a convoluted solution (especially if one without
side-effects can be provided).
not sure what is the best solution here.
if you want to fix the stubs library to use c semantics, you
also have to add code to the header to indicate c bindings
for the functions even when included into c++ code and also
fix up the double_int struct which has the same issue.
ciao,
axel.
Axel Kohlmeyer wrote:
I'm using LAMMPS both as a library and with the "custom" fix approach (btw, great job: the plug in structure of the code is really great!) and I have noticed that the STUBS/mpi.h doesn't actually contains valid C code (which is not irrelevant, since the same file could be included in C code using the library.h interface...).
I'm not an expert, but I think that the (formally correct) definition:
struct MPI_Status {
int MPI_SOURCE;
};
should be changed to something like:
typedef struct __MPI_Status {
int MPI_SOURCE;
} MPI_Status;
i'd say you are correct. however, you may also have noticed
that MPI_Status is only used in MPI APIs that cannot be used
with the Stubs code in the first place.
on second thought, the code in mpi.h is correct c++ code. it is only wrong in c semantics.
in c++ a struct is effectively a class with all public members
and automatically defines a new type.
I see you are right, however whether the functions are going to be actually used or not, the problem is that the header breaks C compilation, regardless how it's used: an alternative for users willing to bypass that, would be for them to have their own mpi.h file, just in order to compile code that uses both the MPI stubs and library.h, however it seems quite a convoluted solution (especially if one without side-effects can be provided).
not sure what is the best solution here.
if you want to fix the stubs library to use c semantics, you
also have to add code to the header to indicate c bindings for the functions even when included into c++ code and also fix up the double_int struct which has the same issue.
I forgot that... On my defense: LAMMPS seemed to compile smoothly with my modifications anyway (but it could be because of some GCCism, so I'll look into that too and submit a patch once the problem will be corrected).
Thanks for the remarks
Ciao,
Riccardo
I have noticed that the STUBS/mpi.h doesn't actually contains valid C
code (which is not irrelevant, since the same file could be included in
C code using the library.h interface...).
It's meant to be included in C++ files, not C files. So it is
C++ code, not C code.
Steve
Steve Plimpton wrote:
I have noticed that the STUBS/mpi.h doesn't actually contains valid C
code (which is not irrelevant, since the same file could be included in
C code using the library.h interface...).
It's meant to be included in C++ files, not C files. So it is
C++ code, not C code.
Steve
in order to compile LAMMPS without MPI, I know.
But still however the library.h file, which includes "mpi.h" and presumably is made to include the STUBS/mpi.h as well when needed, do provides C header compatibility and therefore the mpi.h should probably provide it as well.
Anyway, not a big deal: I was just hoping for a solution that could make everybody happy, if it can't be done, changing the mpi.h file manually for C code or including a different one, altough not elegant, will do...
Riccardo
Hadn't thought of the C-style library.h interface. If you send
me a modified STUBS/mpi.h that is also C-compatible,
I'll take a look.
Steve
steve,
Hadn't thought of the C-style library.h interface. If you send
me a modified STUBS/mpi.h that is also C-compatible,
the cleanest approach would be to
compile the whole thing in c.
the changes are fairly trivial.
check out the attached tar archive.
cheers,
axel.
lammps-mpi-stub.tar.gz (2.4 KB)