[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #21 from Jakub Jelinek  ---
Author: jakub
Date: Thu Mar 26 13:19:59 2015
New Revision: 221694

URL: https://gcc.gnu.org/viewcvs?rev=221694&root=gcc&view=rev
Log:
PR tree-optimization/64715
* passes.def: Add another instance of pass_object_sizes before
ccp1.
* tree-object-size.c (pass_object_sizes::execute): In
first_pass_instance, only handle __bos (, 1) and __bos (, 3)
calls, and keep the call in the IL, as {MIN,MAX}_EXPR of the
__bos result and the computed constant.  Remove redundant
checks, obsoleted by gimple_call_builtin_p test.

* gcc.dg/builtin-object-size-15.c: New test.
* gcc.dg/pr64715-1.c: New test.
* gcc.dg/pr64715-2.c: New test.

Added:
trunk/gcc/testsuite/gcc.dg/builtin-object-size-15.c
trunk/gcc/testsuite/gcc.dg/pr64715-1.c
trunk/gcc/testsuite/gcc.dg/pr64715-2.c
Modified:
trunk/gcc/ChangeLog
trunk/gcc/passes.def
trunk/gcc/testsuite/ChangeLog
trunk/gcc/tree-object-size.c

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #20 from Jakub Jelinek  ---
Created attachment 35133
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35133&action=edit
gcc5-pr64715.patch

Untested temporary hack for GCC 5.

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #19 from Richard Biener  ---
(In reply to Jakub Jelinek from comment #14)
> Or, as discussed on IRC we can consider MEM_REFs where first operand isn't
> just
> SSA_NAME or ADDR_EXPR of a decl, but allow also ADDR_EXPR of
> handled_component_p (with base being a decl or MEM_REF with SSA_NAME first
> operand).
> 
> Or add a new tree code, like OFFSETTED_ADDR_EXPR which would be like
> ADDR_EXPR, but with integer offset address to the ADDR_EXPR.

Thinking about this some more we already have the ability to combine

 _2 = &a.b[i].c;
 _3 = _2 + 8;

by using handled-components:

  &ARRAY_REF , 8 /
__sizeof__(a.b[i].c)>

that is, we could attack this by implementing pointer arithmetic as represented
by the frontend in the way it is defined by the C standard (in terms of array
indexing).

  ptr + idx

would be lowered as

  &ARRAY_REF , idx>

thereby also avoiding the need to apply promotion of idx to sizetype to
avoid the possible overflow of the multiplication by sizeof (*ptr).

Of course it would be nice to avoid the "awkward" VIEW_CONVERT_EXPR here,
but adding a new kind of handled-component isn't exactly non-intrusive either.

Btw, there is an alternative to the array-ref/view-convert-expr form that
is less restrictive on the actual 'idx'.  We could simply use

  COMPONENT_REF <*ptr, , explicit-offset>

though the fact that COMPONENT_REFs have alias analysis effects and that
the lowered offset is measured in DECL_OFFSET_ALIGN units of the field-decl
makes that non-trivial to support as well.

That said, the alternative is to add a new handled-component, say,
OFFSET_VIEW_CONVERT_EXPR , that would have the
same effect as doing

  _ptr = &base + offset;
  MEM [_ptr, offset];

thus offset carries type-based alias information and 'type' carries
alignment info.  Type-based alias analysis would stop at
OFFSET_VIEW_CONVERT_EXPR as it stops now at VIEW_CONVERT_EXPRs.

Now we could also simply change VIEW_CONVERT_EXPR to take an (optional?)
offset parameter.

I like all of the above choices more than special-casing an address-only
variant for offsets.  All of the above enable us to avoid making
variable-indexed indirect references addressable like we have to do now
because of the restrictions of MEM_REF bases / offsets.  The ARRAY_REF
variant allows to handle non-constant pointer offsets as well while
the offsetted VIEW_CONVERT_EXPR would handle only constant offsets
(but allow non-"element"-size increments and arbitrary effective alias
sets on the memory reference).

We should already handle the ARRAY_REF  ...> form
correctly today, we just don't create it.

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #18 from rguenther at suse dot de  ---
On Tue, 24 Mar 2015, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715
> 
> --- Comment #17 from Jakub Jelinek  ---
> In
>   *r_2(D) = 0;
>   _4 = r_2(D) + 4;
>   *_4 = 1;
>   _6 = *r_2(D);
> there are no handled components, so there is no reason not to create
> &MEM_REF[r_2, 4].  But it shouldn't be hard to construct similar testcase 
> where
> there are fields and we would regress.

Yes, this is also somewhat related to PR63916.

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #17 from Jakub Jelinek  ---
In
  *r_2(D) = 0;
  _4 = r_2(D) + 4;
  *_4 = 1;
  _6 = *r_2(D);
there are no handled components, so there is no reason not to create
&MEM_REF[r_2, 4].  But it shouldn't be hard to construct similar testcase where
there are fields and we would regress.

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #16 from Richard Biener  ---
Ok, so disabling the forwprop breaks testcases like gcc.dg/tree-ssa/alias-21.c:

  :
  *r_2(D) = 0;
  _4 = r_2(D) + 4;
  *_4 = 1;
  _6 = *r_2(D);
  return _6;

SCCVN isn't able to disambiguate *r_2(D) against *_4 because the alias
machinery doesn't look at SSA defintions.  forwprop also handles things
like forwarding p + 4 into p->x.y.z or &p->x.y.z which we don't want to
disable either.  We do have both the address we propagate (with a suggested
interface change to handle &xxx + CST without building a &MEM[]) and
the tree we propagate to.  So we should be able to reject / recover
a pointer-plus in forward_propagate_addr_expr_1 as needed.

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #15 from Richard Biener  ---
(In reply to Jakub Jelinek from comment #14)
> Created attachment 35073 [details]
> WIP patch
> 
> So, on top of what you've committed, here is my unfinished attempt to
> disable for __bos undesirable transformations.  On the:
> int
> main ()
> {
>   struct A { char buf1[9]; char buf2[1]; } a;
>   char *p = a.buf1;
>   char *q = p + 1;
>   char *r = q + 4;
>   char *t = r - 1;
>   strcpy (t, str1 + 5);
>   return 0;
> }
> main of this PRs testcase, the match.pd snippet doesn't work (genmatch
> thinks ADDR_EXPR operand must be a SSA_NAME, where that is not valid
> gimple), we end up with
>   q_2 = &a.buf1 + 1;
>   t_4 = &MEM[(void *)q_2 + 3B];

that's from forwprop which runs into an ordering issue here.  IMHO the
whole POINTER_PLUS_EXPR handling in that first loop is now "premature".
Disabling it yields

  q_2 = &a.buf1 + 1;
  r_3 = &a.buf1 + 5;
  t_4 = &a.buf1 + 4;
  str1.0_6 = str1;
  _7 = str1.0_6 + 5;
  _11 = __builtin_object_size (t_4, 1);

Note that I would have expected CCP to already do all this...  it's
unfortunate that it can't consider '&a.buf1 + 1' a constant, simplifying
it along the way.  But changing that is too much work at this point.

> which would be better expressed as
>   t_4 = &a.buf1 + 4;
> and then FRE folds that into:
>   t_4 = &MEM[(void *)&a + 4B];

Yep, and it still does that.  For the same reason as CCP (make the
constant lattice work).  Same awkward fix as for CCP:

Index: gcc/tree-ssa-sccvn.c
===
--- gcc/tree-ssa-sccvn.c(revision 221624)
+++ gcc/tree-ssa-sccvn.c(working copy)
@@ -85,6 +85,7 @@ along with GCC; see the file COPYING3.
 #include "ipa-ref.h"
 #include "plugin-api.h"
 #include "cgraph.h"
+#include "tree-pass.h"

 /* This algorithm is based on the SCC algorithm presented by Keith
Cooper and L. Taylor Simpson in "SCC-Based Value numbering"
@@ -879,7 +880,7 @@ copy_reference_ops_from_ref (tree ref, v
   trick here).  */
&& (TREE_CODE (orig) != ADDR_EXPR
|| off != 0
-   || cfun->after_inlining))
+   || (cfun->curr_properties & PROP_gimple_ebos)))
  temp.off = off.to_shwi ();
  }
  }
@@ -3374,7 +3375,9 @@ simplify_binary_expression (gimple stmt)
   if (code == POINTER_PLUS_EXPR
   && tree_fits_uhwi_p (op1)
   && TREE_CODE (op0) == ADDR_EXPR
-  && is_gimple_min_invariant (op0))
+  && is_gimple_min_invariant (op0)
+  && (!handled_component_p (TREE_OPERAND (op0, 0))
+ || (cfun->curr_properties & PROP_gimple_ebos)))
 return build_invariant_address (TREE_TYPE (op0),
TREE_OPERAND (op0, 0),
tree_to_uhwi (op1));

> so if we went this way, we'd need to change genmatch to handle ADDR_EXPRs
> specially, and FRE to avoid forwarding into &MEM if that would lose info.

I think trying to disable the pointer-plus-expr forwprop is more reasonable.
I'll see what we get as fallout from that.

So maybe we can "propagate" PROP_gimple_ebos up the cgraph in local-pure-const
to at least not pessimize functions that do not have __bos and won't get it
either via inlining (unfortunately local_pure_const is only run after
early opts, so it isn't really the suitable place to do this - cgraph build
maybe).

> Or, as discussed on IRC we can consider MEM_REFs where first operand isn't
> just
> SSA_NAME or ADDR_EXPR of a decl, but allow also ADDR_EXPR of
> handled_component_p (with base being a decl or MEM_REF with SSA_NAME first
> operand).
> 
> Or add a new tree code, like OFFSETTED_ADDR_EXPR which would be like
> ADDR_EXPR, but with integer offset address to the ADDR_EXPR.

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #14 from Jakub Jelinek  ---
Created attachment 35073
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35073&action=edit
WIP patch

So, on top of what you've committed, here is my unfinished attempt to disable
for __bos undesirable transformations.  On the:
int
main ()
{
  struct A { char buf1[9]; char buf2[1]; } a;
  char *p = a.buf1;
  char *q = p + 1;
  char *r = q + 4;
  char *t = r - 1;
  strcpy (t, str1 + 5);
  return 0;
}
main of this PRs testcase, the match.pd snippet doesn't work (genmatch thinks
ADDR_EXPR operand must be a SSA_NAME, where that is not valid gimple), we end
up with
  q_2 = &a.buf1 + 1;
  t_4 = &MEM[(void *)q_2 + 3B];
which would be better expressed as
  t_4 = &a.buf1 + 4;
and then FRE folds that into:
  t_4 = &MEM[(void *)&a + 4B];
so if we went this way, we'd need to change genmatch to handle ADDR_EXPRs
specially, and FRE to avoid forwarding into &MEM if that would lose info.

Or, as discussed on IRC we can consider MEM_REFs where first operand isn't just
SSA_NAME or ADDR_EXPR of a decl, but allow also ADDR_EXPR of
handled_component_p (with base being a decl or MEM_REF with SSA_NAME first
operand).

Or add a new tree code, like OFFSETTED_ADDR_EXPR which would be like ADDR_EXPR,
but with integer offset address to the ADDR_EXPR.

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #13 from Richard Biener  ---
Author: rguenth
Date: Fri Mar 20 12:39:32 2015
New Revision: 221532

URL: https://gcc.gnu.org/viewcvs?rev=221532&root=gcc&view=rev
Log:
2015-03-20  Richard Biener  

PR middle-end/64715
* tree-chrec.c (chrec_fold_poly_cst): Use useless_type_conversion_p
for type comparison and gcc_checking_assert.
(chrec_fold_plus_poly_poly): Likewise.
(chrec_fold_multiply_poly_poly): Likewise.
(chrec_convert_1): Likewise.
* gimplify.c (gimplify_expr): Remove premature folding of
&X + CST to &MEM[&X, CST].

* gcc.dg/pr15347.c: Use -O.
* c-c++-common/pr19807-1.c: Likewise.

Modified:
trunk/gcc/ChangeLog
trunk/gcc/gimplify.c
trunk/gcc/testsuite/ChangeLog
trunk/gcc/testsuite/c-c++-common/pr19807-1.c
trunk/gcc/testsuite/gcc.dg/pr15347.c
trunk/gcc/tree-chrec.c

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #12 from Jakub Jelinek  ---
For the early objsz pass, I'm afraid it can have security implications.
Artificial testcase:
extern char *strcpy (char *__restrict __dest, const char *__restrict __src)
__attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__nonnull__ (1, 2)));
extern __inline __attribute__ ((__always_inline__)) __attribute__
((__gnu_inline__)) __attribute__ ((__artificial__)) char *
__attribute__ ((__nothrow__ , __leaf__)) strcpy (char *__restrict __dest, const
char *__restrict __src)
{
  return __builtin___strcpy_chk (__dest, __src, __builtin_object_size (__dest,
2 > 1));
}

const char *str1 = "JIHGFEDCBA";

int
main ()
{
  struct A { char buf1[9]; char buf2[1]; } a;
  char *p = a.buf1;
  char *q = p + 1;
  char *r = q + 3;
  char *t = r;
  if (r != &a.buf1[4])
t = (char *) &a;
  strcpy (t, str1 + 5);
  return 0;
}

with the early objsz this will use 10 for __bos rather than 5, so will not
detect the buffer overflow.
So, if we want to go forward with that, perhaps:
1) the first instance should (also for performance reasons) only try to deal
with __bos calls where the second argument is 1 or 3, and leave 0 and 2 alone
for the second objsz pass
2) maybe instead of folding the __bos call into constant, it should turn it
into
MIN_EXPR <__bos, XX> where XX would be the value computed by the pass (for
__bos (, 3) MAX_EXPR).  That way, we'd use the smaller value of the two passes.

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #11 from Richard Biener  ---
Created attachment 35064
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35064&action=edit
patch

I am testing the attached, testcases to be ameded from the dups.

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #10 from Richard Biener  ---
I do think that the gimplifiers "folding" is premature.  All propagators know
to apply the trick internally.  This premature folding is probably to avoid
regressions with removing the invalid maybe_fold_* stuff.

I'll see whether removing it is safe.  Of course it won't fix the testcase
on its own - CCP happily applies the same trick to forward the "constant"
address to the builtin call:

--- t.c.028t.copyrename12015-03-19 12:52:53.875179949 +0100
+++ t.c.029t.ccp1   2015-03-19 12:52:53.876179961 +0100
@@ -25,21 +25,15 @@
   struct A a;
   const char * str1.0_2;
   const char * _3;
-  char * _4;
-  int _7;
-  long unsigned int _8;
   char * _9;

   :
   str1.0_2 = str1;
   _3 = str1.0_2 + 5;
-  _4 = &a.buf1 + 4;
-  _8 = __builtin_object_size (_4, 1);
-  _9 = __builtin___strcpy_chk (_4, _3, _8);
+  _9 = __builtin___strcpy_chk (&MEM[(void *)&a + 4B], _3, 6);

also folding the object-size call at the same time (to the bogus value
because of passing it &MEM[(void *)&a, +4B] as well).

I think it is desirable to fold the object-size call here and we can certainly
special-case this particular case (looking at the def of _4 instead of its
lattice value).  But not sure if that's a good enough fix for the general
issue.

After "fixing" the gimplifier we have to make sure neither CCP1, CCP2 or FRE1
perform this trick (forwprop would also wreck things for slightly more
complicated cases).

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #9 from Jakub Jelinek  ---
For the option of not folding this to MEM_REFs before objsz pass, I'd note this
could be just about the &MEM_REF cases, if there is an actual memory access, so
we aren't taking the address of the MEM_REF, then we can use MEM_REF as before.
Similarly if we are folding &a + 4 into &MEM_REF[&a + 4] it would be fine, just
we'd avoid doing that early for &a.field + 4.

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

--- Comment #8 from Jakub Jelinek  ---
So, given that you promoted this to P1, what are we going to do with this?

This indeed started with r214941, and from objsz POV, in *.original, while it
changed from:
-  strcpy (&a.buf1[4], str1 + 5);
+  strcpy ((char *) &a.buf1 + 4, str1 + 5);
it is still reasonable, no information is lost.
The information is lost during gimplification, where we gimplify it as:
-  strcpy (&a.buf1[4], D.1762);
+  strcpy (&MEM[(void *)&a + 4B], D.1762);
and there we already lost the info whether it was
strcpy (&a.buf1 + 4, ...) or strcpy (&a, ...), where we really don't want to
treat those two the same.
So, either we should avoid folding this to a MEM_REF before objsz pass, or
allow MEM_REF operand to be (perhaps just before objsz pass) not just SSA_NAME
or ADDR_EXPR of a DECL, but also ADDR_EXPR of handled_component_p and only
lower it later (lose the information on where it pointed to).
Or add optional third argument to MEM_REF that would contain say the
COMPONENT_REF (if any) with PLACEHOLDER_EXPR inside for the type of the DECL in
ADDR_EXPR in the first operand.
Something else?

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

Richard Biener  changed:

   What|Removed |Added

   Priority|P3  |P1

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

Richard Biener  changed:

   What|Removed |Added

 CC||trippels at gcc dot gnu.org

--- Comment #7 from Richard Biener  ---
*** Bug 65346 has been marked as a duplicate of this bug. ***

[Bug tree-optimization/64715] [5 Regression] __builtin_object_size (..., 1) fails to locate subobject

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64715

Richard Biener  changed:

   What|Removed |Added

 CC||nszabolcs at gmail dot com

--- Comment #6 from Richard Biener  ---
*** Bug 64964 has been marked as a duplicate of this bug. ***