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 <[email protected]>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
