On Wed, Oct 31, 2012 at 5:04 PM, Jeff Squyres <jsquy...@cisco.com> wrote:
> On Oct 31, 2012, at 9:38 AM, Dmitri Gribenko wrote:
>
>>> The rationale here is that correct MPI applications should not need to add 
>>> any extra compiler files to compile without warnings.
>>
>> I would disagree with this.  Compiler warnings are most useful when
>> they are on by default.  Only a few developers will turn on a warning
>> because warnings are hard to discover and enabling a warning requires
>> an explicit action from the developer.
>
> Understood, but:
>
> a) MPI explicitly allows this kind of deliberate mismatch.  It does not make 
> sense to warn for things that are correct in MPI.

I don't think it is MPI.  It is the C standard that allows one to
store any pointer in void* and char*.  But C standard also considers
lots of other weird things to be valid, see below.

> b) Warnings are significantly less useful if the user looks at them and says, 
> "the compiler is wrong; I know that MPI says that this deliberate mismatch in 
> my code is ok."

Well, one can also argue that since the following is valid C, the
warning in question should not be implemented at all:

long *b = malloc(sizeof(int));
MPI_Recv(b, 1, MPI_INT, ...);

But this code is clearly badly written, so we are left with a question
about where to draw the line.

> c) as such, these warnings are really only useful for the application where 
> type/MPI_Datatype matching is expected/desired.

Compilers already warn about valid C code.  Silencing many warnings
relies on conventions that are derived from best practices of being
explicit about something unusual.  For example:

$ cat /tmp/aaa.c
void foo(void *a) {
  for(int i = a; i < 10; i++)
  {
    if(i = 5)
      return;
  }
}
$ clang -fsyntax-only -std=c99 /tmp/aaa.c
/tmp/aaa.c:2:11: warning: incompatible pointer to integer conversion
initializing 'int' with an expression of type 'void *'
[-Wint-conversion]
  for(int i = a; i < 10; i++)
          ^   ~
/tmp/aaa.c:4:10: warning: using the result of an assignment as a
condition without parentheses [-Wparentheses]
    if(i = 5)
       ~~^~~
/tmp/aaa.c:4:10: note: place parentheses around the assignment to
silence this warning
    if(i = 5)
         ^
       (    )
/tmp/aaa.c:4:10: note: use '==' to turn this assignment into an
equality comparison
    if(i = 5)
         ^
         ==
2 warnings generated.

According to C standard this is valid C code, but clang emits two
warnings on this.

> Can these warnings be enabled as part of the warnings rollup -Wall option?  
> That would be an easy way to find/enable these warnings.

IIRC, -Wall warning set is frozen in clang.  -Wall is misleading in
that it does not turn on all warnings implemented in the compiler.
Clang has -Weverything to really turn on all warnings.  But
-Weverything is very noisy (by design, not to be fixed) unless one
also turns off all warnings that are not interesting for the project
with -Wno-foo.

I don't think it is possible to disable this warning by default
because off-by-default warnings are discouraged in Clang.  There is no
formal policy, but the rule of thumb is: either make the warning good
enough for everyone or don't implement it; if some particular app does
something strange, it can disable this warning.

>> The pattern you described is an important one, but most MPI
>> applications will have matching buffer types/type tags.
>
> I agree that most applications *probably* don't do this.  But significant 
> developer in this community (i.e., Sandia) has at least multiple applications 
> that *do* do it.  I can't ignore that.  :-(

Here are a few approaches to solving this in order of preference:

0. Is this really a concern for Sandia?  (I.e., do they target Clang?)

1. Ask the developer to silence the warning with a cast to 'void *' or
-Wno-type-safety.  Rationale: compilers already do warn about valid
but suspicious code.

2. Turn off checking for char* just like for void*.  Rationale: C
standard allows char* to alias a pointer of any type.  Note that char*
is special in this regard (strict aliasing rules).

3. Turn off annotations by default in mpi.h.

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <griboz...@gmail.com>*/

Reply via email to