Re: types for VR_VARYING

[Aldy Hernandez <aldyh at redhat dot com>]

On 8/23/19 4:27 PM, Martin Sebor wrote:
> On 8/15/19 10:06 AM, Aldy Hernandez wrote:
> >
>
> Hey Aldy,
>
> After enabling EVRP for the strlen pass (as part of the sprintf
> integration) I get a SEGV in the return statement in the function
> below.  Backing out this change gets rid of the ICE and lets my
> tests pass, but I have no idea what the root cause of the SEGV
> might be.  The only mildly suspicious thing is the assertion in
> the function:
>
> @@ -355,9 +369,7 @@ value_range_base::singleton_p (tree *result) const
>   tree
>   value_range_base::type () const
>   {
> -  /* Types are only valid for VR_RANGE and VR_ANTI_RANGE, which are
> -     known to have non-zero min/max.  */
> -  gcc_assert (min ());
> +  gcc_assert (m_min || undefined_p ());
>     return TREE_TYPE (min ());
>   }

type() should really be:

tree
value_range_base::type () const
{
  gcc_assert (m_min);
  return TREE_TYPE (min ());
}

I should post a patch to fix this.  However, UNDEF should never have a 
type, so the assert will fail anyhow.

The code asking for the type of an UNDEF is wrong.

> *this looks like so:
>
>    (gdb) p *this
>    $55 = {m_kind = VR_UNDEFINED, m_min = 0x0, m_max = 0x0}
>
> so the assertion passes but the dererefence in TREE_TYPE fails.
>
> The test case is trivial:
>
>    void g (const char *a, const char *b)
>    {
>      if (__builtin_memcmp (a, b, 8))
>        __builtin_abort ();
>    }
>
> The ICE happens when updating the range for the second statement
> below:
>
>    _1 = __builtin_memcmp (a_3(D), b_4(D), 8);
>    _1 = (int) _8;
>
> Any ideas?
>
> Martin
>
>
> during GIMPLE pass: strlen
> u.c: In function ‘g’:
> u.c:1:6: internal compiler error: Segmentation fault
>      1 | void g (const char *a, const char *b)
>        |      ^
> 0x11c4d08 crash_signal
>      /src/gcc/svn/gcc/toplev.c:326
> 0x815519 contains_struct_check(tree_node*, tree_node_structure_enum, 
> char const*, int, char const*)
>      /src/gcc/svn/gcc/tree.h:3376
> 0x15e9391 value_range_base::type() const
>      /src/gcc/svn/gcc/tree-vrp.c:373
> 0x16bdad6 vr_values::update_value_range(tree_node const*, value_range*)
>      /src/gcc/svn/gcc/vr-values.c:237

According to your backtrace, it looks like the call to type comes from here:

      /* Do not allow transitions up the lattice.  The following
         is slightly more awkward than just new_vr->type < old_vr->type
         because VR_RANGE and VR_ANTI_RANGE need to be considered
         the same.  We may not have is_new when transitioning to
         UNDEFINED.  If old_vr->type is VARYING, we shouldn't be
         called, if we are anyway, keep it VARYING.  */
      if (old_vr->varying_p ())
        {
          new_vr->set_varying (new_vr->type ());
          is_new = false;
        }

So new_vr is UNDEFINED and we're asking for it's type.  That should 
probably be:

new_vr->set_varying (TREE_TYPE (var));

Would you mind testing the attached patch?

If it passes, feel free to commit it as obvious.

Thanks for catching this.

Aldy

diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 8067f8560cd..97a82e6ee24 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -369,7 +369,7 @@ value_range_base::singleton_p (tree *result) const
 tree
 value_range_base::type () const
 {
-  gcc_assert (m_min || undefined_p ());
+  gcc_assert (m_min);
   return TREE_TYPE (min ());
 }
 
diff --git a/gcc/vr-values.c b/gcc/vr-values.c
index 6f9a3612931..96c764c987b 100644
--- a/gcc/vr-values.c
+++ b/gcc/vr-values.c
@@ -234,7 +234,7 @@ vr_values::update_value_range (const_tree var, value_range *new_vr)
 	 called, if we are anyway, keep it VARYING.  */
       if (old_vr->varying_p ())
 	{
-	  new_vr->set_varying (new_vr->type ());
+	  new_vr->set_varying (TREE_TYPE (var));
 	  is_new = false;
 	}
       else if (new_vr->undefined_p ())