This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Improve checks in c_strlen (PR 87053)
On 08/24/18 07:58, Jeff Law wrote:
> On 08/23/2018 03:27 AM, Bernd Edlinger wrote:
>> On 08/22/18 18:28, Martin Sebor wrote:
>>> On 08/22/2018 08:41 AM, Bernd Edlinger wrote:
>>>> Hi!
>>>>
>>>>
>>>> This patch adds some more checks to c_getstr to fix PR middle-end/87053
>>>> wrong code bug.
>>>>
>>>> Unfortunately this patch alone is not sufficient to fix the problem,
>>>> but also the patch for PR 86714 that hardens c_getstr is necessary
>>>> to prevent the wrong folding.
>>>>
>>>>
>>>> Bootstrapped and reg-tested on top of my PR 86711/86714 patch.
>>>> Is it OK for trunk?
>>>
>>> This case is also the subject of the patch I submitted back in
>>> July for 86711/86714 and 86552. With it, GCC avoid folding
>>> the strlen call early and warns for the missing nul:
>>>
>>> warning: ‘__builtin_strlen’ argument missing terminating nul [-Wstringop-overflow=]
>>> if (__builtin_strlen (u.z) != 7)
>>> ^~~~~~~~~~~~~~~~
>>>
>>> The patch doesn't doesn't prevent all such strings from being
>>> folded and it eventually lets fold_builtin_strlen() do its thing:
>>>
>>> /* To avoid warning multiple times about unterminated
>>> arrays only warn if its length has been determined
>>> and is being folded to a constant. */
>>> if (nonstr)
>>> warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr);
>>>
>>> return fold_convert_loc (loc, type, len);
>>>
>>> Handling this case is a matter of avoiding the folding here as
>>> well and moving the warning later.
>>>
>>> Since my patch is still in the review queue and does much more
>>> than just prevent folding of non-nul terminated arrays it should
>>> be reviewed first.
>>>
>>
>> Hmmm, now you made me curious.
>>
>> So I tried to install your patch (I did this on r263508
>> since it does not apply to trunk, one thing I noted is
>> that part 4 and part 3 seem to create gcc/testsuite/gcc.dg/warn-strcpy-no-nul.c
>> I did not check if they are identical or not).
>>
>> So I tried the test case from this PR on the compiler built with your patch:
>>
>> $ cat cat pr87053.c
>> /* PR middle-end/87053 */
>>
>> const union
>> { struct {
>> char x[4];
>> char y[4];
>> };
>> struct {
>> char z[8];
>> };
>> } u = {{"1234", "567"}};
>>
>> int main ()
>> {
>> if (__builtin_strlen (u.z) != 7)
>> __builtin_abort ();
>> }
>> $ gcc -S pr87053.c
>> pr87053.c: In function 'main':
>> pr87053.c:15:7: warning: '__builtin_strlen' argument missing terminating nul [-Wstringop-overflow=]
>> 15 | if (__builtin_strlen (u.z) != 7)
>> | ^~~~~~~~~~~~~~~~
>> pr87053.c:11:3: note: referenced argument declared here
>> 11 | } u = {{"1234", "567"}};
>> | ^
>> $ cat pr87053.s
>> .file "pr87053.c"
>> .text
>> .globl u
>> .section .rodata
>> .align 8
>> .type u, @object
>> .size u, 8
>> u:
>> .ascii "1234"
>> .string "567"
>> .text
>> .globl main
>> .type main, @function
>> main:
>> .LFB0:
>> .cfi_startproc
>> pushq %rbp
>> .cfi_def_cfa_offset 16
>> .cfi_offset 6, -16
>> movq %rsp, %rbp
>> .cfi_def_cfa_register 6
>> call abort
>> .cfi_endproc
>> .LFE0:
>> .size main, .-main
>> .ident "GCC: (GNU) 9.0.0 20180813 (experimental)"
>> .section .note.GNU-stack,"",@progbits
>>
>>
>> So we get a warning, and still wrong code.
>>
>> That is the reason why I think this patch of yours adds
>> confusion by trying to fix everything in one step.
>>
>> And I would like you to think of ways how to solve
>> a problem step by step.
>>
>> And at this time, sorry, we should restore correctness issues.
>> And fix wrong-code issues.
>> If possible without breaking existing warnings, yes.
>> But no new warnings, sorry again.
> Just a note, Martin's most fix for 86711/86714 fixes codegen issues
> without breaking existing warnings or adding new warnings. The new
> warnings were broken out into follow-up patches.
>
BTW: the warning about u.z not null terminated is bogus.
There are middle-end consistency and correctness issues all over.
They have IMO precedence even over wrong-code issues.
Bernd.