On 8/26/19 12:19 PM, Martin Sebor wrote:
> On 8/26/19 11:41 AM, Jeff Law wrote:
>> I think we need to go back and revisit this hunk:
>>
>>> /* If the offset is known to be out of bounds, warn, and call
>>> strlen at
>>> runtime. */
>>> if (eltoff < 0 || eltoff >= maxelts)
>>> {
>>> /* Suppress multiple warnings for propagated constant
>>> strings. */
>>> if (only_value != 2
>>> && !TREE_NO_WARNING (arg)
>>> && warning_at (loc, OPT_Warray_bounds,
>>> "offset %qwi outside bounds of constant
>>> string",
>>> eltoff))
>>> {
>>> if (decl)
>>> inform (DECL_SOURCE_LOCATION (decl), "%qE declared
>>> here", decl);
>>> TREE_NO_WARNING (arg) = 1;
>>> }
>>> return NULL_TREE;
>>> }
>>
>> The problem we have is this is warning inside a utility routine that can
>> be called many times.
>>
>> ARG is often going to be an ADDR_EXPR that wraps a _DECL node. So we'll
>> end up setting TREE_NO_WARNING on the ADDR_EXPR node. At first glance
>> that seems fine, but it's really not sufficient to avoid duplicate
>> warnings.
>>
>> Consider that we might call get_range_strlen for ARG again later in the
>> optimizer pipeline. That will call the overloaded get_range_strlen
>> which has the VISITED/RKIND arguments via:
>>
>>> bool
>>> get_range_strlen (tree arg, c_strlen_data *pdata, unsigned eltsize)
>>> {
>>> bitmap visited = NULL;
>>>
>>> if (!get_range_strlen (arg, &visited, SRK_LENRANGE, pdata, eltsize))
>>
>> That overload will call get_range_strlen_tree via:
>>
>>> static bool
>>> get_range_strlen (tree arg, bitmap *visited,
>>> strlen_range_kind rkind,
>>> c_strlen_data *pdata, unsigned eltsize)
>>> {
>>>
>>> if (TREE_CODE (arg) != SSA_NAME)
>>> return get_range_strlen_tree (arg, visited, rkind, pdata, eltsize);
>>
>> get_range_strlen_tree in turn calls c_strlen which may return NULL which
>> results in stripping the ADDR_EXPR and calling get_range_strlen again
>> via:
>>
>>> else
>>> {
>>> c_strlen_data lendata = { };
>>> val = c_strlen (arg, 1, &lendata, eltsize);
>>>
>>> if (!val && lendata.decl)
>>> {
>>> /* ARG refers to an unterminated const character array.
>>> DATA.DECL with size DATA.LEN. */
>>> val = lendata.minlen;
>>> pdata->decl = lendata.decl;
>>> }
>>> }
>>>
>>> if (!val && rkind == SRK_LENRANGE)
>>> {
>>> if (TREE_CODE (arg) == ADDR_EXPR)
>>> return get_range_strlen (TREE_OPERAND (arg, 0), visited, rkind,
>>> pdata, eltsize);
>>
>> Note that we've stripped the ADDR_EXPR before the call to
>> get_range_strlen here. So we have no way to know we've already warned
>> on this expression and boom it's trivial to get multiple warnings simply
>> by asking for the range of a node.
>>
>> This is a great example of why warning from folders and other routines
>> that may be called more than once (directly or indirectly) is a bad idea.
>>
>>
>> I think you need to find another place for this warning, preferably
>> outside the folders and their utility routines.
>
> I didn't add the warning, just a test that exercises it. I don't
> think it was being tested before. AFAICS, the warning itself goes
> back to r5379 from 1993. The patch the bits above are quoted from
> just let it detect a few more instances of out-of-bounds offsets
> than it did before: in references to flexible members of declared
> objects.
You're right. Your tweaks in this code were just adding a reference
back to the offending _DECL. My apologies.
I think I've got a bit of a workaround/hack in place. Essentially
falling back to c_strlen with a suitable ONLY_VALUE parameter for cases
that are likely going to tickle this issue.
It's a bit ironic since the warning actually comes from c_strlen. But
the path by which get_range_strlen gets there isn't terribly convenient
for passing down a suitable "don't warn" parameter like ONLY_VALUE.
Another approach would be to just always use c_strlen, but c_strlen is a
lot less powerful than get_range_strlen.
>
> I agree that warning from low-level utility functions can lead
> to duplicates or false positives. Other utility functions deal
> with it either by adding a flag to request a warning, or some
> other parameter to let the caller know of the problem and let
> it handle it.
Yea. I pondered that a bit. I think that's working around a
fundamental implementation problem, namely that we shouldn't be warning
in the folder or a routine like c_strlen to begin with. Passing down
the relevant flag through all the intertwined functions is likely to be
painful. This might argue that c_strlen_data should be an inout which
would make this more feasible (and if we go that direction, eltsize
probably should move into the structure as well).
We could look to pull the warning out of c_strlen, but that would mean
the callers would have to handle warning and bubbling out enough
information to make that possible, which looks painful.
I pondered setting TREE_NO_WARNING on the underlying _DECL, but that
means we'd just miss a warning if we had another call site (and thus
would have unique outer ADDR_EXPRs, but the same underlying _DECL node).
Overall it's a mess.
jeff
