[mpich-devel] [mpich-commits] [mpich] MPICH primary repository branch, master, updated. v3.2a1-34-geda105b

[William Gropp]

There are two points here:

1) Should the code compile “cleanly” with every compiler and many choices of warning options?  This sounds like a good idea on the surface, but in fact it is not.  Lots of compilers generate too many false positives, and suppressing them often requires adding unnecessary code and distorting the rest of the code.  That is the case here (the same compiler that can’t figure out that the array isn’t referenced before use is going to have to perform the initialization, adding unnecessary overhead and thus latency).  Rather, the code should compile cleanly with some compiler and set of warnings that is accurate and effective.  The fact that some compiler has poor accuracy in its warnings should not be a reason to harm the code just to keep that compiler quiet.  Note that this is more important for performance sensitive code (particularly for latency) such as MPICH; however, even for non-performance-sensitive codes, making the compiler happy can distort the code, making it harder, not easier, to maintain.

2) Language version choices and coding standards.  The real point here is that coding standards are important and should be followed by anyone working on the code.  Changes to the coding standard need to be made collectively and explicitly; individuals must not decide that they know better.  On this specific question, it may very well be time to move to C99, but that must be a deliberate, collective choice.  And the issue is not whether such compilers exist, but whether our user community uses and trusts them.  At this point, the answer to that question is probably yes as well, but that’s the correct question to ask (and yes, a deliberate and explicit decision could be made to require a C99 compiler and exclude any users that insist on sticking with C89).

Junchao made the right call.  

Bill

On Oct 1, 2014, at 11:15 PM, Dave Goodell (dgoodell) <dgoodell at cisco.com> wrote:

> On Oct 1, 2014, at 10:20 PM, Junchao Zhang <jczhang at mcs.anl.gov> wrote:
> 
>> On Wed, Oct 1, 2014 at 5:40 PM, Dave Goodell (dgoodell) <dgoodell at cisco.com>
>> wrote:
>> 
>>> Begin forwarded message:
>>> 
>>>> - Log -----------------------------------------------------------------
>>>> 
>>> http://git.mpich.org/mpich.git/commitdiff/eda105b029572ebf7f4008f6de2e1afb9ccf6fc2
>>>> 
>>>> commit eda105b029572ebf7f4008f6de2e1afb9ccf6fc2
>>>> Author: Junchao Zhang <jczhang at mcs.anl.gov>
>>>> Date:   Tue Sep 16 15:58:55 2014 -0500
>>>> 
>>>>  Remove unnessary array init in MPIU_CHKLMEM_DECL
>>>> 
>>>>  Initializing mpiu_chklmem_stk_sp_ to 0 should already have the effect
>>> to
>>>>  initialize mpiu_chklmem_stk_[n_].
>>>> 
>>>>  Signed-off-by: Antonio J. Pena <apenya at mcs.anl.gov>
>>>> 
>>>> diff --git a/src/include/mpimem.h b/src/include/mpimem.h
>>>> index a217e8b..e1478a5 100644
>>>> --- a/src/include/mpimem.h
>>>> +++ b/src/include/mpimem.h
>>>> @@ -317,7 +317,7 @@ extern char *strdup( const char * );
>>>> }}
>>>> #else
>>>> #define MPIU_CHKLMEM_DECL(n_) \
>>>> - void *(mpiu_chklmem_stk_[n_]) = {0};\
>>>> + void *(mpiu_chklmem_stk_[n_]); \
>>>> int mpiu_chklmem_stk_sp_=0;\
>>>> MPIU_AssertDeclValue(const int mpiu_chklmem_stk_sz_,n_)
>>> 
>>> You might want to revert this.  My recollection is that not all compilers
>>> are smart enough to see that mpiu_chklmem_stk_ is not used before being
>>> defined, which can lead to a *lot* of warnings throughout the MPICH
>>> codebase.
>>> 
>> 
>> Dave, I don't quite understand your concern. I removed the array
>> initialization code since MPIU_CHKLMEM_FREEALL() can only free pointers set
>> before. The number of pointers is mpiu_chklmem_stk_sp_, which is
>> initialized to 0.
> 
> Look, I don't have time (or frankly, that much interest) to run down an example compiler where this will happen, but I definitely made this change in the past because we were encountering the warning: http://git.mpich.org/mpich.git/commitdiff/70f4742
> 
> I wish I had made a better commit message at the time :-/
> 
> I have a suspicion that GCC ~4.1 with some set of (non-?)optimization flags would issue the "may be used uninitialized" warning, but I really don't recall for sure.  I don't disagree with your analysis that the value cannot actually be used uninitialized, I'm just disputing whether the compiler will issue a warning about it.
> 
> Actually, it looks like I'm not the only person who has experienced this, Toni fixed an issue like this much more recently than 2009: http://git.mpich.org/mpich.git/commitdiff/d8b8eb6
> 
>>> Also, what's the sudden interest in restoring C89 support again?  The
>>> whole world really does support enough of C99 now...  Seems like a big
>>> waste of time and an unnecessary annoyance to developers who might want to
>>> use useful C99 features (e.g., named structure initializers).
>>> 
>>> 
>> See https://trac.mpich.org/projects/mpich/ticket/2167
> 
> 
> That's not really an answer.  It's just a rehash of the endless discussion that MPICH development has had for the past 7+ years.
> 
> Open MPI and many other projects now require nominal C99 compliance for any recent release.  All the work that you did to run around and fix the "--enable-strict=c89" mode was just a huge waste of time.  The correct fix is to delete "--enable-strict=c89" and embrace the "future" of 6+ years ago.
> 
> -Dave
> 
> _______________________________________________
> To manage subscription options or unsubscribe:
> https://lists.mpich.org/mailman/listinfo/devel