Re: Pointer Bounds Checker and trailing arrays (PR68270)

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

2016-11-25 15:47 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>:
> Hi,
>
> The patch below addresses PR68270. could you please take a look?
>
> 2016-11-25  Alexander Ivchenko  <aivchenk@gmail.com>
>
>        * c-family/c.opt (flag_chkp_flexible_struct_trailing_arrays):
>        Add new option.
>        * tree-chkp.c (chkp_parse_array_and_component_ref): Forbid
>        narrowing when chkp_parse_array_and_component_ref is used and
>        the ARRAY_REF points to an array in the end of the struct.
>
>
>
> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> index 7d8a726..e45d6a2 100644
> --- a/gcc/c-family/c.opt
> +++ b/gcc/c-family/c.opt
> @@ -1166,6 +1166,11 @@ C ObjC C++ ObjC++ LTO RejectNegative Report
> Var(flag_chkp_narrow_to_innermost_ar
>  Forces Pointer Bounds Checker to use bounds of the innermost arrays in case of
>  nested static arryas access.  By default outermost array is used.
>
> +fchkp-flexible-struct-trailing-arrays
> +C ObjC C++ ObjC++ LTO RejectNegative Report
> Var(flag_chkp_flexible_struct_trailing_arrays)
> +Allow Pointer Bounds Checker to treat all trailing arrays in structures as
> +possibly flexible.

Words 'allow' and 'possibly' are confusing here. This option is about to force
checker to do something, not to give him a choice.

New option has to be documented in invoke.texi. It would also be nice to reflect
changes on GCC MPX wiki page.

We have an attribute to change compiler behavior when this option is not set.
But we have no way to make exceptions when this option is used. Should we
add one?

> +
>  fchkp-optimize
>  C ObjC C++ ObjC++ LTO Report Var(flag_chkp_optimize) Init(-1)
>  Allow Pointer Bounds Checker optimizations.  By default allowed
> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
> index 2769682..40f99c3 100644
> --- a/gcc/tree-chkp.c
> +++ b/gcc/tree-chkp.c
> @@ -3425,7 +3425,9 @@ chkp_parse_array_and_component_ref (tree node, tree *ptr,
>    if (flag_chkp_narrow_bounds
>        && !flag_chkp_narrow_to_innermost_arrray
>        && (!last_comp
> -  || chkp_may_narrow_to_field (TREE_OPERAND (last_comp, 1))))
> +  || (chkp_may_narrow_to_field (TREE_OPERAND (last_comp, 1))
> +      && !(flag_chkp_flexible_struct_trailing_arrays
> +   && array_at_struct_end_p (var)))))

This is incorrect place for fix. Consider code

struct S {
  int a;
  int b[10];
};

struct S s;
int *p = s.b;

Here you need to compute bounds for p and you want your option to take effect
but in this case you won't event reach your new check because there is no
ARRAY_REF. And even if we change it to

int *p = s.b[5];

then it still would be narrowed because s.b would still be written
into 'comp_to_narrow'
variable. Correct place for fix is in chkp_may_narrow_to_field.

Also you should consider fchkp-narrow-to-innermost-arrray option. Should it
be more powerfull or not? I think fchkp-narrow-to-innermost-arrray shouldn't
narrow to variable sized fields. BTW looks like right now bnd_variable_size
attribute is ignored by fchkp-narrow-to-innermost-arrray. This is another
problem and may be fixed in another patch though.

Also patch lacks tests for various situations (with option and without, with
ARRAY_REF and without). In case of new attribute and fix for
fchkp-narrow-to-innermost-arrray behavior additional tests are required.

--
Ilya

>      {
>        comp_to_narrow = last_comp;
>        break;