Re: [PATCH] Improve checks in c_strlen (PR 87053)

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

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.


Bernd.