RE: [patch] cilkplus array notation for C (clean, independent patchset, take 1)

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Fri, 22 Mar 2013, Iyer, Balaji V wrote:

> > Why the random check for a NULL argument?  If a NULL argument is valid
> > (meaning that it makes the code cleaner to allow such arguments rather than
> > making sure the function isn't called with them), this should be documented in
> > the comment above the function; otherwise, if such an argument isn't valid,
> > there is no need to check for it.
> 
> I always tend to check for a null pointer before I access the fields in 
> the structure. In this case it is unnecessary. In some cases (e.g. 
> find_rank) there is a good chance a null pointer will be passed into the 
> function and we need to check that and reject those.

"always tend to check for a null pointer" is not good practice.  
Sometimes such checks are good, and sometimes they are bad.

Each function should have a defined interface that makes clear the 
semantics and valid values of its operands - including whether NULL is 
valid, and, if so, what the semantics of a NULL argument are.  These 
semantics should be clear from the comment above the function.

What the semantics should be in each case depends on a global view of the 
relevant code.  Based on such a global view you need to determine the 
interfaces resulting in the cleanest code - in some cases there may be a 
special case, the absence of a datastructure as opposed to its presence, 
that is naturally represented by a NULL argument, but in that case it's a 
judgement to be made for the code as a whole whether this case should be 
checked for in the caller or the callee (or maybe further up or down the 
call stack in some cases).  Where erroneous input to the compiler is the 
cause of NULL arguments, passing error_mark_node may be better in some 
cases if it means fewer special-case checks.

If in a particular case it shouldn't be possible for a NULL pointer to be 
present in a particular place (function argument, variable, etc.), for any 
input to the compiler, it is always wrong to have a check for a NULL 
pointer that silently continues compilation.  Instead, in such cases where 
NULL is invalid you have two options:

* No check, if NULL is present and dereferenced the compiler will crash.

* A check inside a gcc_assert, to verify this precondition of the function 
and give a slightly friendlier internal compiler error than a segfault.  
(Or any other similar option resulting in an abort with an ICE.)

You need to judge in each case which of the two is appropriate, just as 
with any other case where there is some precondition for a function that 
you might or might not decide to verify with an assertion, depending on 
factors such as:

* how complicated such a check is to write;

* how expensive the check is when the compiler runs;

* how likely the check is to find bugs in the compiler;

* how helpful the check's presence is to people reading the source code 
and trying to understand what the data may look like at a particular 
point.

Note also that the fact that you observe a NULL pointer being passed to a 
function and causing a crash there does not of itself mean that adding a 
check for NULL is a valid fix.  It's only a valid fix if analysis of the 
issue shows that it was indeed correct for NULL to be passed to that 
function for the given input to the compiler.  Sometimes it may turn out 
that NULL shouldn't have been passed to that function at all for that 
input and that the proper fix is elsewhere in the compiler.

> > > +  if (TREE_CODE (fn) == CALL_EXPR)
> > > +    {
> > > +      fn_arg = CALL_EXPR_ARG (fn, 0);
> > > +      if (really_constant_p (fn_arg))
> > 
> > I don't think really_constant_p is what's wanted;
> > <http://software.intel.com/sites/default/files/m/4/e/7/3/1/40297-
> > Intel_Cilk_plus_lang_spec_2.htm>
> > says "The argument shall be an integer constant expression.", and such
> > expressions always appear in the C front end as INTEGER_CST.  So you can just
> > check for INTEGER_CST.
> 
> What about C++? This function is shared by both C and C++.

This patch seems just to be for C.  really_constant_p is wrong for C, 
since INTEGER_CSTs wrapped with conversions can be used to represent 
values folded to constants that aren't integer constant expressions.  
Maybe you need a conditional on the language, but I don't know what the 
right check for C++ might be in that case.

> > > +void
> > > +find_rank (tree array, bool ignore_builtin_fn, size_t *rank) {
> > > +  tree ii_tree;
> > > +  size_t current_rank = 0, ii = 0;
> > > +  an_reduce_type dummy_type = REDUCE_UNKNOWN;
> > > +  if (!array)
> > > +    return;
> > 
> > As before, avoid random checks for NULL parameters unless there is an actual
> > reason to allow them and the comments document that they are allowed and
> > what the semantics are in that case.  In general, explain what ARRAY is - an
> > expression?
> 
> This check is necessary. Find rank can get a NULL pointer and that must 
> be caught and rejected.

Then make sure the comment explaining the semantics of ARRAY includes the 
case of a NULL pointer, explaining what the semantics of such a pointer 
are and what the semantics of the function are in that case.

> > What's NODE?  My first guess would have been an expression, but if a TREE_LIST
> > is possible that's clearly not the answer, so explain in the comment above the
> > function what NODE is.  (If a TREE_LIST is being used within expressions to store
> > something specific to array notation, don't do so - TREE_LIST is deprecated,
> > existing uses should be phased out in favour of more specific and less memory-
> > hungry datastructures and new uses should not be added.)
> 
> FIXED! What is replacing tree-list? I have used tree-list in my later 
> patches and in my Cilk Plus branch.

TREE_LIST is replaced by whatever datastructure is appropriate in each 
case.  When possible, that structure has a static type other than "tree", 
specific to the particular case in question; it may be an ad hoc type in 
GCC, or use GCC-specific infrastructure such as vec.h, or standard C++ 
library features where there aren't problems with interaction with garbage 
collection.  If in doubt about how to represent something without using 
TREE_LIST, post details of the particular datastructure and how it is used 
to the gcc@ list.

TREE_LISTs have TYPE, CHAIN, VALUE and PURPOSE pointers as well as a 
TREE_CODE.  It's rare for all of these to be used, so they are generally 
particularly memory-inefficient.

-- 
Joseph S. Myers
joseph@codesourcery.com