On 07/31/18 05:51, Martin Sebor wrote:
> On 07/30/2018 03:11 PM, Bernd Edlinger wrote:
>> Hi,
>>
>>> @@ -621,6 +674,12 @@ c_strlen (tree src, int only_value)
>>> maxelts = maxelts / eltsize - 1;
>>> }
>>>
>>> + /* Unless the caller is prepared to handle it by passing in a non-null
>>> + ARR, fail if the terminating nul doesn't fit in the array the string
>>> + is stored in (as in const char a[3] = "123"; */
>>> + if (!arr && maxelts < strelts)
>>> + return NULL_TREE;
>>> +
>>
>> this is c_strlen, how is the caller ever supposed to handle non-zero
>> terminated strings???
>> especially if you do this above?
>
> Callers that pass in a non-null ARR handle them by issuing
> a warning. The rest get back a null result. It should be
> evident from the rest of the patch. It can be debated what
> each caller should do when it detects such a missing nul
> where one is expected. Different approaches may be more
> or less appropriate for different callers/functions (e.g.,
> strcpy vs strlen).
>
Sorry, right in the beginning you have "if (!add) arr = arrs;"
>>> +c_strlen (tree src, int only_value, tree *arr /* = NULL */)
>>> {
>>> STRIP_NOPS (src);
>>> +
>>> + /* Used to detect non-nul-terminated strings in subexpressions
>>> + of a conditional expression. When ARR is null, point it at
>>> + one of the elements for simplicity. */
>>> + tree arrs[] = { NULL_TREE, NULL_TREE };
>>> + if (!arr)
>>> + arr = arrs;
>>
>>> @@ -11427,7 +11478,9 @@ string_constant (tree arg, tree *ptr_offset)
>>> unsigned HOST_WIDE_INT length = TREE_STRING_LENGTH (init);
>>> length = string_length (TREE_STRING_POINTER (init), charsize,
>>> length / charsize);
>>> - if (compare_tree_int (array_size, length + 1) < 0)
>>> + if (nulterm)
>>> + *nulterm = array_elts > length;
>>> + else if (array_elts <= length)
>>> return NULL_TREE;
>>
>> I don't understand why you can't use
>> compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (init)), TREE_STRING_LENGTH
>> (init))
>> instead of this convoluted code above ???
>>
>> Sorry, this patch does not look like it is ready any time soon.
>
> I'm open to technical comments on the substance of my changes
> but I'm not interested in your opinion of the readiness of
> the patch (whatever that might mean), certainly not if you
> have formed it after skimming a random handful of lines out
> of a 600 line patch.
>
Sorry, again. I just meant you should fix the issues, and
maybe make the patch a bit smaller.
>> But actually I am totally puzzled by your priorities.
>> This is what I see right now:
>>
>> 1) We have missing warnings.
>> 2) We have wrong code bugs.
>> 3) We have apparently a specification error on the C Language standard (*)
>>
>>
>> Why are you prioritizing 1) over 2) thus blocking my attempts to fix a wrong
>> code
>> issue,and why do you not tackle 3) in your WG14?
>
> My priorities are none of your concern.
>
Sorry, again, but your priorities seem to conflict with mine.
> Your "attempts to fix" issues interfere with my work on a number
> of projects. You are not being helpful -- instead, by submitting
> changes that you know fully well conflict with mine, you are
> impeding and undermining my work. That is why I object to them.
>
>> (*) which means that GCC is currently removing code from assertions
>> as I pointed out here:
>> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01695.html
>>
>> This happens because GCC follows the language standards literally right now.
>>
>> I would say too literally, and it proves that the language standard's logic
>> is
>> flawed IMHO.
>
> I have no idea what your point is about standards, but bugs
> like the one in the example, including those arising from
> uninitialized arrays, could be detected with only minor
> enhancements to the tree-ssa-strlen pass. Implementing some
> of this is among the projects I'm expected and expecting to
> work on for GCC 9. This patch is a small step in that
> direction.
>
> If you care about detecting bugs I would expect you to be
> supportive rather than dismissive of this work, and helpful
> in bringing it to fruition rather that putting it down or
> questioning my priorities. Especially since the work was
> prompted by your own (valid) complaint that GCC doesn't
> diagnose them.
>
You don't really listen to what I am saying, I did not say
that we need another warning instead of fixing the wrong
optimization issue at hand.
But I am in good company, you don't listen to Jakub and Richi
either.
Bernd.
> Martin
>
