This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] fix a couple of bugs in const string folding (PR 86532)


On Thu, 19 Jul 2018, Martin Sebor wrote:

> Here's one more update with tweaks addressing a couple more
> of Bernd's comments:
> 
> 1) correct the use of TREE_STRING_LENGTH() where a number of
> array elements is expected and not bytes
> 2) set CHARTYPE as soon as it's first determined rather than
> trying to extract it again later

Please look at Bernds followup comments.  One additional note:

I see you are ultimatively using CHARTYPE to get at the size
of the access.  That is wrong.

   if (TREE_CODE (arg) == ADDR_EXPR)
     {
+      tree argtype = TREE_TYPE (arg);
+      chartype = argtype;
+
       arg = TREE_OPERAND (arg, 0);
       tree ref = arg;
       if (TREE_CODE (arg) == ARRAY_REF)
        {

so the "access" is of size array_ref_element_size (arg) here.  You
may not simply use TYPE_SIZE_UNIT of sth.

That is, lookign at current trunk,

  if (TREE_CODE (arg) == ADDR_EXPR)
    {
      arg = TREE_OPERAND (arg, 0);
      tree ref = arg;
      if (TREE_CODE (arg) == ARRAY_REF)
        {
          tree idx = TREE_OPERAND (arg, 1);
          if (TREE_CODE (idx) != INTEGER_CST)
            {
              /* Extract the variable index to prevent
                 get_addr_base_and_unit_offset() from failing due to
                 it.  Use it later to compute the non-constant offset
                 into the string and return it to the caller.  */
              varidx = idx;
              ref = TREE_OPERAND (arg, 0);

you should scale the index here by array_ref_element_size (arg).
Or simply rewrite this to instead of using get_addr_base_and_unit_offset,
use get_inner_reference which does all that magic for you.

That is, you shouldn't need chartype.

Richard.


> On 07/19/2018 01:49 PM, Martin Sebor wrote:
> > On 07/19/2018 01:17 AM, Richard Biener wrote:
> > > On Wed, 18 Jul 2018, Martin Sebor wrote:
> > > 
> > > > > > > +      while (TREE_CODE (chartype) != INTEGER_TYPE)
> > > > > > > +    chartype = TREE_TYPE (chartype);
> > > > > > This is a bit concerning.  First under what conditions is chartype
> > > > > > not
> > > > > > going to be an INTEGER_TYPE?  And under what conditions will
> > > > > > extracting
> > > > > > its type ultimately lead to something that is an INTEGER_TYPE?
> > > > > 
> > > > > chartype is usually (maybe even always) pointer type here:
> > > > > 
> > > > >   const char a[] = "123";
> > > > >   extern int i;
> > > > >   n = strlen (&a[i]);
> > > > 
> > > > But your hunch was correct that the loop isn't safe because
> > > > the element type need not be an integer (I didn't know/forgot
> > > > that the function is called for non-strings too).  The loop
> > > > should be replaced by:
> > > > 
> > > >       while (TREE_CODE (chartype) == ARRAY_TYPE
> > > >          || TREE_CODE (chartype) == POINTER_TYPE)
> > > >     chartype = TREE_TYPE (chartype);
> > > 
> > > As this function may be called "late" you need to cope with
> > > the middle-end ignoring type changes and thus happily
> > > passing int *** directly rather than (char *) of that.
> > > 
> > > Also doesn't the above yield int for int *[]?
> > 
> > I don't think it ever gets this far for either a pointer to
> > an array of int, or for an array of pointers to int.  So for
> > something like the following the function fails earlier:
> > 
> >   const int* const a[2] = { ... };
> >   const char* (const *p)[2] = &a;
> > 
> >   int f (void)
> >   {
> >     return __builtin_memcmp (*p, "12345678", 8);
> >   }
> > 
> > (Assuming this is what you were asking about.)
> > 
> > > I guess you really want
> > > 
> > >    if (POINTER_TYPE_P (chartype))
> > >      chartype = TREE_TYPE (chartype);
> > >    while (TREE_CODE (chartype) == ARRAY_TYPE)
> > >      chartype = TREE_TYPE (chartype);
> > > 
> > > ?
> > 
> > That seems to work too.  Attached is an update with this tweak.
> > The update also addresses some of Bernd's comments: it removes
> > the pointless second test in:
> > 
> >     if (TREE_CODE (type) == ARRAY_TYPE
> >         && TREE_CODE (type) != INTEGER_TYPE)
> > 
> > the unused assignment to chartype in:
> > 
> >    else if (DECL_P (arg))
> >      {
> >        array = arg;
> >        chartype = TREE_TYPE (arg);
> >      }
> > 
> > and calls string_constant() instead of strnlen() to compute
> > the length of a generic string.
> > 
> > Other improvements  are possible in this area but they are
> > orthogonal to the bug I'm trying to fix so I'll post separate
> > patches for some of those.
> > 
> > Martin
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]