public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Bernd Edlinger <bernd.edlinger@hotmail.de>
Cc: Richard Biener <rguenther@suse.de>,
	       "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
	       Jeff Law <law@redhat.com>
Subject: Re: [PATCH] Fix ICU miscompilation due to tree-ssa-strlen.c bug (PR tree-optimization/88693)
Date: Sat, 05 Jan 2019 12:31:00 -0000	[thread overview]
Message-ID: <20190105123123.GG30353@tucnak> (raw)
In-Reply-To: <DB7PR07MB5353D9B8A6B44444959138E4E48F0@DB7PR07MB5353.eurprd07.prod.outlook.com>

On Sat, Jan 05, 2019 at 12:20:24PM +0000, Bernd Edlinger wrote:
> co-incidentally my "Make strlen range computations more conservative" patch
> contained a fix on the same spot, I just did not have a test case for it:
> 
> @@ -3184,7 +3146,10 @@ get_min_string_length (tree rhs, bool *f
>        && TREE_READONLY (rhs))
>      rhs = DECL_INITIAL (rhs);
> 
> -  if (rhs && TREE_CODE (rhs) == STRING_CST)
> +  if (rhs && TREE_CODE (rhs) == STRING_CST
> +      && tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (rhs)))) == 1
> +      && TREE_STRING_LENGTH (rhs) > 0
> +      && TREE_STRING_POINTER (rhs) [TREE_STRING_LENGTH (rhs) - 1] == '\0')
>      {
>        *full_string_p = true;
>        return strlen (TREE_STRING_POINTER (rhs));
> 
> 
> additionally to your patch this tests the string is in fact a single-byte string.
> since strlen returns garbage otherwise.

Multi-byte STRING_CSTs seem to be stored in the target byte order, e.g.
L"abcde" in a x86_64-linux -> powerpc64-linux cross is still
"\000\000\000a\000\000\000b\000\000\000c\000\000\000d\000\000\000\000"
so I think strlen is exactly what we want.  The tree-ssa-strlen.c pass
doesn't track string lengths in whatever units it has, it tracks number of
non-zero bytes followed by zero byte known at certain address, or, if there
is no zero byte known, the minimum amount of non-zero bytes known at certain
address.

And, your patch has also the bad effect that it won't return any length for
the strings that aren't zero terminated.  We do want to return 9 for the
"abcdefghi" string without zero terminator at the end, just need to set
*full_string_p to false.  And, for "abcd\000fghi" string without zero
termination at the end, we do want to return 4 and set *full_string_p to
true.

	Jakub

  reply	other threads:[~2019-01-05 12:31 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-05 12:20 Bernd Edlinger
2019-01-05 12:31 ` Jakub Jelinek [this message]
2019-01-05 13:17   ` Bernd Edlinger
2019-01-05 13:23     ` Jakub Jelinek
  -- strict thread matches above, loose matches on Subject: below --
2019-01-04 22:54 Jakub Jelinek
2019-01-05  1:10 ` Jeff Law
2019-01-11 18:29 ` Jeff Law

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=20190105123123.GG30353@tucnak \
    --to=jakub@redhat.com \
    --cc=bernd.edlinger@hotmail.de \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=law@redhat.com \
    --cc=rguenther@suse.de \
    /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).