public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Martin Sebor <msebor@gmail.com>
To: Renlin Li <renlin.li@foss.arm.com>, Renlin Li <renlin.li@arm.com>,
	Christophe Lyon <christophe.lyon@linaro.org>
Cc: Andreas Schwab <schwab@linux-m68k.org>, Jeff Law <law@redhat.com>,
	Jakub Jelinek <jakub@redhat.com>,
	Joseph Myers <joseph@codesourcery.com>,
	Gcc Patch List <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] have -Wformat-overflow handle -fexec-charset (PR 80503)
Date: Thu, 01 Feb 2018 18:27:00 -0000	[thread overview]
Message-ID: <99546592-a9eb-3c31-c566-03e30165af23@gmail.com> (raw)
In-Reply-To: <297883fa-9e00-0be8-d3d0-1dd654677bb8@foss.arm.com>

On 02/01/2018 04:54 AM, Renlin Li wrote:
> Hi Martin,
>
>
> On 01/02/18 00:40, Martin Sebor wrote:
>> On 01/31/2018 10:36 AM, Renlin Li wrote:
>>> Hi there,
>>>
>>> I have a patch to fix to regressions we observed in armhf native
>>> environment.
>>>
>>> To effectively check out of range for format string, a target type
>>> should be
>>> used. And according to the standard, int type is used for "width" and
>>> "precision"
>>> field of format string.
>>>
>>> Here target_strtol10 is rename to target_strtoi10, and fixed to use
>>> target_int_max () which is target dependent.
>>>
>>> The value returned by target_strtol10 is (target_int_max () + 1) when it
>>> exceeds the range.
>>> This is used to indicate its value exceeds target INT_MAX for the later
>>> warning.
>>
>> Sorry for not responding to your original email. It's still
>> in my inbox, just buried under a pile of stuff.
>>
>> Using LONG_MAX is not ideal but unless I missed something
>> (it's been a while) I think using INT_MAX for the target would
>> be even less optimal.  Unless specified by the asterisk, width
>> and precision can be arbitrarily large.
>
> I cannot find more document about this other than here:
> http://en.cppreference.com/w/c/io/fprintf

The best reference is the C standard.  The most recent publicly
available draft of C11 is here:
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf

The next best (and sometimes better) reference is POSIX:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html

>> (optional) integer value or * that specifies minimum field width.
>> (optional) . followed by integer number or *, or neither that
>> specifies precision of the conversion.
>
> It only mentions a integer value, with no type. I assume it could be of
> type int?
> It is indeed very unclear about the range of width and precision.

Because the spec doesn't say what precision the number is
to be interpreted in, it must be taken to mean a number with
an arbitrary precision.

>> constraining either to INT_MAX would trigger warnings on LP64
>> targets for safe calls like:
>>
>>    // INT_MAX is 2147483647
>>    sprintf (d, "%.2147483648s", "");
>>
>> I think we want to use the maximum value of an unsigned type
>> with the greatest precision on the host.
>  It will still warn
>> for directives with precisions in excess of HOST_WIDE_INT_MAX
>
> Is it here a target type should be used to test the range against?
> Instead of the
> host where the toolchain is built?

I think for the purposes of runtime evaluation by a conforming
implementation, both the width and the precision need to be
interpreted "as if" with infinite precision.

But after sleeping on it, I suppose that for the purposes of
the warning, it could be helpful to point out values that exceed
even INT_MAX on the host (rather than target) because they aren't
very meaningful and are most likely mistakes (e.g., hidden by
macro expansion).

It might still be useful to have the pass store the larger number
(up to its limit, whatever that is on the host), and use it for
its internal computation and subsequent diagnostics for accuracy.

Martin

>
> Regards,
> Renlin
>
>> but short of using something like offset_int it's probably as
>> good as it's going to get.  (It has been suggested that
>> the whole pass would benefit from using offset_int to track
>> large numbers so if/when it's enhanced to make this change
>> it should also make the code behave more consistently across
>> different hosts.)
>
>
>
>>
>> Martin
>>
>>>
>>> Is it Okay to commit?
>>>
>>> Regards,
>>> Renlin
>>>
>>> gcc/ChangeLog:
>>>
>>> 2018-01-31  Renlin Li  <renlin.li@arm.com>
>>>
>>>     * gimple-ssa-sprintf.c (target_strtol10): Use target integer to
>>> check
>>>     the range. Rename it into ....
>>>     (target_strtoi10): This.
>>>     (parse_directive): Compare with target int max instead of LONG_MAX.
>>>
>>>
>>> On 20/06/17 12:00, Renlin Li wrote:
>>>> Hi Martin,
>>>>
>>>> I did a little investigation into this. Please correct me if I missed
>>>> anything.
>>>>
>>>> I build a native arm-linux-gnueabihf toolchain in armhf hardware.
>>>> It's ILP32. So in this situation:
>>>>
>>>> HOST_WIDE_INT is long, which is 32-bit.
>>>> integer type 32-bit as well, so target_int_max () == LONG_MAX
>>>>
>>>>
>>>> gimple-ssa-sprintf.c line 2887
>>>>>   /* Has the likely and maximum directive output exceeded INT_MAX?  */
>>>>>   bool likelyximax = *dir.beg && res->range.likely > target_int_max
>>>>> ();
>>>>
>>>> likelyximax will be false as the latter expression is always false.
>>>> res->range.likely is truncated to LONG_MAX (in target_strtol10
>>>> function)
>>>>
>>>> I have checked in cross build environment (host x86_64), this variable
>>>> is true.
>>>>
>>>> Regards,
>>>> Renlin
>>>>
>>>>
>>>> On 13/06/17 09:16, Renlin Li wrote:
>>>>> Hi Martin,
>>>>>
>>>>> On 04/06/17 23:24, Martin Sebor wrote:
>>>>>> On 06/02/2017 09:38 AM, Renlin Li wrote:
>>>>>>> Hi Martin,
>>>>>>>
>>>>>>> After r247444, I saw the following two regressions in
>>>>>>> arm-linux-gnueabihf environment:
>>>>>>>
>>>>>>> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-18.c  (test for warnings,
>>>>>>> line 119)
>>>>>>> PASS: gcc.dg/tree-ssa/builtin-sprintf-warn-18.c  (test for warnings,
>>>>>>> line 121)
>>>>>>> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-18.c  (test for warnings,
>>>>>>> line 121)
>>>>>>>
>>>>>>> The warning message related to those two lines are:
>>>>>>> testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-18.c:119:3: warning:
>>>>>>> '%9223372036854775808i' directive width out of range
>>>>>>> [-Wformat-overflow=]
>>>>>>>
>>>>>>> testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-18.c:121:3: warning:
>>>>>>> '%.9223372036854775808i' directive precision out of range
>>>>>>> [-Wformat-overflow=]
>>>>>>>
>>>>>>> testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-18.c:121:3: warning:
>>>>>>> '%.9223372036854775808i' directive precision out of range
>>>>>>> [-Wformat-overflow=]
>>>>>>>
>>>>>>> Did you notice similar things from your test environment,
>>>>>>> Christophe?
>>>>>>
>>>>>> Looks like you're missing a couple of warnings.  I see the following
>>>>>> output with both my arm-linux-gnueabihf cross compiler and my native
>>>>>> x86_64 GCC, both in 32-bit and 64-bit modes, as expected by the test,
>>>>>> so I don't see the same issue in my environment.
>>>>>
>>>>> Yes, it happens on arm-linux-gnueabihf native environment. the
>>>>> warnings with "INT_MAX"
>>>>> line are missing. I don't know if the host environment will cause the
>>>>> difference.
>>>>>
>>>>> Regards,
>>>>> Renlin
>>

      reply	other threads:[~2018-02-01 18:27 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-26 22:27 Martin Sebor
2017-04-27  3:39 ` Joseph Myers
2017-04-27  5:07   ` Jakub Jelinek
2017-04-27 22:52     ` Martin Sebor
2017-04-28 16:35       ` Jeff Law
2017-04-28 16:37         ` Jakub Jelinek
2017-04-28 17:05           ` Jeff Law
2017-04-28 18:32         ` Martin Sebor
2017-04-28 19:14           ` Jeff Law
2017-04-29 20:44           ` Andreas Schwab
2017-05-03 14:35             ` Christophe Lyon
2017-05-03 15:02               ` Martin Sebor
2017-05-03 15:24                 ` Christophe Lyon
2017-06-02 15:38                   ` Renlin Li
2017-06-04 22:24                     ` Martin Sebor
2017-06-13  8:16                       ` Renlin Li
2017-06-20 11:00                         ` Renlin Li
2018-01-31 17:57                           ` Renlin Li
2018-02-01  0:41                             ` Martin Sebor
2018-02-01 11:54                               ` Renlin Li
2018-02-01 18:27                                 ` Martin Sebor [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=99546592-a9eb-3c31-c566-03e30165af23@gmail.com \
    --to=msebor@gmail.com \
    --cc=christophe.lyon@linaro.org \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jakub@redhat.com \
    --cc=joseph@codesourcery.com \
    --cc=law@redhat.com \
    --cc=renlin.li@arm.com \
    --cc=renlin.li@foss.arm.com \
    --cc=schwab@linux-m68k.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).