| Message ID | 20240725234838.3804003-1-siddhesh@gotplt.org |
|---|---|
| State | New |
| Headers | show |
| Series | gimple-ssa-sprintf: Fix typo in range check | expand |
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
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
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
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. */
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(-)