[PATCH] gimple-ssa-sprintf: Fix typo in range check

[PATCH] gimple-ssa-sprintf: Fix typo in range check

[Siddhesh Poyarekar]

The code to scale ranges for wide chars in format_string incorrectly
checks range.likely to scale range.unlikely, which is a copy-paste typo
from the immediate previous condition.

gcc/ChangeLog:

	gimple-ssa-sprintf.cc (format_string): Fix type in range check
	for UNLIKELY for wide chars.

Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
---
Tested on x86_64, no new testsuite regressions due to this fix.

 gcc/gimple-ssa-sprintf.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/gimple-ssa-sprintf.cc b/gcc/gimple-ssa-sprintf.cc
index 025b0fbff6f..0900710647c 100644
--- a/gcc/gimple-ssa-sprintf.cc
+++ b/gcc/gimple-ssa-sprintf.cc
@@ -2623,7 +2623,7 @@ format_string (const directive &dir, tree arg, pointer_query &ptr_qry)
 	  if (slen.range.likely < target_int_max ())
 	    slen.range.likely *= 2;
 
-	  if (slen.range.likely < target_int_max ())
+	  if (slen.range.unlikely < target_int_max ())
 	    slen.range.unlikely *= target_mb_len_max ();
 
 	  /* A non-empty wide character conversion may fail.  */
-- 
2.45.1

Re: [PATCH] gimple-ssa-sprintf: Fix typo in range check

[Jakub Jelinek]

On Thu, Jul 25, 2024 at 07:48:38PM -0400, Siddhesh Poyarekar wrote:
> The code to scale ranges for wide chars in format_string incorrectly
> checks range.likely to scale range.unlikely, which is a copy-paste typo
> from the immediate previous condition.
> 
> gcc/ChangeLog:
> 
> 	gimple-ssa-sprintf.cc (format_string): Fix type in range check
> 	for UNLIKELY for wide chars.
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>

LGTM.
What exactly the code really wants to do is unclear to me, what does
the INT_MAX on the target have to do with the minimum/maximum/expected
sizes of %S or %ls printed strings is unclear, target PTRDIFF_MAX
maybe.  And why it uses this
	  if (slen.range.something < target_int_max ())
	    slen.range.something *= something_else;
rather than say
	  slen.range.something
	    = MIN (slang.range.something * something_else, target_int_max ());
perhaps with some overflow checking is also something that is hard to guess.
In any case, I think your patch is a step in the right direction.

> --- a/gcc/gimple-ssa-sprintf.cc
> +++ b/gcc/gimple-ssa-sprintf.cc
> @@ -2623,7 +2623,7 @@ format_string (const directive &dir, tree arg, pointer_query &ptr_qry)
>  	  if (slen.range.likely < target_int_max ())
>  	    slen.range.likely *= 2;
>  
> -	  if (slen.range.likely < target_int_max ())
> +	  if (slen.range.unlikely < target_int_max ())
>  	    slen.range.unlikely *= target_mb_len_max ();
>  
>  	  /* A non-empty wide character conversion may fail.  */
> -- 
> 2.45.1

	Jakub

Re: [PATCH] gimple-ssa-sprintf: Fix typo in range check

[Siddhesh Poyarekar]

On 2024-07-26 13:11, Jakub Jelinek wrote:
> On Thu, Jul 25, 2024 at 07:48:38PM -0400, Siddhesh Poyarekar wrote:
>> The code to scale ranges for wide chars in format_string incorrectly
>> checks range.likely to scale range.unlikely, which is a copy-paste typo
>> from the immediate previous condition.
>>
>> gcc/ChangeLog:
>>
>> 	gimple-ssa-sprintf.cc (format_string): Fix type in range check
>> 	for UNLIKELY for wide chars.
>>
>> Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
> 
> LGTM.

Thanks, pushed.

> What exactly the code really wants to do is unclear to me, what does
> the INT_MAX on the target have to do with the minimum/maximum/expected
> sizes of %S or %ls printed strings is unclear, target PTRDIFF_MAX

I think that is because the printf family returns the number of 
bytes/chars written in an int, which imposes the INT_MAX limitation on 
the format string expansion.

> maybe.  And why it uses this
> 	  if (slen.range.something < target_int_max ())
> 	    slen.range.something *= something_else;
> rather than say
> 	  slen.range.something
> 	    = MIN (slang.range.something * something_else, target_int_max ());
> perhaps with some overflow checking is also something that is hard to guess.

That's what I tried first but I settled for the minimal change because I 
didn't want to dig in deeper than I had time for to at the moment. 
Further down it checks if MAX and UNLIKELY cross INT_MAX and then resets 
to INT_MAX, but that looks suspicious on, e.g. 32-bit targets.  The code 
could use some refactoring/cleanup.

Thanks,
Sid

Re: [PATCH] gimple-ssa-sprintf: Fix typo in range check

[Jakub Jelinek]

On Fri, Jul 26, 2024 at 01:39:04PM -0400, Siddhesh Poyarekar wrote:
> > What exactly the code really wants to do is unclear to me, what does
> > the INT_MAX on the target have to do with the minimum/maximum/expected
> > sizes of %S or %ls printed strings is unclear, target PTRDIFF_MAX
> 
> I think that is because the printf family returns the number of bytes/chars
> written in an int, which imposes the INT_MAX limitation on the format string
> expansion.

Ah, yes, that makes sense.

> > maybe.  And why it uses this
> > 	  if (slen.range.something < target_int_max ())
> > 	    slen.range.something *= something_else;
> > rather than say
> > 	  slen.range.something
> > 	    = MIN (slang.range.something * something_else, target_int_max ());
> > perhaps with some overflow checking is also something that is hard to guess.
> 
> That's what I tried first but I settled for the minimal change because I
> didn't want to dig in deeper than I had time for to at the moment. Further
> down it checks if MAX and UNLIKELY cross INT_MAX and then resets to INT_MAX,
> but that looks suspicious on, e.g. 32-bit targets.  The code could use some
> refactoring/cleanup.

I think the counters are HOST_WIDE_INT or unsigned HOST_WIDE_INT, so always
64-bit.

	Jakub