Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

On 9/14/18, Martin Sebor wrote:
> As I said above, this happens during the dom walk in the ccp
> pass:
> 
>   substitute_and_fold_dom_walker walker (CDI_DOMINATORS, this);
>   walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));
> 
> The warning is issued during the walker.walk() call as
> strncpy is being folded into memcpy.  The prior assignments are
> only propagated later, when the next statement after the strncpy
> call is reached.  It happens in
> substitute_and_fold_dom_walker::before_dom_children(). So during
> the strncpy folding we see the next statement as:
> 
>   MEM[(struct S *)_1].a[n_7] = 0;
> 
> After the strncpy call is transformed to memcpy, the assignment
> above is transformed to
> 
>   MEM[(struct S *)_8].a[3] = 0;
> 
> 
>>     If they're only discovered as copies within the pass where you're trying
>>     to issue the diagnostic, then you'd want to see if the pass has any
>>     internal structures that tell you about equivalences.
> 
> 
> I don't know if this is possible.  I don't see any APIs in
> tree-ssa-propagate.h that would let me query the internal data
> somehow to find out during folding (when the warning is issued).


Well,

if I see this right, the CCP is doing tree transformations
while from the folding of strncpy the predicate maybe_diag_stxncpy_trunc
is called, and sees inconsistent information, in the tree,
and therefore it issues a warning.

I understand that walking the references is fragile at least
in this state.

But why not just prevent warnings when this is called from CCP?


Like this?

Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr84561.diff --]
[-- Type: text/x-patch; name="patch-pr84561.diff", Size: 2414 bytes --]

gcc:
2018-09-15  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR tree-optimization/84561
	* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Bail out if called
	from CCP.

testsuite:
2018-09-15  Martin Sebor  <msebor@redhat.com>

	PR tree-optimization/84561
	* gcc.dg/Wstringop-truncation-6.c: New test.

diff -Npur gcc/tree-ssa-strlen.c gcc/tree-ssa-strlen.c
--- gcc/tree-ssa-strlen.c	2018-09-14 13:12:10.000000000 +0200
+++ gcc/tree-ssa-strlen.c	2018-09-15 08:04:57.011142267 +0200
@@ -1846,11 +1846,13 @@ is_strlen_related_p (tree src, tree len)
 bool
 maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi, tree src, tree cnt)
 {
+  wide_int cntrange[2];
   gimple *stmt = gsi_stmt (gsi);
   if (gimple_no_warning_p (stmt))
     return false;
 
-  wide_int cntrange[2];
+  if (current_pass && !strcmp (current_pass->name, "ccp"))
+    return false;
 
   if (TREE_CODE (cnt) == INTEGER_CST)
     cntrange[0] = cntrange[1] = wi::to_wide (cnt);
diff -Npur gcc/testsuite/gcc.dg/Wstringop-truncation-6.c gcc/testsuite/gcc.dg/Wstringop-truncation-6.c
--- gcc/testsuite/gcc.dg/Wstringop-truncation-6.c	1970-01-01 01:00:00.000000000 +0100
+++ gcc/testsuite/gcc.dg/Wstringop-truncation-6.c	2018-09-15 08:02:35.597198845 +0200
@@ -0,0 +1,59 @@
+/* PR tree-optimization/84561 - -Wstringop-truncation with -O2 depends
+   on strncpy's size type
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+enum { N = 3 };
+
+struct S
+{
+  char a[N + 1];
+};
+
+void set (struct S *ps, const char* s, size_t n)
+{
+  if (n > N)
+    n = N;
+
+  __builtin_strncpy (ps->a, s, n);   /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+  ps->a[n] = 0;
+}
+
+struct A
+{
+  struct S str;
+};
+
+void setStringSize_t (struct A *pa, const char *s, size_t n)
+{
+  set (&pa->str, s, n);
+}
+
+void setStringUnsignedInt (struct A *pa, const char* s, unsigned int n)
+{
+  set (&pa->str, s, n);
+}
+
+struct B
+{
+  struct A a;
+};
+
+struct A* getA (struct B *pb)
+{
+  return &pb->a;
+}
+
+void f (struct A *pa)
+{
+  setStringUnsignedInt (pa, "123", N); // No warning here.
+  setStringSize_t (pa, "123", N);      // No warning here.
+}
+
+void g (struct B *pb)
+{
+  setStringUnsignedInt (getA (pb), "123", N);  // Unexpected warning here.
+  setStringSize_t (getA (pb), "123", N);       // No warning here.
+}

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Jeff Law]

On 9/15/18 2:14 AM, Bernd Edlinger wrote:
> On 9/14/18, Martin Sebor wrote:
>> As I said above, this happens during the dom walk in the ccp
>> pass:
>>
>>   substitute_and_fold_dom_walker walker (CDI_DOMINATORS, this);
>>   walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>>
>> The warning is issued during the walker.walk() call as
>> strncpy is being folded into memcpy.  The prior assignments are
>> only propagated later, when the next statement after the strncpy
>> call is reached.  It happens in
>> substitute_and_fold_dom_walker::before_dom_children(). So during
>> the strncpy folding we see the next statement as:
>>
>>   MEM[(struct S *)_1].a[n_7] = 0;
>>
>> After the strncpy call is transformed to memcpy, the assignment
>> above is transformed to
>>
>>   MEM[(struct S *)_8].a[3] = 0;
>>
>>
>>>     If they're only discovered as copies within the pass where you're trying
>>>     to issue the diagnostic, then you'd want to see if the pass has any
>>>     internal structures that tell you about equivalences.
>>
>>
>> I don't know if this is possible.  I don't see any APIs in
>> tree-ssa-propagate.h that would let me query the internal data
>> somehow to find out during folding (when the warning is issued).
> 
> 
> Well,
> 
> if I see this right, the CCP is doing tree transformations
> while from the folding of strncpy the predicate maybe_diag_stxncpy_trunc
> is called, and sees inconsistent information, in the tree,
> and therefore it issues a warning.
> 
> I understand that walking the references is fragile at least
> in this state.
> 
> But why not just prevent warnings when this is called from CCP?
> 
> 
> Like this?
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
No.  That's just hacking around the real problem.

jeff

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Richard Biener]

On September 17, 2018 7:24:16 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>On 9/15/18 2:14 AM, Bernd Edlinger wrote:
>> On 9/14/18, Martin Sebor wrote:
>>> As I said above, this happens during the dom walk in the ccp
>>> pass:
>>>
>>>   substitute_and_fold_dom_walker walker (CDI_DOMINATORS, this);
>>>   walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>>>
>>> The warning is issued during the walker.walk() call as
>>> strncpy is being folded into memcpy.  The prior assignments are
>>> only propagated later, when the next statement after the strncpy
>>> call is reached.  It happens in
>>> substitute_and_fold_dom_walker::before_dom_children(). So during
>>> the strncpy folding we see the next statement as:
>>>
>>>   MEM[(struct S *)_1].a[n_7] = 0;
>>>
>>> After the strncpy call is transformed to memcpy, the assignment
>>> above is transformed to
>>>
>>>   MEM[(struct S *)_8].a[3] = 0;
>>>
>>>
>>>>     If they're only discovered as copies within the pass where
>you're trying
>>>>     to issue the diagnostic, then you'd want to see if the pass has
>any
>>>>     internal structures that tell you about equivalences.
>>>
>>>
>>> I don't know if this is possible.  I don't see any APIs in
>>> tree-ssa-propagate.h that would let me query the internal data
>>> somehow to find out during folding (when the warning is issued).
>> 
>> 
>> Well,
>> 
>> if I see this right, the CCP is doing tree transformations
>> while from the folding of strncpy the predicate
>maybe_diag_stxncpy_trunc
>> is called, and sees inconsistent information, in the tree,
>> and therefore it issues a warning.
>> 
>> I understand that walking the references is fragile at least
>> in this state.
>> 
>> But why not just prevent warnings when this is called from CCP?
>> 
>> 
>> Like this?
>> 
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>No.  That's just hacking around the real problem.

The real problem is emitting diagnostics from folding code. 

Richard. 

>
>jeff

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Bernd Edlinger]

On 09/17/18 19:35, Richard Biener wrote:
> On September 17, 2018 7:24:16 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>> On 9/15/18 2:14 AM, Bernd Edlinger wrote:
>>> On 9/14/18, Martin Sebor wrote:
>>>> As I said above, this happens during the dom walk in the ccp
>>>> pass:
>>>>
>>>>    substitute_and_fold_dom_walker walker (CDI_DOMINATORS, this);
>>>>    walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>>>>
>>>> The warning is issued during the walker.walk() call as
>>>> strncpy is being folded into memcpy.  The prior assignments are
>>>> only propagated later, when the next statement after the strncpy
>>>> call is reached.  It happens in
>>>> substitute_and_fold_dom_walker::before_dom_children(). So during
>>>> the strncpy folding we see the next statement as:
>>>>
>>>>    MEM[(struct S *)_1].a[n_7] = 0;
>>>>
>>>> After the strncpy call is transformed to memcpy, the assignment
>>>> above is transformed to
>>>>
>>>>    MEM[(struct S *)_8].a[3] = 0;
>>>>
>>>>
>>>>>      If they're only discovered as copies within the pass where
>> you're trying
>>>>>      to issue the diagnostic, then you'd want to see if the pass has
>> any
>>>>>      internal structures that tell you about equivalences.
>>>>
>>>>
>>>> I don't know if this is possible.  I don't see any APIs in
>>>> tree-ssa-propagate.h that would let me query the internal data
>>>> somehow to find out during folding (when the warning is issued).
>>>
>>>
>>> Well,
>>>
>>> if I see this right, the CCP is doing tree transformations
>>> while from the folding of strncpy the predicate
>> maybe_diag_stxncpy_trunc
>>> is called, and sees inconsistent information, in the tree,
>>> and therefore it issues a warning.
>>>
>>> I understand that walking the references is fragile at least
>>> in this state.
>>>
>>> But why not just prevent warnings when this is called from CCP?
>>>
>>>
>>> Like this?
>>>
>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>> Is it OK for trunk?
>> No.  That's just hacking around the real problem.
> 
> The real problem is emitting diagnostics from folding code.
> 

Yes, I am also very concerned about that.

So if this is a design bug, then it is probably impossible
to fix at the implementation, and we should fix the design.

It is unfortunate, that this means regressions on the existing
(and probably also proposed, future) test cases.


Bernd.

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Martin Sebor]

On 09/17/2018 11:35 AM, Richard Biener wrote:
> On September 17, 2018 7:24:16 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>> On 9/15/18 2:14 AM, Bernd Edlinger wrote:
>>> On 9/14/18, Martin Sebor wrote:
>>>> As I said above, this happens during the dom walk in the ccp
>>>> pass:
>>>>
>>>>   substitute_and_fold_dom_walker walker (CDI_DOMINATORS, this);
>>>>   walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>>>>
>>>> The warning is issued during the walker.walk() call as
>>>> strncpy is being folded into memcpy.  The prior assignments are
>>>> only propagated later, when the next statement after the strncpy
>>>> call is reached.  It happens in
>>>> substitute_and_fold_dom_walker::before_dom_children(). So during
>>>> the strncpy folding we see the next statement as:
>>>>
>>>>   MEM[(struct S *)_1].a[n_7] = 0;
>>>>
>>>> After the strncpy call is transformed to memcpy, the assignment
>>>> above is transformed to
>>>>
>>>>   MEM[(struct S *)_8].a[3] = 0;
>>>>
>>>>
>>>>>     If they're only discovered as copies within the pass where
>> you're trying
>>>>>     to issue the diagnostic, then you'd want to see if the pass has
>> any
>>>>>     internal structures that tell you about equivalences.
>>>>
>>>>
>>>> I don't know if this is possible.  I don't see any APIs in
>>>> tree-ssa-propagate.h that would let me query the internal data
>>>> somehow to find out during folding (when the warning is issued).
>>>
>>>
>>> Well,
>>>
>>> if I see this right, the CCP is doing tree transformations
>>> while from the folding of strncpy the predicate
>> maybe_diag_stxncpy_trunc
>>> is called, and sees inconsistent information, in the tree,
>>> and therefore it issues a warning.
>>>
>>> I understand that walking the references is fragile at least
>>> in this state.
>>>
>>> But why not just prevent warnings when this is called from CCP?
>>>
>>>
>>> Like this?
>>>
>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>> Is it OK for trunk?
>> No.  That's just hacking around the real problem.
>
> The real problem is emitting diagnostics from folding code.

Strncpy is a particularly dangerous function that's often
misunderstood and misused.  IMO, it would be a worthwhile
tradeoff to move the strncpy to memcpy transformation to
the strlen pass where these bugs could be detected more
reliably, and with fewer false positives.  I would not
expect it to have a noticeable impact on code efficiency.

I'd be happy to modify the patch to do that if you find it
acceptable.

Martin

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Jeff Law]

On 9/17/18 3:15 PM, Martin Sebor wrote:
> On 09/17/2018 11:35 AM, Richard Biener wrote:
>> On September 17, 2018 7:24:16 PM GMT+02:00, Jeff Law <law@redhat.com>
>> wrote:
>>> On 9/15/18 2:14 AM, Bernd Edlinger wrote:
>>>> On 9/14/18, Martin Sebor wrote:
>>>>> As I said above, this happens during the dom walk in the ccp
>>>>> pass:
>>>>>
>>>>>   substitute_and_fold_dom_walker walker (CDI_DOMINATORS, this);
>>>>>   walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>>>>>
>>>>> The warning is issued during the walker.walk() call as
>>>>> strncpy is being folded into memcpy.  The prior assignments are
>>>>> only propagated later, when the next statement after the strncpy
>>>>> call is reached.  It happens in
>>>>> substitute_and_fold_dom_walker::before_dom_children(). So during
>>>>> the strncpy folding we see the next statement as:
>>>>>
>>>>>   MEM[(struct S *)_1].a[n_7] = 0;
>>>>>
>>>>> After the strncpy call is transformed to memcpy, the assignment
>>>>> above is transformed to
>>>>>
>>>>>   MEM[(struct S *)_8].a[3] = 0;
>>>>>
>>>>>
>>>>>>     If they're only discovered as copies within the pass where
>>> you're trying
>>>>>>     to issue the diagnostic, then you'd want to see if the pass has
>>> any
>>>>>>     internal structures that tell you about equivalences.
>>>>>
>>>>>
>>>>> I don't know if this is possible.  I don't see any APIs in
>>>>> tree-ssa-propagate.h that would let me query the internal data
>>>>> somehow to find out during folding (when the warning is issued).
>>>>
>>>>
>>>> Well,
>>>>
>>>> if I see this right, the CCP is doing tree transformations
>>>> while from the folding of strncpy the predicate
>>> maybe_diag_stxncpy_trunc
>>>> is called, and sees inconsistent information, in the tree,
>>>> and therefore it issues a warning.
>>>>
>>>> I understand that walking the references is fragile at least
>>>> in this state.
>>>>
>>>> But why not just prevent warnings when this is called from CCP?
>>>>
>>>>
>>>> Like this?
>>>>
>>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>>> Is it OK for trunk?
>>> No.  That's just hacking around the real problem.
>>
>> The real problem is emitting diagnostics from folding code.
> 
> Strncpy is a particularly dangerous function that's often
> misunderstood and misused.  IMO, it would be a worthwhile
> tradeoff to move the strncpy to memcpy transformation to
> the strlen pass where these bugs could be detected more
> reliably, and with fewer false positives.  I would not
> expect it to have a noticeable impact on code efficiency.
> 
> I'd be happy to modify the patch to do that if you find it
> acceptable.
That natural question is what is the immediate (ie testsuite) fallout?

Jeff

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Martin Sebor]

On 09/17/2018 05:13 PM, Jeff Law wrote:
> On 9/17/18 3:15 PM, Martin Sebor wrote:
>> On 09/17/2018 11:35 AM, Richard Biener wrote:
>>> On September 17, 2018 7:24:16 PM GMT+02:00, Jeff Law <law@redhat.com>
>>> wrote:
>>>> On 9/15/18 2:14 AM, Bernd Edlinger wrote:
>>>>> On 9/14/18, Martin Sebor wrote:
>>>>>> As I said above, this happens during the dom walk in the ccp
>>>>>> pass:
>>>>>>
>>>>>>   substitute_and_fold_dom_walker walker (CDI_DOMINATORS, this);
>>>>>>   walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>>>>>>
>>>>>> The warning is issued during the walker.walk() call as
>>>>>> strncpy is being folded into memcpy.  The prior assignments are
>>>>>> only propagated later, when the next statement after the strncpy
>>>>>> call is reached.  It happens in
>>>>>> substitute_and_fold_dom_walker::before_dom_children(). So during
>>>>>> the strncpy folding we see the next statement as:
>>>>>>
>>>>>>   MEM[(struct S *)_1].a[n_7] = 0;
>>>>>>
>>>>>> After the strncpy call is transformed to memcpy, the assignment
>>>>>> above is transformed to
>>>>>>
>>>>>>   MEM[(struct S *)_8].a[3] = 0;
>>>>>>
>>>>>>
>>>>>>>     If they're only discovered as copies within the pass where
>>>> you're trying
>>>>>>>     to issue the diagnostic, then you'd want to see if the pass has
>>>> any
>>>>>>>     internal structures that tell you about equivalences.
>>>>>>
>>>>>>
>>>>>> I don't know if this is possible.  I don't see any APIs in
>>>>>> tree-ssa-propagate.h that would let me query the internal data
>>>>>> somehow to find out during folding (when the warning is issued).
>>>>>
>>>>>
>>>>> Well,
>>>>>
>>>>> if I see this right, the CCP is doing tree transformations
>>>>> while from the folding of strncpy the predicate
>>>> maybe_diag_stxncpy_trunc
>>>>> is called, and sees inconsistent information, in the tree,
>>>>> and therefore it issues a warning.
>>>>>
>>>>> I understand that walking the references is fragile at least
>>>>> in this state.
>>>>>
>>>>> But why not just prevent warnings when this is called from CCP?
>>>>>
>>>>>
>>>>> Like this?
>>>>>
>>>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>>>> Is it OK for trunk?
>>>> No.  That's just hacking around the real problem.
>>>
>>> The real problem is emitting diagnostics from folding code.
>>
>> Strncpy is a particularly dangerous function that's often
>> misunderstood and misused.  IMO, it would be a worthwhile
>> tradeoff to move the strncpy to memcpy transformation to
>> the strlen pass where these bugs could be detected more
>> reliably, and with fewer false positives.  I would not
>> expect it to have a noticeable impact on code efficiency.
>>
>> I'd be happy to modify the patch to do that if you find it
>> acceptable.
> That natural question is what is the immediate (ie testsuite) fallout?

I disabled strncpy folding altogether as a quick and dirty
experiment.  The fallout is minimal: there are just a few
regressions in warning tests (Wstringop-truncation.c and
Wrestrict.c), and a couple more in
gcc.c-torture/execute/builtins/strncpy{-chk}.c.  The latter
two would clear up with the folding moved to tree-ssa-strlen
as I'm guessing would the warnings.

Martin

[PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

The attached patch adds code to work harder to determine whether
the destination of an assignment involving MEM_REF is the same
as the destination of a prior strncpy call.  The included test
case demonstrates when this situation comes up.  During ccp,
dstbase and lhsbase returned by get_addr_base_and_unit_offset()
end up looking like this:

   _8 = &pb_3(D)->a;
   _9 = _8;
   _1 = _9;
   strncpy (MEM_REF (&pb_3(D)->a), ...);
   MEM[(struct S *)_1].a[n_7] = 0;

so the loops follow the simple assignments until we get at
the ADDR_EXPR assigned to _8 which is the same as the strncpy
destination.

Tested on x86_64-linux.

Martin

[-- Attachment #2: gcc-84561.diff --]
[-- Type: text/x-patch, Size: 3278 bytes --]

PR tree-optimization/84561 - -Wstringop-truncation with -O2 depends on strncpy's size type

gcc/ChangeLog:

	PR tree-optimization/84561
	* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Look harder for
	MEM_REF operand equality.

gcc/testsuite/ChangeLog:

	PR tree-optimization/84561
	* gcc.dg/Wstringop-truncation-6.c: New test.

Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 263965)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -1978,11 +1978,43 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
 
       poly_int64 lhsoff;
       tree lhsbase = get_addr_base_and_unit_offset (lhs, &lhsoff);
-      if (lhsbase
-	  && dstbase
-	  && known_eq (dstoff, lhsoff)
-	  && operand_equal_p (dstbase, lhsbase, 0))
+      bool eqloff = lhsbase && dstbase && known_eq (dstoff, lhsoff);
+
+      if (eqloff && operand_equal_p (dstbase, lhsbase, 0))
 	return false;
+
+      if (eqloff
+	  && TREE_CODE (dstbase) == MEM_REF
+	  && TREE_CODE (lhsbase) == MEM_REF
+	  && tree_int_cst_equal (TREE_OPERAND (dstbase, 1),
+				 TREE_OPERAND (lhsbase, 1)))
+	{
+	  /* For MEM_REFs with the same offset follow the chain of
+	     SSA_NAME assignments to their source and compare those
+	     for equality.  */
+	  dstbase = TREE_OPERAND (dstbase, 0);
+	  while (TREE_CODE (dstbase) == SSA_NAME)
+	    {
+	      gimple *def = SSA_NAME_DEF_STMT (dstbase);
+	      if (gimple_assign_single_p (def))
+		dstbase = gimple_assign_rhs1 (def);
+	      else
+		break;
+	    }
+
+	  lhsbase = TREE_OPERAND (lhsbase, 0);
+	  while (TREE_CODE (lhsbase) == SSA_NAME)
+	    {
+	      gimple *def = SSA_NAME_DEF_STMT (lhsbase);
+	      if (gimple_assign_single_p (def))
+		lhsbase = gimple_assign_rhs1 (def);
+	      else
+		break;
+	    }
+
+	  if (operand_equal_p (dstbase, lhsbase, 0))
+	    return false;
+	}
     }
 
   int prec = TYPE_PRECISION (TREE_TYPE (cnt));
Index: gcc/testsuite/gcc.dg/Wstringop-truncation-6.c
===================================================================
--- gcc/testsuite/gcc.dg/Wstringop-truncation-6.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/Wstringop-truncation-6.c	(working copy)
@@ -0,0 +1,59 @@
+/* PR tree-optimization/84561 - -Wstringop-truncation with -O2 depends
+   on strncpy's size type
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+enum { N = 3 };
+
+struct S
+{
+  char a[N + 1];
+};
+
+void set (struct S *ps, const char* s, size_t n)
+{
+  if (n > N)
+    n = N;
+
+  __builtin_strncpy (ps->a, s, n);   /* { dg-bogus "\\\[-Wstringop-truncation]" } */
+  ps->a[n] = 0;
+}
+
+struct A
+{
+  struct S str;
+};
+
+void setStringSize_t (struct A *pa, const char *s, size_t n)
+{
+  set (&pa->str, s, n);
+}
+
+void setStringUnsignedInt (struct A *pa, const char* s, unsigned int n)
+{
+  set (&pa->str, s, n);
+}
+
+struct B
+{
+  struct A a;
+};
+
+struct A* getA (struct B *pb)
+{
+  return &pb->a;
+}
+
+void f (struct A *pa)
+{
+  setStringUnsignedInt (pa, "123", N); // No warning here.
+  setStringSize_t (pa, "123", N);      // No warning here.
+}
+
+void g (struct B *pb)
+{
+  setStringUnsignedInt (getA (pb), "123", N);  // Unexpected warning here.
+  setStringSize_t (getA (pb), "123", N);       // No warning here.
+}

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Richard Biener]

On Thu, Aug 30, 2018 at 2:12 AM Martin Sebor <msebor@gmail.com> wrote:
>
> The attached patch adds code to work harder to determine whether
> the destination of an assignment involving MEM_REF is the same
> as the destination of a prior strncpy call.  The included test
> case demonstrates when this situation comes up.  During ccp,
> dstbase and lhsbase returned by get_addr_base_and_unit_offset()
> end up looking like this:

"During CCP" means exactly when?  The CCP lattice tracks copies
so CCP should already know that _1 == _8.  I suppose during
substitute_and_fold then?  But that replaces uses before folding
the stmt.

So I'm confused.

>
>    _8 = &pb_3(D)->a;
>    _9 = _8;
>    _1 = _9;
>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>    MEM[(struct S *)_1].a[n_7] = 0;
>
> so the loops follow the simple assignments until we get at
> the ADDR_EXPR assigned to _8 which is the same as the strncpy
> destination.
>
> Tested on x86_64-linux.
>
> Martin

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Martin Sebor]

On 08/30/2018 02:35 AM, Richard Biener wrote:
> On Thu, Aug 30, 2018 at 2:12 AM Martin Sebor <msebor@gmail.com> wrote:
>>
>> The attached patch adds code to work harder to determine whether
>> the destination of an assignment involving MEM_REF is the same
>> as the destination of a prior strncpy call.  The included test
>> case demonstrates when this situation comes up.  During ccp,
>> dstbase and lhsbase returned by get_addr_base_and_unit_offset()
>> end up looking like this:
>
> "During CCP" means exactly when?  The CCP lattice tracks copies
> so CCP should already know that _1 == _8.  I suppose during
> substitute_and_fold then?  But that replaces uses before folding
> the stmt.

Yes, when ccp_finalize() performs the final substitution during
substitute_and_fold().

Martin

>
> So I'm confused.
>
>>
>>    _8 = &pb_3(D)->a;
>>    _9 = _8;
>>    _1 = _9;
>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>    MEM[(struct S *)_1].a[n_7] = 0;
>>
>> so the loops follow the simple assignments until we get at
>> the ADDR_EXPR assigned to _8 which is the same as the strncpy
>> destination.
>>
>> Tested on x86_64-linux.
>>
>> Martin

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Richard Biener]

On August 30, 2018 6:54:21 PM GMT+02:00, Martin Sebor <msebor@gmail.com> wrote:
>On 08/30/2018 02:35 AM, Richard Biener wrote:
>> On Thu, Aug 30, 2018 at 2:12 AM Martin Sebor <msebor@gmail.com>
>wrote:
>>>
>>> The attached patch adds code to work harder to determine whether
>>> the destination of an assignment involving MEM_REF is the same
>>> as the destination of a prior strncpy call.  The included test
>>> case demonstrates when this situation comes up.  During ccp,
>>> dstbase and lhsbase returned by get_addr_base_and_unit_offset()
>>> end up looking like this:
>>
>> "During CCP" means exactly when?  The CCP lattice tracks copies
>> so CCP should already know that _1 == _8.  I suppose during
>> substitute_and_fold then?  But that replaces uses before folding
>> the stmt.
>
>Yes, when ccp_finalize() performs the final substitution during
>substitute_and_fold().

But then you shouldn't need the loop but at most look at the pointer SSA Def to get at the non-invariant ADDR_EXPR. 

Richard. 

>Martin
>
>>
>> So I'm confused.
>>
>>>
>>>    _8 = &pb_3(D)->a;
>>>    _9 = _8;
>>>    _1 = _9;
>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>>    MEM[(struct S *)_1].a[n_7] = 0;
>>>
>>> so the loops follow the simple assignments until we get at
>>> the ADDR_EXPR assigned to _8 which is the same as the strncpy
>>> destination.
>>>
>>> Tested on x86_64-linux.
>>>
>>> Martin

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Martin Sebor]

On 08/30/2018 11:22 AM, Richard Biener wrote:
> On August 30, 2018 6:54:21 PM GMT+02:00, Martin Sebor <msebor@gmail.com> wrote:
>> On 08/30/2018 02:35 AM, Richard Biener wrote:
>>> On Thu, Aug 30, 2018 at 2:12 AM Martin Sebor <msebor@gmail.com>
>> wrote:
>>>>
>>>> The attached patch adds code to work harder to determine whether
>>>> the destination of an assignment involving MEM_REF is the same
>>>> as the destination of a prior strncpy call.  The included test
>>>> case demonstrates when this situation comes up.  During ccp,
>>>> dstbase and lhsbase returned by get_addr_base_and_unit_offset()
>>>> end up looking like this:
>>>
>>> "During CCP" means exactly when?  The CCP lattice tracks copies
>>> so CCP should already know that _1 == _8.  I suppose during
>>> substitute_and_fold then?  But that replaces uses before folding
>>> the stmt.
>>
>> Yes, when ccp_finalize() performs the final substitution during
>> substitute_and_fold().
>
> But then you shouldn't need the loop but at most look at the pointer SSA Def to get at the non-invariant ADDR_EXPR.

I don't follow.   Are you suggesting to compare
SSA_NAME_DEF_STMT (dstbase) to SSA_NAME_DEF_STMT (lhsbase) for
equality?  They're not equal.

The first loop iterates once and retrieves

   1.  _8 = &pb_3(D)->a;

The second loop iterates three times and retrieves:

   1.  _1 = _9
   2.  _9 = _8
   3.  _8 = &pb_3(D)->a;

How do I get from _1 to &pb_3(D)->a without iterating?  Or are
you saying to still iterate but compare the SSA_NAME_DEF_STMT?

Martin

>
> Richard.
>
>> Martin
>>
>>>
>>> So I'm confused.
>>>
>>>>
>>>>    _8 = &pb_3(D)->a;
>>>>    _9 = _8;
>>>>    _1 = _9;
>>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>>>    MEM[(struct S *)_1].a[n_7] = 0;
>>>>
>>>> so the loops follow the simple assignments until we get at
>>>> the ADDR_EXPR assigned to _8 which is the same as the strncpy
>>>> destination.
>>>>
>>>> Tested on x86_64-linux.
>>>>
>>>> Martin
>

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Richard Biener]

On Thu, Aug 30, 2018 at 7:39 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 08/30/2018 11:22 AM, Richard Biener wrote:
> > On August 30, 2018 6:54:21 PM GMT+02:00, Martin Sebor <msebor@gmail.com> wrote:
> >> On 08/30/2018 02:35 AM, Richard Biener wrote:
> >>> On Thu, Aug 30, 2018 at 2:12 AM Martin Sebor <msebor@gmail.com>
> >> wrote:
> >>>>
> >>>> The attached patch adds code to work harder to determine whether
> >>>> the destination of an assignment involving MEM_REF is the same
> >>>> as the destination of a prior strncpy call.  The included test
> >>>> case demonstrates when this situation comes up.  During ccp,
> >>>> dstbase and lhsbase returned by get_addr_base_and_unit_offset()
> >>>> end up looking like this:
> >>>
> >>> "During CCP" means exactly when?  The CCP lattice tracks copies
> >>> so CCP should already know that _1 == _8.  I suppose during
> >>> substitute_and_fold then?  But that replaces uses before folding
> >>> the stmt.
> >>
> >> Yes, when ccp_finalize() performs the final substitution during
> >> substitute_and_fold().
> >
> > But then you shouldn't need the loop but at most look at the pointer SSA Def to get at the non-invariant ADDR_EXPR.
>
> I don't follow.   Are you suggesting to compare
> SSA_NAME_DEF_STMT (dstbase) to SSA_NAME_DEF_STMT (lhsbase) for
> equality?  They're not equal.

No.

> The first loop iterates once and retrieves
>
>    1.  _8 = &pb_3(D)->a;
>
> The second loop iterates three times and retrieves:
>
>    1.  _1 = _9
>    2.  _9 = _8
>    3.  _8 = &pb_3(D)->a;
>
> How do I get from _1 to &pb_3(D)->a without iterating?  Or are
> you saying to still iterate but compare the SSA_NAME_DEF_STMT?

I say you should retrieve _8 = &pb_3(D)->a immediately since the
copies should be
propagated out at this stage.

> Martin
>
> >
> > Richard.
> >
> >> Martin
> >>
> >>>
> >>> So I'm confused.
> >>>
> >>>>
> >>>>    _8 = &pb_3(D)->a;
> >>>>    _9 = _8;
> >>>>    _1 = _9;
> >>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
> >>>>    MEM[(struct S *)_1].a[n_7] = 0;
> >>>>
> >>>> so the loops follow the simple assignments until we get at
> >>>> the ADDR_EXPR assigned to _8 which is the same as the strncpy
> >>>> destination.
> >>>>
> >>>> Tested on x86_64-linux.
> >>>>
> >>>> Martin
> >
>

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Martin Sebor]

On 08/31/2018 04:07 AM, Richard Biener wrote:
> On Thu, Aug 30, 2018 at 7:39 PM Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 08/30/2018 11:22 AM, Richard Biener wrote:
>>> On August 30, 2018 6:54:21 PM GMT+02:00, Martin Sebor <msebor@gmail.com> wrote:
>>>> On 08/30/2018 02:35 AM, Richard Biener wrote:
>>>>> On Thu, Aug 30, 2018 at 2:12 AM Martin Sebor <msebor@gmail.com>
>>>> wrote:
>>>>>>
>>>>>> The attached patch adds code to work harder to determine whether
>>>>>> the destination of an assignment involving MEM_REF is the same
>>>>>> as the destination of a prior strncpy call.  The included test
>>>>>> case demonstrates when this situation comes up.  During ccp,
>>>>>> dstbase and lhsbase returned by get_addr_base_and_unit_offset()
>>>>>> end up looking like this:
>>>>>
>>>>> "During CCP" means exactly when?  The CCP lattice tracks copies
>>>>> so CCP should already know that _1 == _8.  I suppose during
>>>>> substitute_and_fold then?  But that replaces uses before folding
>>>>> the stmt.
>>>>
>>>> Yes, when ccp_finalize() performs the final substitution during
>>>> substitute_and_fold().
>>>
>>> But then you shouldn't need the loop but at most look at the pointer SSA Def to get at the non-invariant ADDR_EXPR.
>>
>> I don't follow.   Are you suggesting to compare
>> SSA_NAME_DEF_STMT (dstbase) to SSA_NAME_DEF_STMT (lhsbase) for
>> equality?  They're not equal.
>
> No.
>
>> The first loop iterates once and retrieves
>>
>>    1.  _8 = &pb_3(D)->a;
>>
>> The second loop iterates three times and retrieves:
>>
>>    1.  _1 = _9
>>    2.  _9 = _8
>>    3.  _8 = &pb_3(D)->a;
>>
>> How do I get from _1 to &pb_3(D)->a without iterating?  Or are
>> you saying to still iterate but compare the SSA_NAME_DEF_STMT?
>
> I say you should retrieve _8 = &pb_3(D)->a immediately since the
> copies should be
> propagated out at this stage.

The warning is issued as the strncpy call is being folded (during
the dom walk in substitute_and_fold_engine::substitute_and_fold)
but before the subsequent statements have been folded (during
the subsequent loop to eliminate statements).  So at the point
of the strncpy folding the three assignments above are still
there.

I can't think of a good way to solve this problem that's not
overly intrusive.  Unless you have some suggestions for how
to deal with it, is the patch okay as is?

Martin

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Jeff Law]

On 9/12/18 11:46 AM, Martin Sebor wrote:
> On 08/31/2018 04:07 AM, Richard Biener wrote:
>> On Thu, Aug 30, 2018 at 7:39 PM Martin Sebor <msebor@gmail.com> wrote:
>>>
>>> On 08/30/2018 11:22 AM, Richard Biener wrote:
>>>> On August 30, 2018 6:54:21 PM GMT+02:00, Martin Sebor
>>>> <msebor@gmail.com> wrote:
>>>>> On 08/30/2018 02:35 AM, Richard Biener wrote:
>>>>>> On Thu, Aug 30, 2018 at 2:12 AM Martin Sebor <msebor@gmail.com>
>>>>> wrote:
>>>>>>>
>>>>>>> The attached patch adds code to work harder to determine whether
>>>>>>> the destination of an assignment involving MEM_REF is the same
>>>>>>> as the destination of a prior strncpy call.  The included test
>>>>>>> case demonstrates when this situation comes up.  During ccp,
>>>>>>> dstbase and lhsbase returned by get_addr_base_and_unit_offset()
>>>>>>> end up looking like this:
>>>>>>
>>>>>> "During CCP" means exactly when?  The CCP lattice tracks copies
>>>>>> so CCP should already know that _1 == _8.  I suppose during
>>>>>> substitute_and_fold then?  But that replaces uses before folding
>>>>>> the stmt.
>>>>>
>>>>> Yes, when ccp_finalize() performs the final substitution during
>>>>> substitute_and_fold().
>>>>
>>>> But then you shouldn't need the loop but at most look at the pointer
>>>> SSA Def to get at the non-invariant ADDR_EXPR.
>>>
>>> I don't follow.   Are you suggesting to compare
>>> SSA_NAME_DEF_STMT (dstbase) to SSA_NAME_DEF_STMT (lhsbase) for
>>> equality?  They're not equal.
>>
>> No.
>>
>>> The first loop iterates once and retrieves
>>>
>>>    1.  _8 = &pb_3(D)->a;
>>>
>>> The second loop iterates three times and retrieves:
>>>
>>>    1.  _1 = _9
>>>    2.  _9 = _8
>>>    3.  _8 = &pb_3(D)->a;
>>>
>>> How do I get from _1 to &pb_3(D)->a without iterating?  Or are
>>> you saying to still iterate but compare the SSA_NAME_DEF_STMT?
>>
>> I say you should retrieve _8 = &pb_3(D)->a immediately since the
>> copies should be
>> propagated out at this stage.
> 
> The warning is issued as the strncpy call is being folded (during
> the dom walk in substitute_and_fold_engine::substitute_and_fold)
> but before the subsequent statements have been folded (during
> the subsequent loop to eliminate statements).  So at the point
> of the strncpy folding the three assignments above are still
> there.
> 
> I can't think of a good way to solve this problem that's not
> overly intrusive.  Unless you have some suggestions for how
> to deal with it, is the patch okay as is?
In what pass do you see the the naked copies?  In general those should
have been propagated away.

If they're only discovered as copies within the pass where you're trying
to issue the diagnostic, then you'd want to see if the pass has any
internal structures that tell you about equivalences.

Jeff

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Martin Sebor]

On 09/14/2018 03:35 PM, Jeff Law wrote:
> On 9/12/18 11:46 AM, Martin Sebor wrote:
>> On 08/31/2018 04:07 AM, Richard Biener wrote:
>>> On Thu, Aug 30, 2018 at 7:39 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>
>>>> On 08/30/2018 11:22 AM, Richard Biener wrote:
>>>>> On August 30, 2018 6:54:21 PM GMT+02:00, Martin Sebor
>>>>> <msebor@gmail.com> wrote:
>>>>>> On 08/30/2018 02:35 AM, Richard Biener wrote:
>>>>>>> On Thu, Aug 30, 2018 at 2:12 AM Martin Sebor <msebor@gmail.com>
>>>>>> wrote:
>>>>>>>>
>>>>>>>> The attached patch adds code to work harder to determine whether
>>>>>>>> the destination of an assignment involving MEM_REF is the same
>>>>>>>> as the destination of a prior strncpy call.  The included test
>>>>>>>> case demonstrates when this situation comes up.  During ccp,
>>>>>>>> dstbase and lhsbase returned by get_addr_base_and_unit_offset()
>>>>>>>> end up looking like this:
>>>>>>>
>>>>>>> "During CCP" means exactly when?  The CCP lattice tracks copies
>>>>>>> so CCP should already know that _1 == _8.  I suppose during
>>>>>>> substitute_and_fold then?  But that replaces uses before folding
>>>>>>> the stmt.
>>>>>>
>>>>>> Yes, when ccp_finalize() performs the final substitution during
>>>>>> substitute_and_fold().
>>>>>
>>>>> But then you shouldn't need the loop but at most look at the pointer
>>>>> SSA Def to get at the non-invariant ADDR_EXPR.
>>>>
>>>> I don't follow.   Are you suggesting to compare
>>>> SSA_NAME_DEF_STMT (dstbase) to SSA_NAME_DEF_STMT (lhsbase) for
>>>> equality?  They're not equal.
>>>
>>> No.
>>>
>>>> The first loop iterates once and retrieves
>>>>
>>>>    1.  _8 = &pb_3(D)->a;
>>>>
>>>> The second loop iterates three times and retrieves:
>>>>
>>>>    1.  _1 = _9
>>>>    2.  _9 = _8
>>>>    3.  _8 = &pb_3(D)->a;
>>>>
>>>> How do I get from _1 to &pb_3(D)->a without iterating?  Or are
>>>> you saying to still iterate but compare the SSA_NAME_DEF_STMT?
>>>
>>> I say you should retrieve _8 = &pb_3(D)->a immediately since the
>>> copies should be
>>> propagated out at this stage.
>>
>> The warning is issued as the strncpy call is being folded (during
>> the dom walk in substitute_and_fold_engine::substitute_and_fold)
>> but before the subsequent statements have been folded (during
>> the subsequent loop to eliminate statements).  So at the point
>> of the strncpy folding the three assignments above are still
>> there.
>>
>> I can't think of a good way to solve this problem that's not
>> overly intrusive.  Unless you have some suggestions for how
>> to deal with it, is the patch okay as is?
> In what pass do you see the the naked copies?  In general those should
> have been propagated away.

As I said above, this happens during the dom walk in the ccp
pass:

   substitute_and_fold_dom_walker walker (CDI_DOMINATORS, this);
   walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));

The warning is issued during the walker.walk() call as
strncpy is being folded into memcpy.  The prior assignments are
only propagated later, when the next statement after the strncpy
call is reached.  It happens in
substitute_and_fold_dom_walker::before_dom_children(). So during
the strncpy folding we see the next statement as:

   MEM[(struct S *)_1].a[n_7] = 0;

After the strncpy call is transformed to memcpy, the assignment
above is transformed to

   MEM[(struct S *)_8].a[3] = 0;

>
> If they're only discovered as copies within the pass where you're trying
> to issue the diagnostic, then you'd want to see if the pass has any
> internal structures that tell you about equivalences.

I don't know if this is possible.  I don't see any APIs in
tree-ssa-propagate.h that would let me query the internal data
somehow to find out during folding (when the warning is issued).

Martin

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Jeff Law]

On 9/14/18 4:11 PM, Martin Sebor wrote:
> On 09/14/2018 03:35 PM, Jeff Law wrote:
>> On 9/12/18 11:46 AM, Martin Sebor wrote:
>>> On 08/31/2018 04:07 AM, Richard Biener wrote:
>>>> On Thu, Aug 30, 2018 at 7:39 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>>
>>>>> On 08/30/2018 11:22 AM, Richard Biener wrote:
>>>>>> On August 30, 2018 6:54:21 PM GMT+02:00, Martin Sebor
>>>>>> <msebor@gmail.com> wrote:
>>>>>>> On 08/30/2018 02:35 AM, Richard Biener wrote:
>>>>>>>> On Thu, Aug 30, 2018 at 2:12 AM Martin Sebor <msebor@gmail.com>
>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> The attached patch adds code to work harder to determine whether
>>>>>>>>> the destination of an assignment involving MEM_REF is the same
>>>>>>>>> as the destination of a prior strncpy call.  The included test
>>>>>>>>> case demonstrates when this situation comes up.  During ccp,
>>>>>>>>> dstbase and lhsbase returned by get_addr_base_and_unit_offset()
>>>>>>>>> end up looking like this:
>>>>>>>>
>>>>>>>> "During CCP" means exactly when?  The CCP lattice tracks copies
>>>>>>>> so CCP should already know that _1 == _8.  I suppose during
>>>>>>>> substitute_and_fold then?  But that replaces uses before folding
>>>>>>>> the stmt.
>>>>>>>
>>>>>>> Yes, when ccp_finalize() performs the final substitution during
>>>>>>> substitute_and_fold().
>>>>>>
>>>>>> But then you shouldn't need the loop but at most look at the pointer
>>>>>> SSA Def to get at the non-invariant ADDR_EXPR.
>>>>>
>>>>> I don't follow.   Are you suggesting to compare
>>>>> SSA_NAME_DEF_STMT (dstbase) to SSA_NAME_DEF_STMT (lhsbase) for
>>>>> equality?  They're not equal.
>>>>
>>>> No.
>>>>
>>>>> The first loop iterates once and retrieves
>>>>>
>>>>>    1.  _8 = &pb_3(D)->a;
>>>>>
>>>>> The second loop iterates three times and retrieves:
>>>>>
>>>>>    1.  _1 = _9
>>>>>    2.  _9 = _8
>>>>>    3.  _8 = &pb_3(D)->a;
>>>>>
>>>>> How do I get from _1 to &pb_3(D)->a without iterating?  Or are
>>>>> you saying to still iterate but compare the SSA_NAME_DEF_STMT?
>>>>
>>>> I say you should retrieve _8 = &pb_3(D)->a immediately since the
>>>> copies should be
>>>> propagated out at this stage.
>>>
>>> The warning is issued as the strncpy call is being folded (during
>>> the dom walk in substitute_and_fold_engine::substitute_and_fold)
>>> but before the subsequent statements have been folded (during
>>> the subsequent loop to eliminate statements).  So at the point
>>> of the strncpy folding the three assignments above are still
>>> there.
>>>
>>> I can't think of a good way to solve this problem that's not
>>> overly intrusive.  Unless you have some suggestions for how
>>> to deal with it, is the patch okay as is?
>> In what pass do you see the the naked copies?  In general those should
>> have been propagated away.
> 
> As I said above, this happens during the dom walk in the ccp
> pass:
My bad.  Sigh. CCP doesn't track copies, just constants, so there's not
going to be any data structure you can exploit.  And I don't think
there's a value number you can use to determine the two objects are the
same.

Hmm, let's back up a bit, what is does the relevant part of the IL look
like before CCP?  Is the real problem here that we have unpropagated
copies lying around in the IL?  Hmm, more likely the IL looksl ike:

   _8 = &pb_3(D)->a;
   _9 = _8;
   _1 = _9;
   strncpy (MEM_REF (&pb_3(D)->a), ...);
   MEM[(struct S *)_1].a[n_7] = 0;

If we were to propagate the copies out we'd at best have:

   _8 = &pb_3(D)->a;
   strncpy (MEM_REF (&pb_3(D)->a), ...);
   MEM[(struct S *)_8].a[n_7] = 0;


Is that in a form you can handle?  Or would we also need to forward
propagate the address computation into the use of _8?


jeff

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Martin Sebor]

On 09/17/2018 05:09 PM, Jeff Law wrote:
> On 9/14/18 4:11 PM, Martin Sebor wrote:
>> On 09/14/2018 03:35 PM, Jeff Law wrote:
>>> On 9/12/18 11:46 AM, Martin Sebor wrote:
>>>> On 08/31/2018 04:07 AM, Richard Biener wrote:
>>>>> On Thu, Aug 30, 2018 at 7:39 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>>>
>>>>>> On 08/30/2018 11:22 AM, Richard Biener wrote:
>>>>>>> On August 30, 2018 6:54:21 PM GMT+02:00, Martin Sebor
>>>>>>> <msebor@gmail.com> wrote:
>>>>>>>> On 08/30/2018 02:35 AM, Richard Biener wrote:
>>>>>>>>> On Thu, Aug 30, 2018 at 2:12 AM Martin Sebor <msebor@gmail.com>
>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> The attached patch adds code to work harder to determine whether
>>>>>>>>>> the destination of an assignment involving MEM_REF is the same
>>>>>>>>>> as the destination of a prior strncpy call.  The included test
>>>>>>>>>> case demonstrates when this situation comes up.  During ccp,
>>>>>>>>>> dstbase and lhsbase returned by get_addr_base_and_unit_offset()
>>>>>>>>>> end up looking like this:
>>>>>>>>>
>>>>>>>>> "During CCP" means exactly when?  The CCP lattice tracks copies
>>>>>>>>> so CCP should already know that _1 == _8.  I suppose during
>>>>>>>>> substitute_and_fold then?  But that replaces uses before folding
>>>>>>>>> the stmt.
>>>>>>>>
>>>>>>>> Yes, when ccp_finalize() performs the final substitution during
>>>>>>>> substitute_and_fold().
>>>>>>>
>>>>>>> But then you shouldn't need the loop but at most look at the pointer
>>>>>>> SSA Def to get at the non-invariant ADDR_EXPR.
>>>>>>
>>>>>> I don't follow.   Are you suggesting to compare
>>>>>> SSA_NAME_DEF_STMT (dstbase) to SSA_NAME_DEF_STMT (lhsbase) for
>>>>>> equality?  They're not equal.
>>>>>
>>>>> No.
>>>>>
>>>>>> The first loop iterates once and retrieves
>>>>>>
>>>>>>    1.  _8 = &pb_3(D)->a;
>>>>>>
>>>>>> The second loop iterates three times and retrieves:
>>>>>>
>>>>>>    1.  _1 = _9
>>>>>>    2.  _9 = _8
>>>>>>    3.  _8 = &pb_3(D)->a;
>>>>>>
>>>>>> How do I get from _1 to &pb_3(D)->a without iterating?  Or are
>>>>>> you saying to still iterate but compare the SSA_NAME_DEF_STMT?
>>>>>
>>>>> I say you should retrieve _8 = &pb_3(D)->a immediately since the
>>>>> copies should be
>>>>> propagated out at this stage.
>>>>
>>>> The warning is issued as the strncpy call is being folded (during
>>>> the dom walk in substitute_and_fold_engine::substitute_and_fold)
>>>> but before the subsequent statements have been folded (during
>>>> the subsequent loop to eliminate statements).  So at the point
>>>> of the strncpy folding the three assignments above are still
>>>> there.
>>>>
>>>> I can't think of a good way to solve this problem that's not
>>>> overly intrusive.  Unless you have some suggestions for how
>>>> to deal with it, is the patch okay as is?
>>> In what pass do you see the the naked copies?  In general those should
>>> have been propagated away.
>>
>> As I said above, this happens during the dom walk in the ccp
>> pass:
> My bad.  Sigh. CCP doesn't track copies, just constants, so there's not
> going to be any data structure you can exploit.  And I don't think
> there's a value number you can use to determine the two objects are the
> same.
>
> Hmm, let's back up a bit, what is does the relevant part of the IL look
> like before CCP?  Is the real problem here that we have unpropagated
> copies lying around in the IL?  Hmm, more likely the IL looksl ike:
>
>    _8 = &pb_3(D)->a;
>    _9 = _8;
>    _1 = _9;
>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>    MEM[(struct S *)_1].a[n_7] = 0;

Yes, that is what the folder sees while the strncpy call is
being transformed/folded by ccp.  The MEM_REF is folded just
after the strncpy call and that's when it's transformed into

   MEM[(struct S *)_8].a[n_7] = 0;

(The assignments to _1 and _9 don't get removed until after
the dom walk finishes).

>
> If we were to propagate the copies out we'd at best have:
>
>    _8 = &pb_3(D)->a;
>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>    MEM[(struct S *)_8].a[n_7] = 0;
>
>
> Is that in a form you can handle?  Or would we also need to forward
> propagate the address computation into the use of _8?

The above works as long as we look at the def_stmt of _8 in
the MEM_REF (we currently don't).  That's also what the last
iteration of the loop does.  In this case (with _8) it would
be discovered in the first iteration, so the loop could be
replaced by a simple if statement.

But I'm not sure I understand the concern with the loop.  Is
it that we are looping at all, i.e., the cost?  Or that ccp
is doing something wrong or suboptimal? (Should have
propagated the value of _8 earlier?)

Martin

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Jeff Law]

On 9/18/18 11:12 AM, Martin Sebor wrote:

>> My bad.  Sigh. CCP doesn't track copies, just constants, so there's not
>> going to be any data structure you can exploit.  And I don't think
>> there's a value number you can use to determine the two objects are the
>> same.
>>
>> Hmm, let's back up a bit, what is does the relevant part of the IL look
>> like before CCP?  Is the real problem here that we have unpropagated
>> copies lying around in the IL?  Hmm, more likely the IL looksl ike:
>>
>>    _8 = &pb_3(D)->a;
>>    _9 = _8;
>>    _1 = _9;
>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>    MEM[(struct S *)_1].a[n_7] = 0;
> 
> Yes, that is what the folder sees while the strncpy call is
> being transformed/folded by ccp.  The MEM_REF is folded just
> after the strncpy call and that's when it's transformed into
> 
>   MEM[(struct S *)_8].a[n_7] = 0;
> 
> (The assignments to _1 and _9 don't get removed until after
> the dom walk finishes).
> 
>>
>> If we were to propagate the copies out we'd at best have:
>>
>>    _8 = &pb_3(D)->a;
>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>    MEM[(struct S *)_8].a[n_7] = 0;
>>
>>
>> Is that in a form you can handle?  Or would we also need to forward
>> propagate the address computation into the use of _8?
> 
> The above works as long as we look at the def_stmt of _8 in
> the MEM_REF (we currently don't).  That's also what the last
> iteration of the loop does.  In this case (with _8) it would
> be discovered in the first iteration, so the loop could be
> replaced by a simple if statement.
> 
> But I'm not sure I understand the concern with the loop.  Is
> it that we are looping at all, i.e., the cost?  Or that ccp
> is doing something wrong or suboptimal? (Should have
> propagated the value of _8 earlier?)
I suspect it's more a concern that things like copies are typically
propagated away.   So their existence in the IL (and consequently your
need to handle them) raises the question "has something else failed to
do its job earlier".

During which of the CCP passes is this happening?  Can we pull the
warning out of the folder (even if that means having a distinct warning
pass over the IL?)


Jeff

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Martin Sebor]

On 09/18/2018 12:58 PM, Jeff Law wrote:
> On 9/18/18 11:12 AM, Martin Sebor wrote:
>
>>> My bad.  Sigh. CCP doesn't track copies, just constants, so there's not
>>> going to be any data structure you can exploit.  And I don't think
>>> there's a value number you can use to determine the two objects are the
>>> same.
>>>
>>> Hmm, let's back up a bit, what is does the relevant part of the IL look
>>> like before CCP?  Is the real problem here that we have unpropagated
>>> copies lying around in the IL?  Hmm, more likely the IL looksl ike:
>>>
>>>    _8 = &pb_3(D)->a;
>>>    _9 = _8;
>>>    _1 = _9;
>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>>    MEM[(struct S *)_1].a[n_7] = 0;
>>
>> Yes, that is what the folder sees while the strncpy call is
>> being transformed/folded by ccp.  The MEM_REF is folded just
>> after the strncpy call and that's when it's transformed into
>>
>>   MEM[(struct S *)_8].a[n_7] = 0;
>>
>> (The assignments to _1 and _9 don't get removed until after
>> the dom walk finishes).
>>
>>>
>>> If we were to propagate the copies out we'd at best have:
>>>
>>>    _8 = &pb_3(D)->a;
>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>>    MEM[(struct S *)_8].a[n_7] = 0;
>>>
>>>
>>> Is that in a form you can handle?  Or would we also need to forward
>>> propagate the address computation into the use of _8?
>>
>> The above works as long as we look at the def_stmt of _8 in
>> the MEM_REF (we currently don't).  That's also what the last
>> iteration of the loop does.  In this case (with _8) it would
>> be discovered in the first iteration, so the loop could be
>> replaced by a simple if statement.
>>
>> But I'm not sure I understand the concern with the loop.  Is
>> it that we are looping at all, i.e., the cost?  Or that ccp
>> is doing something wrong or suboptimal? (Should have
>> propagated the value of _8 earlier?)
> I suspect it's more a concern that things like copies are typically
> propagated away.   So their existence in the IL (and consequently your
> need to handle them) raises the question "has something else failed to
> do its job earlier".
>
> During which of the CCP passes is this happening?  Can we pull the
> warning out of the folder (even if that means having a distinct warning
> pass over the IL?)

It happens during the third run of the pass.

The only way to do what you suggest that I could think of is
to defer the strncpy to memcpy transformation until after
the warning pass.  That was also my earlier suggestion: defer
both it and the warning until the tree-ssa-strlen pass (where
the warning is implemented to begin with -- the folder calls
into it).

Martin

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Jeff Law]

On 9/18/18 1:46 PM, Martin Sebor wrote:
> On 09/18/2018 12:58 PM, Jeff Law wrote:
>> On 9/18/18 11:12 AM, Martin Sebor wrote:
>>
>>>> My bad.  Sigh. CCP doesn't track copies, just constants, so there's not
>>>> going to be any data structure you can exploit.  And I don't think
>>>> there's a value number you can use to determine the two objects are the
>>>> same.
>>>>
>>>> Hmm, let's back up a bit, what is does the relevant part of the IL look
>>>> like before CCP?  Is the real problem here that we have unpropagated
>>>> copies lying around in the IL?  Hmm, more likely the IL looksl ike:
>>>>
>>>>    _8 = &pb_3(D)->a;
>>>>    _9 = _8;
>>>>    _1 = _9;
>>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>>>    MEM[(struct S *)_1].a[n_7] = 0;
>>>
>>> Yes, that is what the folder sees while the strncpy call is
>>> being transformed/folded by ccp.  The MEM_REF is folded just
>>> after the strncpy call and that's when it's transformed into
>>>
>>>   MEM[(struct S *)_8].a[n_7] = 0;
>>>
>>> (The assignments to _1 and _9 don't get removed until after
>>> the dom walk finishes).
>>>
>>>>
>>>> If we were to propagate the copies out we'd at best have:
>>>>
>>>>    _8 = &pb_3(D)->a;
>>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>>>    MEM[(struct S *)_8].a[n_7] = 0;
>>>>
>>>>
>>>> Is that in a form you can handle?  Or would we also need to forward
>>>> propagate the address computation into the use of _8?
>>>
>>> The above works as long as we look at the def_stmt of _8 in
>>> the MEM_REF (we currently don't).  That's also what the last
>>> iteration of the loop does.  In this case (with _8) it would
>>> be discovered in the first iteration, so the loop could be
>>> replaced by a simple if statement.
>>>
>>> But I'm not sure I understand the concern with the loop.  Is
>>> it that we are looping at all, i.e., the cost?  Or that ccp
>>> is doing something wrong or suboptimal? (Should have
>>> propagated the value of _8 earlier?)
>> I suspect it's more a concern that things like copies are typically
>> propagated away.   So their existence in the IL (and consequently your
>> need to handle them) raises the question "has something else failed to
>> do its job earlier".
>>
>> During which of the CCP passes is this happening?  Can we pull the
>> warning out of the folder (even if that means having a distinct warning
>> pass over the IL?)
> 
> It happens during the third run of the pass.
> 
> The only way to do what you suggest that I could think of is
> to defer the strncpy to memcpy transformation until after
> the warning pass.  That was also my earlier suggestion: defer
> both it and the warning until the tree-ssa-strlen pass (where
> the warning is implemented to begin with -- the folder calls
> into it).
If it's happening that late (CCP3) in general, then ISTM we ought to be
able to get the warning out of the folder.  We just have to pick the
right spot.

warn_restrict runs before fold_all_builtins, but after dom/vrp so we
should have the IL in pretty good shape.  That seems like about the
right time.

I wonder if we could generalize warn_restrict to be a more generic
warning pass over the IL and place it right before fold_builtins.

Jeff
jeff

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Martin Sebor]

On 09/18/2018 10:23 PM, Jeff Law wrote:
> On 9/18/18 1:46 PM, Martin Sebor wrote:
>> On 09/18/2018 12:58 PM, Jeff Law wrote:
>>> On 9/18/18 11:12 AM, Martin Sebor wrote:
>>>
>>>>> My bad.  Sigh. CCP doesn't track copies, just constants, so there's not
>>>>> going to be any data structure you can exploit.  And I don't think
>>>>> there's a value number you can use to determine the two objects are the
>>>>> same.
>>>>>
>>>>> Hmm, let's back up a bit, what is does the relevant part of the IL look
>>>>> like before CCP?  Is the real problem here that we have unpropagated
>>>>> copies lying around in the IL?  Hmm, more likely the IL looksl ike:
>>>>>
>>>>>    _8 = &pb_3(D)->a;
>>>>>    _9 = _8;
>>>>>    _1 = _9;
>>>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>>>>    MEM[(struct S *)_1].a[n_7] = 0;
>>>>
>>>> Yes, that is what the folder sees while the strncpy call is
>>>> being transformed/folded by ccp.  The MEM_REF is folded just
>>>> after the strncpy call and that's when it's transformed into
>>>>
>>>>   MEM[(struct S *)_8].a[n_7] = 0;
>>>>
>>>> (The assignments to _1 and _9 don't get removed until after
>>>> the dom walk finishes).
>>>>
>>>>>
>>>>> If we were to propagate the copies out we'd at best have:
>>>>>
>>>>>    _8 = &pb_3(D)->a;
>>>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>>>>    MEM[(struct S *)_8].a[n_7] = 0;
>>>>>
>>>>>
>>>>> Is that in a form you can handle?  Or would we also need to forward
>>>>> propagate the address computation into the use of _8?
>>>>
>>>> The above works as long as we look at the def_stmt of _8 in
>>>> the MEM_REF (we currently don't).  That's also what the last
>>>> iteration of the loop does.  In this case (with _8) it would
>>>> be discovered in the first iteration, so the loop could be
>>>> replaced by a simple if statement.
>>>>
>>>> But I'm not sure I understand the concern with the loop.  Is
>>>> it that we are looping at all, i.e., the cost?  Or that ccp
>>>> is doing something wrong or suboptimal? (Should have
>>>> propagated the value of _8 earlier?)
>>> I suspect it's more a concern that things like copies are typically
>>> propagated away.   So their existence in the IL (and consequently your
>>> need to handle them) raises the question "has something else failed to
>>> do its job earlier".
>>>
>>> During which of the CCP passes is this happening?  Can we pull the
>>> warning out of the folder (even if that means having a distinct warning
>>> pass over the IL?)
>>
>> It happens during the third run of the pass.
>>
>> The only way to do what you suggest that I could think of is
>> to defer the strncpy to memcpy transformation until after
>> the warning pass.  That was also my earlier suggestion: defer
>> both it and the warning until the tree-ssa-strlen pass (where
>> the warning is implemented to begin with -- the folder calls
>> into it).
> If it's happening that late (CCP3) in general, then ISTM we ought to be
> able to get the warning out of the folder.  We just have to pick the
> right spot.
>
> warn_restrict runs before fold_all_builtins, but after dom/vrp so we
> should have the IL in pretty good shape.  That seems like about the
> right time.
>
> I wonder if we could generalize warn_restrict to be a more generic
> warning pass over the IL and place it right before fold_builtins.

The restrict pass doesn't know about string lengths so it can't
handle all the warnings about string built-ins (the strlen pass
now calls into it to issue some).  The strlen pass does so it
could handle most if not all of them (the folder also calls
into it to issue some warnings).  It would work even better if
it were also integrated with the object size pass.

We're already working on merging strlen with sprintf.  It seems
to me that the strlen pass would benefit not only from that but
also from integrating with object size and warn-restrict.  With
that, -Wstringop-overflow could be moved from builtins.c into
it as well (and also benefit not only from accurate string
lengths but also from the more accurate object size info).

What do you think about that?

Martin

PS I don't think I could do more than merger strlen and sprintf
before stage 1 ends (if even that much) so this would be a longer
term goal.

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Richard Biener]

On Wed, Sep 19, 2018 at 4:19 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 09/18/2018 10:23 PM, Jeff Law wrote:
> > On 9/18/18 1:46 PM, Martin Sebor wrote:
> >> On 09/18/2018 12:58 PM, Jeff Law wrote:
> >>> On 9/18/18 11:12 AM, Martin Sebor wrote:
> >>>
> >>>>> My bad.  Sigh. CCP doesn't track copies, just constants, so there's not
> >>>>> going to be any data structure you can exploit.  And I don't think
> >>>>> there's a value number you can use to determine the two objects are the
> >>>>> same.
> >>>>>
> >>>>> Hmm, let's back up a bit, what is does the relevant part of the IL look
> >>>>> like before CCP?  Is the real problem here that we have unpropagated
> >>>>> copies lying around in the IL?  Hmm, more likely the IL looksl ike:
> >>>>>
> >>>>>    _8 = &pb_3(D)->a;
> >>>>>    _9 = _8;
> >>>>>    _1 = _9;
> >>>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
> >>>>>    MEM[(struct S *)_1].a[n_7] = 0;
> >>>>
> >>>> Yes, that is what the folder sees while the strncpy call is
> >>>> being transformed/folded by ccp.  The MEM_REF is folded just
> >>>> after the strncpy call and that's when it's transformed into
> >>>>
> >>>>   MEM[(struct S *)_8].a[n_7] = 0;
> >>>>
> >>>> (The assignments to _1 and _9 don't get removed until after
> >>>> the dom walk finishes).
> >>>>
> >>>>>
> >>>>> If we were to propagate the copies out we'd at best have:
> >>>>>
> >>>>>    _8 = &pb_3(D)->a;
> >>>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
> >>>>>    MEM[(struct S *)_8].a[n_7] = 0;
> >>>>>
> >>>>>
> >>>>> Is that in a form you can handle?  Or would we also need to forward
> >>>>> propagate the address computation into the use of _8?
> >>>>
> >>>> The above works as long as we look at the def_stmt of _8 in
> >>>> the MEM_REF (we currently don't).  That's also what the last
> >>>> iteration of the loop does.  In this case (with _8) it would
> >>>> be discovered in the first iteration, so the loop could be
> >>>> replaced by a simple if statement.
> >>>>
> >>>> But I'm not sure I understand the concern with the loop.  Is
> >>>> it that we are looping at all, i.e., the cost?  Or that ccp
> >>>> is doing something wrong or suboptimal? (Should have
> >>>> propagated the value of _8 earlier?)
> >>> I suspect it's more a concern that things like copies are typically
> >>> propagated away.   So their existence in the IL (and consequently your
> >>> need to handle them) raises the question "has something else failed to
> >>> do its job earlier".
> >>>
> >>> During which of the CCP passes is this happening?  Can we pull the
> >>> warning out of the folder (even if that means having a distinct warning
> >>> pass over the IL?)
> >>
> >> It happens during the third run of the pass.
> >>
> >> The only way to do what you suggest that I could think of is
> >> to defer the strncpy to memcpy transformation until after
> >> the warning pass.  That was also my earlier suggestion: defer
> >> both it and the warning until the tree-ssa-strlen pass (where
> >> the warning is implemented to begin with -- the folder calls
> >> into it).
> > If it's happening that late (CCP3) in general, then ISTM we ought to be
> > able to get the warning out of the folder.  We just have to pick the
> > right spot.
> >
> > warn_restrict runs before fold_all_builtins, but after dom/vrp so we
> > should have the IL in pretty good shape.  That seems like about the
> > right time.
> >
> > I wonder if we could generalize warn_restrict to be a more generic
> > warning pass over the IL and place it right before fold_builtins.
>
> The restrict pass doesn't know about string lengths so it can't
> handle all the warnings about string built-ins (the strlen pass
> now calls into it to issue some).  The strlen pass does so it
> could handle most if not all of them (the folder also calls
> into it to issue some warnings).  It would work even better if
> it were also integrated with the object size pass.
>
> We're already working on merging strlen with sprintf.  It seems
> to me that the strlen pass would benefit not only from that but
> also from integrating with object size and warn-restrict.  With
> that, -Wstringop-overflow could be moved from builtins.c into
> it as well (and also benefit not only from accurate string
> lengths but also from the more accurate object size info).
>
> What do you think about that?

I think integrating the various "passes" (objectsize is also
as much a facility as a pass) generally makes sense given
it might end up improving all of them and reduce code duplication.

Richard.

>
> Martin
>
> PS I don't think I could do more than merger strlen and sprintf
> before stage 1 ends (if even that much) so this would be a longer
> term goal.

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Martin Sebor]

On 09/20/2018 03:06 AM, Richard Biener wrote:
> On Wed, Sep 19, 2018 at 4:19 PM Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 09/18/2018 10:23 PM, Jeff Law wrote:
>>> On 9/18/18 1:46 PM, Martin Sebor wrote:
>>>> On 09/18/2018 12:58 PM, Jeff Law wrote:
>>>>> On 9/18/18 11:12 AM, Martin Sebor wrote:
>>>>>
>>>>>>> My bad.  Sigh. CCP doesn't track copies, just constants, so there's not
>>>>>>> going to be any data structure you can exploit.  And I don't think
>>>>>>> there's a value number you can use to determine the two objects are the
>>>>>>> same.
>>>>>>>
>>>>>>> Hmm, let's back up a bit, what is does the relevant part of the IL look
>>>>>>> like before CCP?  Is the real problem here that we have unpropagated
>>>>>>> copies lying around in the IL?  Hmm, more likely the IL looksl ike:
>>>>>>>
>>>>>>>    _8 = &pb_3(D)->a;
>>>>>>>    _9 = _8;
>>>>>>>    _1 = _9;
>>>>>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>>>>>>    MEM[(struct S *)_1].a[n_7] = 0;
>>>>>>
>>>>>> Yes, that is what the folder sees while the strncpy call is
>>>>>> being transformed/folded by ccp.  The MEM_REF is folded just
>>>>>> after the strncpy call and that's when it's transformed into
>>>>>>
>>>>>>   MEM[(struct S *)_8].a[n_7] = 0;
>>>>>>
>>>>>> (The assignments to _1 and _9 don't get removed until after
>>>>>> the dom walk finishes).
>>>>>>
>>>>>>>
>>>>>>> If we were to propagate the copies out we'd at best have:
>>>>>>>
>>>>>>>    _8 = &pb_3(D)->a;
>>>>>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>>>>>>    MEM[(struct S *)_8].a[n_7] = 0;
>>>>>>>
>>>>>>>
>>>>>>> Is that in a form you can handle?  Or would we also need to forward
>>>>>>> propagate the address computation into the use of _8?
>>>>>>
>>>>>> The above works as long as we look at the def_stmt of _8 in
>>>>>> the MEM_REF (we currently don't).  That's also what the last
>>>>>> iteration of the loop does.  In this case (with _8) it would
>>>>>> be discovered in the first iteration, so the loop could be
>>>>>> replaced by a simple if statement.
>>>>>>
>>>>>> But I'm not sure I understand the concern with the loop.  Is
>>>>>> it that we are looping at all, i.e., the cost?  Or that ccp
>>>>>> is doing something wrong or suboptimal? (Should have
>>>>>> propagated the value of _8 earlier?)
>>>>> I suspect it's more a concern that things like copies are typically
>>>>> propagated away.   So their existence in the IL (and consequently your
>>>>> need to handle them) raises the question "has something else failed to
>>>>> do its job earlier".
>>>>>
>>>>> During which of the CCP passes is this happening?  Can we pull the
>>>>> warning out of the folder (even if that means having a distinct warning
>>>>> pass over the IL?)
>>>>
>>>> It happens during the third run of the pass.
>>>>
>>>> The only way to do what you suggest that I could think of is
>>>> to defer the strncpy to memcpy transformation until after
>>>> the warning pass.  That was also my earlier suggestion: defer
>>>> both it and the warning until the tree-ssa-strlen pass (where
>>>> the warning is implemented to begin with -- the folder calls
>>>> into it).
>>> If it's happening that late (CCP3) in general, then ISTM we ought to be
>>> able to get the warning out of the folder.  We just have to pick the
>>> right spot.
>>>
>>> warn_restrict runs before fold_all_builtins, but after dom/vrp so we
>>> should have the IL in pretty good shape.  That seems like about the
>>> right time.
>>>
>>> I wonder if we could generalize warn_restrict to be a more generic
>>> warning pass over the IL and place it right before fold_builtins.
>>
>> The restrict pass doesn't know about string lengths so it can't
>> handle all the warnings about string built-ins (the strlen pass
>> now calls into it to issue some).  The strlen pass does so it
>> could handle most if not all of them (the folder also calls
>> into it to issue some warnings).  It would work even better if
>> it were also integrated with the object size pass.
>>
>> We're already working on merging strlen with sprintf.  It seems
>> to me that the strlen pass would benefit not only from that but
>> also from integrating with object size and warn-restrict.  With
>> that, -Wstringop-overflow could be moved from builtins.c into
>> it as well (and also benefit not only from accurate string
>> lengths but also from the more accurate object size info).
>>
>> What do you think about that?
>
> I think integrating the various "passes" (objectsize is also
> as much a facility as a pass) generally makes sense given
> it might end up improving all of them and reduce code duplication.

Okay.  If Jeff agrees I'll see if I can make it happen for GCC
10.  Until then, does either of you have any suggestions for
a different approach in this patch or is it acceptable with
the loop as is?

Martin

>
> Richard.
>
>>
>> Martin
>>
>> PS I don't think I could do more than merger strlen and sprintf
>> before stage 1 ends (if even that much) so this would be a longer
>> term goal.

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Jeff Law]

On 9/19/18 8:19 AM, Martin Sebor wrote:
> 
> The restrict pass doesn't know about string lengths so it can't
> handle all the warnings about string built-ins (the strlen pass
> now calls into it to issue some).  The strlen pass does so it
> could handle most if not all of them (the folder also calls
> into it to issue some warnings).  It would work even better if
> it were also integrated with the object size pass.
> 
> We're already working on merging strlen with sprintf.  It seems
> to me that the strlen pass would benefit not only from that but
> also from integrating with object size and warn-restrict.  With
> that, -Wstringop-overflow could be moved from builtins.c into
> it as well (and also benefit not only from accurate string
> lengths but also from the more accurate object size info).
> 
> What do you think about that?
Well, given that Wrestrict is already a DOM walk integrating it with
sprintf and strlen would be fairly natural.   In that model we'd come up
with some clever name for the pass.  It'd have an embedded strlen (and
perhaps objsize) analysis.  It's walker would dispatch each statement to
the restrict or sprintf & strlen checker.

> 
> Martin
> 
> PS I don't think I could do more than merger strlen and sprintf
> before stage 1 ends (if even that much) so this would be a longer
> term goal.
Given the time lost to the STRING_CST and range issues, agreed.  It's
not going to happen this stage1.

Jeff

Re: [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Richard Biener]

On Tue, Sep 18, 2018 at 8:58 PM Jeff Law <law@redhat.com> wrote:
>
> On 9/18/18 11:12 AM, Martin Sebor wrote:
>
> >> My bad.  Sigh. CCP doesn't track copies, just constants, so there's not
> >> going to be any data structure you can exploit.  And I don't think
> >> there's a value number you can use to determine the two objects are the
> >> same.
> >>
> >> Hmm, let's back up a bit, what is does the relevant part of the IL look
> >> like before CCP?  Is the real problem here that we have unpropagated
> >> copies lying around in the IL?  Hmm, more likely the IL looksl ike:
> >>
> >>    _8 = &pb_3(D)->a;
> >>    _9 = _8;
> >>    _1 = _9;
> >>    strncpy (MEM_REF (&pb_3(D)->a), ...);
> >>    MEM[(struct S *)_1].a[n_7] = 0;
> >
> > Yes, that is what the folder sees while the strncpy call is
> > being transformed/folded by ccp.  The MEM_REF is folded just
> > after the strncpy call and that's when it's transformed into
> >
> >   MEM[(struct S *)_8].a[n_7] = 0;
> >
> > (The assignments to _1 and _9 don't get removed until after
> > the dom walk finishes).
> >
> >>
> >> If we were to propagate the copies out we'd at best have:
> >>
> >>    _8 = &pb_3(D)->a;
> >>    strncpy (MEM_REF (&pb_3(D)->a), ...);
> >>    MEM[(struct S *)_8].a[n_7] = 0;
> >>
> >>
> >> Is that in a form you can handle?  Or would we also need to forward
> >> propagate the address computation into the use of _8?
> >
> > The above works as long as we look at the def_stmt of _8 in
> > the MEM_REF (we currently don't).  That's also what the last
> > iteration of the loop does.  In this case (with _8) it would
> > be discovered in the first iteration, so the loop could be
> > replaced by a simple if statement.
> >
> > But I'm not sure I understand the concern with the loop.  Is
> > it that we are looping at all, i.e., the cost?  Or that ccp
> > is doing something wrong or suboptimal? (Should have
> > propagated the value of _8 earlier?)
> I suspect it's more a concern that things like copies are typically
> propagated away.   So their existence in the IL (and consequently your
> need to handle them) raises the question "has something else failed to
> do its job earlier".
>
> During which of the CCP passes is this happening?  Can we pull the
> warning out of the folder (even if that means having a distinct warning
> pass over the IL?)

Note CCP also propagates copies.

Richard.

>
> Jeff