Re: [PATCH] avoid warning on constant strncpy until next statement is reachable (PR 87028)

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Aug 30, 2018 at 2:27 AM Jeff Law <law@redhat.com> wrote:
>
> On 08/28/2018 06:12 PM, Martin Sebor wrote:
> >>> Sadly, dstbase is the PARM_DECL for d.  That's where things are going
> >>> "wrong".  Not sure why you're getting the PARM_DECL in that case.  I'd
> >>> debug get_addr_base_and_unit_offset to understand what's going on.
> >>> Essentially you're getting different results of
> >>> get_addr_base_and_unit_offset in a case where they arguably should be
> >>> the same.
> >>
> >> Probably get_attr_nonstring_decl has the same "mistake" and returns
> >> the PARM_DECL instead of the SSA name pointer.  So we're comparing
> >> apples and oranges here.
> >
> > Returning the SSA_NAME_VAR from get_attr_nonstring_decl() is
> > intentional but the function need not (perhaps should not)
> > also set *REF to it.
> >
> >>
> >> Yeah:
> >>
> >> /* If EXPR refers to a character array or pointer declared attribute
> >>    nonstring return a decl for that array or pointer and set *REF to
> >>    the referenced enclosing object or pointer.  Otherwise returns
> >>    null.  */
> >>
> >> tree
> >> get_attr_nonstring_decl (tree expr, tree *ref)
> >> {
> >>   tree decl = expr;
> >>   if (TREE_CODE (decl) == SSA_NAME)
> >>     {
> >>       gimple *def = SSA_NAME_DEF_STMT (decl);
> >>
> >>       if (is_gimple_assign (def))
> >>         {
> >>           tree_code code = gimple_assign_rhs_code (def);
> >>           if (code == ADDR_EXPR
> >>               || code == COMPONENT_REF
> >>               || code == VAR_DECL)
> >>             decl = gimple_assign_rhs1 (def);
> >>         }
> >>       else if (tree var = SSA_NAME_VAR (decl))
> >>         decl = var;
> >>     }
> >>
> >>   if (TREE_CODE (decl) == ADDR_EXPR)
> >>     decl = TREE_OPERAND (decl, 0);
> >>
> >>   if (ref)
> >>     *ref = decl;
> >>
> >> I see a lot of "magic" here again in the attempt to "propagate"
> >> a nonstring attribute.
> >
> > That's the function's purpose: to look for the attribute.  Is
> > there a better way to do this?
> Well, there's a distinction between looking for the attribute (which
> will be on the _DECL node) and determining if the current instance (an
> SSA_NAME) has that attribute.
>
> What I think Richard is implying is that it might be better to propagate
> the state of the attribute to instances rather than going from an
> SSA_NAME backwards through the use-def chains or SSA_NAME_VAR to get to
> a potentially related _DECL node.
>
> This could be built into the alias oracle, or via a propagation engine.
> In either approach you should be able to cut down on false positives as
> well as false negatives.

It's more like the underlying decl of a SSA name doesn't guarantee you
the entity was originally related to that decl.

Maybe we're should be more strict here because we use the underlying
decl for debug info purposes.

Given there's really no semantic on the attribute but it just suppresses
warnings I'm OK with looking at the underlying decl.  Yes, propagating
would eventually improve things but it might be overkill at the same time
(just costing compile-time).

> >
> >> Note
> >>
> >> foo (char *p __attribute__(("nonstring")))
> >> {
> >>   p = "bar";
> >>   strlen (p); // or whatever is necessary to call get_attr_nonstring_decl
> >> }
> >>
> >> is perfectly valid and p as passed to strlen is _not_ nonstring(?).
> >
> > I don't know if you're saying that it should get a warning or
> > shouldn't.  Right now it doesn't because the strlen() call is
> > folded before we check for nonstring.
> >
> > I could see an argument for diagnosing it but I suspect you
> > wouldn't like it because it would mean more warning from
> > the folder.  I could also see an argument against it because,
> > as you said, it's safe.
> Well, this is where propagating the bit would help.  The assignment p =
> "bar" would clobber the nonstring property because we know "bar" is
> properly terminated. Pointer arithmetic, casts and the like would
> preserve the property and so on.
>
> If it were done via the aliasing oracle, the instance of P in the strlen
> call would be known to point to a proper string and thus the call is safe.
>
> Hope this helps...

So to elaborate a bit here - to propagate these kind of attributes
in PTA analysis (for example) you'd need to introduce fake
pointed-to objects (just special ids like nonlocal), nonstring
and string and have "sources" of those generate constraints.
After propagation finished you could then see whether an
SSA name points to either string or nonstring exclusively or
to both and set a bit in the pointer-info according to that
result.

It comes at the cost of increasing points-to bitmaps and
more constraints during propagation.

If you can do with just knowing whether any nonstring source
can be possibly pointed-to the effect on code not using that
attribute would be none.  Just be aware that with points-to
analysis this stuff leaks quite a bit since it is conservative
propagation (may point to nonstring) - separately tracking
may point to string allows you to get an idea of
"must point to nonstring".  But that comes at a cost.

A "must point to" propagator would be useful thing to have
as well I guess.  That would fit in a value-numbering kind
of framework.

Richard.

>
>
> Jeff