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
jos...@codesourcery.com
