[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc-patches
Subject:    Re: [PATCH] Fix PR/8268: implement compile time array subscript  checking
From:       Roger Sayle <roger () eyesopen ! com>
Date:       2007-01-13 17:35:22
Message-ID: Pine.LNX.4.44.0701130958480.27791-100000 () www ! eyesopen ! com
[Download RAW message or body]


Hi Dirk,

Given you haven't committed this patch yet, I thought I'd comment on a few
minor style issues missed by Richard's review.

On Thu, 21 Dec 2006, Dirk Mueller wrote:
> 	* tree-vrp.c (vrp_finalize): Call check_array_refs if -Warray-bounds
> 	is enabled.
> 	* tree.c (simple_cst_equal): Manually inline tree_int_cst_sgn.
> 	(check_array_refs, check_array_bounds, check_array_ref): New.

The new check_array_refs, check_array_bounds and check_array_ref are
actually in tree-vrp.c not tree.c.  Looks like the lines of your ChangeLog
entry have become scrambled.

> +  tree low_bound, up_bound = array_ref_up_bound(ref);

Space needed between "array_ref_up_bound" and "(ref);"

> +  low_bound = array_ref_low_bound(ref);

Likewise.

> +static bool array_bounds_already_done;

New global variables require a comment above them, describing their
semantics.  Particularly important is their scope.  It looks like this
variable is never reset, so you may only be reporting array bounds
problems (or some class of array bound problems) for the first function
in a file (or the first function to reach VRP).

> +/* walk_tree() callback that checks if *TP is
> +   an ARRAY_REF inside an ADDR_EXPR (in which an array
> +   subscript one outside the valid range is allowed). Call
> +   check_array_ref for each ARRAY_REF found. The location is
> +   passed in DATA.  */
> +
> +static tree
> +check_array_bounds (tree *tp, int *walk_subtree, void *data)
> +{
> +
> + tree t = *tp;
> +
> + *walk_subtree = TRUE;

Looks like you have indentation issues in this function.  The usual
GNU style indentation is two spaces, and multiples thereof.

> +  /* workaround for PR/26726. The problem here is that -fivopts
> +     sometimes shifts offsets so that arrays are accessed apparently
> +     out of bounds, but actually are not. therefore we do not warn
> +     about ARRAY_REF's inside ADDR_EXPR's anymore after the first run
> +     (which is before ivopts).
> +   */

Comments should begin with a capital letter.  The closing "*/" needs
to be on the previous line.


As a very minor style nit, though this is just a personal preference,
I find the idiom that you use in check_array_ref a little awkward and
potentially inefficient.  This sequence:

    TREE_NO_WARNING (ref) = 1;
    if (condition)
      warning (...);
    else
      TREE_NO_WARNING (ref) = 0;

initially unconditionally sets the warning issued indicator, then decides
whether or not to issue the warning and ultimately may reset it back to
zero in the common case.  Good style is to avoid writing to memory if
that's not necessary, especially for tree nodes and other datastructures
which it would be nice to keep immutable, share, store in read-only
memory or clean VM pages.  Much preferred is something like:

    if (condition)
      {
        TREE_NO_WARNING (ref) = 1;
        warning (...);
      }

where the common path now has no writes to memory.


It's true that partial dead code elimination, or lazy code motion
or other optimizations should eventually be able to clean this idiom
up for us (once implemented), but it's good practice to write
efficient/readable code to start with.

I hope this helps.

Roger
--

[prev in list] [next in list] [prev in thread] [next in thread]