* [PATCH] gimple-ssa-sprintf: Fix typo in range check
@ 2024-07-25 23:48 Siddhesh Poyarekar
2024-07-26 17:11 ` Jakub Jelinek
0 siblings, 1 reply; 4+ messages in thread
From: Siddhesh Poyarekar @ 2024-07-25 23:48 UTC (permalink / raw)
To: gcc-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gimple-ssa-sprintf: Fix typo in range check
2024-07-25 23:48 [PATCH] gimple-ssa-sprintf: Fix typo in range check Siddhesh Poyarekar
@ 2024-07-26 17:11 ` Jakub Jelinek
2024-07-26 17:39 ` Siddhesh Poyarekar
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2024-07-26 17:11 UTC (permalink / raw)
To: Siddhesh Poyarekar; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gimple-ssa-sprintf: Fix typo in range check
2024-07-26 17:11 ` Jakub Jelinek
@ 2024-07-26 17:39 ` Siddhesh Poyarekar
2024-07-26 18:19 ` Jakub Jelinek
0 siblings, 1 reply; 4+ messages in thread
From: Siddhesh Poyarekar @ 2024-07-26 17:39 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gimple-ssa-sprintf: Fix typo in range check
2024-07-26 17:39 ` Siddhesh Poyarekar
@ 2024-07-26 18:19 ` Jakub Jelinek
0 siblings, 0 replies; 4+ messages in thread
From: Jakub Jelinek @ 2024-07-26 18:19 UTC (permalink / raw)
To: Siddhesh Poyarekar; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-07-26 18:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-25 23:48 [PATCH] gimple-ssa-sprintf: Fix typo in range check Siddhesh Poyarekar
2024-07-26 17:11 ` Jakub Jelinek
2024-07-26 17:39 ` Siddhesh Poyarekar
2024-07-26 18:19 ` Jakub Jelinek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).