* [PATCH] builtins: Fix up two bugs in access_ref::inform_access [PR98721]
@ 2021-01-20 7:52 Jakub Jelinek
2021-01-20 8:18 ` Richard Biener
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2021-01-20 7:52 UTC (permalink / raw)
To: Richard Biener, Jeff Law; +Cc: gcc-patches
Hi!
The following patch fixes two bugs in the access_ref::inform_access function
(plus some formatting nits).
The first problem is that ref can be various things, e.g. *_DECL, or
SSA_NAME, or IDENTIFIER_NODE. And allocfn is non-NULL only if ref is
(at least originally) an SSA_NAME initialized to the result of some
allocator function (but not e.g. __builtin_alloca_with_align which is
handled differently).
A few lines above the last hunk of this patch in builtins.c, the code uses
if (mode == access_read_write || mode == access_write_only)
{
if (allocfn == NULL_TREE)
{
if (*offstr)
inform (loc, "at offset %s into destination object %qE of size %s",
offstr, ref, sizestr);
else
inform (loc, "destination object %qE of size %s", ref, sizestr);
return;
}
if (*offstr)
inform (loc,
"at offset %s into destination object of size %s "
"allocated by %qE", offstr, sizestr, allocfn);
else
inform (loc, "destination object of size %s allocated by %qE",
sizestr, allocfn);
return;
}
so if allocfn is NULL, it prints whatever ref is, if it is non-NULL,
it prints instead the allocation function. But strangely the hunk
a few lines below wasn't consistent with that and instead printed the
first form only if DECL_P (ref) and would ICE if ref wasn't a decl but
still allocfn was NULL. Fixed by making it consistent what the code does
earlier.
Another bug is that the code earlier contains an ugly hack for VLAs and was
assuming that SSA_NAME_IDENTIFIER must be non-NULL on the lhs of
__builtin_alloca_with_align. While that is likely true for the cases where
the compiler emits this builtin for VLAs (and it will also be true that
the name of the VLA in that case can be taken from that identifier up to the
first .), the builtin is user accessible as the testcase shows, so one can
have any other SSA_NAME in there. I think it would be better to add some
more reliable way how to identify VLA names corresponding to
__builtin_alloca_with_align allocations, perhaps internal fn or whatever,
but that is beyond the scope of this patch.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2021-01-20 Jakub Jelinek <jakub@redhat.com>
PR tree-optimization/98721
* builtins.c (access_ref::inform_access): Don't assume
SSA_NAME_IDENTIFIER must be non-NULL. Print messages about
object whenever allocfn is NULL, rather than only when DECL_P
is true. Use %qE instead of %qD for that. Formatting fixes.
* gcc.dg/pr98721-1.c: New test.
* gcc.dg/pr98721-2.c: New test.
--- gcc/builtins.c.jj 2021-01-18 19:07:16.022895507 +0100
+++ gcc/builtins.c 2021-01-19 11:56:52.247070923 +0100
@@ -4414,8 +4414,8 @@ access_ref::inform_access (access_mode m
MAXREF on which the result is based. */
const offset_int orng[] =
{
- offrng[0] - maxref.offrng[0],
- wi::smax (offrng[1] - maxref.offrng[1], offrng[0]),
+ offrng[0] - maxref.offrng[0],
+ wi::smax (offrng[1] - maxref.offrng[1], offrng[0]),
};
/* Add the final PHI's offset to that of each of the arguments
@@ -4493,12 +4493,15 @@ access_ref::inform_access (access_mode m
/* Strip the SSA_NAME suffix from the variable name and
recreate an identifier with the VLA's original name. */
ref = gimple_call_lhs (stmt);
- ref = SSA_NAME_IDENTIFIER (ref);
- const char *id = IDENTIFIER_POINTER (ref);
- size_t len = strcspn (id, ".$");
- if (!len)
- len = strlen (id);
- ref = get_identifier_with_length (id, len);
+ if (SSA_NAME_IDENTIFIER (ref))
+ {
+ ref = SSA_NAME_IDENTIFIER (ref);
+ const char *id = IDENTIFIER_POINTER (ref);
+ size_t len = strcspn (id, ".$");
+ if (!len)
+ len = strlen (id);
+ ref = get_identifier_with_length (id, len);
+ }
}
else
{
@@ -4557,13 +4560,13 @@ access_ref::inform_access (access_mode m
return;
}
- if (DECL_P (ref))
+ if (allocfn == NULL_TREE)
{
if (*offstr)
- inform (loc, "at offset %s into source object %qD of size %s",
+ inform (loc, "at offset %s into source object %qE of size %s",
offstr, ref, sizestr);
else
- inform (loc, "source object %qD of size %s", ref, sizestr);
+ inform (loc, "source object %qE of size %s", ref, sizestr);
return;
}
--- gcc/testsuite/gcc.dg/pr98721-1.c.jj 2021-01-19 12:15:03.825600828 +0100
+++ gcc/testsuite/gcc.dg/pr98721-1.c 2021-01-19 12:14:24.730045488 +0100
@@ -0,0 +1,14 @@
+/* PR tree-optimization/98721 */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+int
+foo (int n)
+{
+ if (n <= 0)
+ {
+ char vla[n]; /* { dg-message "source object 'vla' of size 0" } */
+ return __builtin_strlen (vla); /* { dg-warning "'__builtin_strlen' reading 1 or more bytes from a region of size 0" } */
+ }
+ return -1;
+}
--- gcc/testsuite/gcc.dg/pr98721-2.c.jj 2021-01-19 12:00:16.005742548 +0100
+++ gcc/testsuite/gcc.dg/pr98721-2.c 2021-01-19 11:59:29.372275423 +0100
@@ -0,0 +1,8 @@
+/* PR tree-optimization/98721 */
+/* { dg-do compile } */
+
+int
+foo (void)
+{
+ return __builtin_strlen (__builtin_alloca_with_align (0, 16)); /* { dg-warning "'__builtin_strlen' reading 1 or more bytes from a region of size 0" } */
+} /* { dg-message "source object '<unknown>' of size 0" "" { target *-*-* } .-1 } */
Jakub
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] builtins: Fix up two bugs in access_ref::inform_access [PR98721]
2021-01-20 7:52 [PATCH] builtins: Fix up two bugs in access_ref::inform_access [PR98721] Jakub Jelinek
@ 2021-01-20 8:18 ` Richard Biener
0 siblings, 0 replies; 2+ messages in thread
From: Richard Biener @ 2021-01-20 8:18 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Jeff Law, gcc-patches
On Wed, 20 Jan 2021, Jakub Jelinek wrote:
> Hi!
>
> The following patch fixes two bugs in the access_ref::inform_access function
> (plus some formatting nits).
>
> The first problem is that ref can be various things, e.g. *_DECL, or
> SSA_NAME, or IDENTIFIER_NODE. And allocfn is non-NULL only if ref is
> (at least originally) an SSA_NAME initialized to the result of some
> allocator function (but not e.g. __builtin_alloca_with_align which is
> handled differently).
>
> A few lines above the last hunk of this patch in builtins.c, the code uses
> if (mode == access_read_write || mode == access_write_only)
> {
> if (allocfn == NULL_TREE)
> {
> if (*offstr)
> inform (loc, "at offset %s into destination object %qE of size %s",
> offstr, ref, sizestr);
> else
> inform (loc, "destination object %qE of size %s", ref, sizestr);
> return;
> }
>
> if (*offstr)
> inform (loc,
> "at offset %s into destination object of size %s "
> "allocated by %qE", offstr, sizestr, allocfn);
> else
> inform (loc, "destination object of size %s allocated by %qE",
> sizestr, allocfn);
> return;
> }
> so if allocfn is NULL, it prints whatever ref is, if it is non-NULL,
> it prints instead the allocation function. But strangely the hunk
> a few lines below wasn't consistent with that and instead printed the
> first form only if DECL_P (ref) and would ICE if ref wasn't a decl but
> still allocfn was NULL. Fixed by making it consistent what the code does
> earlier.
>
> Another bug is that the code earlier contains an ugly hack for VLAs and was
> assuming that SSA_NAME_IDENTIFIER must be non-NULL on the lhs of
> __builtin_alloca_with_align. While that is likely true for the cases where
> the compiler emits this builtin for VLAs (and it will also be true that
> the name of the VLA in that case can be taken from that identifier up to the
> first .), the builtin is user accessible as the testcase shows, so one can
> have any other SSA_NAME in there. I think it would be better to add some
> more reliable way how to identify VLA names corresponding to
> __builtin_alloca_with_align allocations, perhaps internal fn or whatever,
> but that is beyond the scope of this patch.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Thanks,
Richard.
> 2021-01-20 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/98721
> * builtins.c (access_ref::inform_access): Don't assume
> SSA_NAME_IDENTIFIER must be non-NULL. Print messages about
> object whenever allocfn is NULL, rather than only when DECL_P
> is true. Use %qE instead of %qD for that. Formatting fixes.
>
> * gcc.dg/pr98721-1.c: New test.
> * gcc.dg/pr98721-2.c: New test.
>
> --- gcc/builtins.c.jj 2021-01-18 19:07:16.022895507 +0100
> +++ gcc/builtins.c 2021-01-19 11:56:52.247070923 +0100
> @@ -4414,8 +4414,8 @@ access_ref::inform_access (access_mode m
> MAXREF on which the result is based. */
> const offset_int orng[] =
> {
> - offrng[0] - maxref.offrng[0],
> - wi::smax (offrng[1] - maxref.offrng[1], offrng[0]),
> + offrng[0] - maxref.offrng[0],
> + wi::smax (offrng[1] - maxref.offrng[1], offrng[0]),
> };
>
> /* Add the final PHI's offset to that of each of the arguments
> @@ -4493,12 +4493,15 @@ access_ref::inform_access (access_mode m
> /* Strip the SSA_NAME suffix from the variable name and
> recreate an identifier with the VLA's original name. */
> ref = gimple_call_lhs (stmt);
> - ref = SSA_NAME_IDENTIFIER (ref);
> - const char *id = IDENTIFIER_POINTER (ref);
> - size_t len = strcspn (id, ".$");
> - if (!len)
> - len = strlen (id);
> - ref = get_identifier_with_length (id, len);
> + if (SSA_NAME_IDENTIFIER (ref))
> + {
> + ref = SSA_NAME_IDENTIFIER (ref);
> + const char *id = IDENTIFIER_POINTER (ref);
> + size_t len = strcspn (id, ".$");
> + if (!len)
> + len = strlen (id);
> + ref = get_identifier_with_length (id, len);
> + }
> }
> else
> {
> @@ -4557,13 +4560,13 @@ access_ref::inform_access (access_mode m
> return;
> }
>
> - if (DECL_P (ref))
> + if (allocfn == NULL_TREE)
> {
> if (*offstr)
> - inform (loc, "at offset %s into source object %qD of size %s",
> + inform (loc, "at offset %s into source object %qE of size %s",
> offstr, ref, sizestr);
> else
> - inform (loc, "source object %qD of size %s", ref, sizestr);
> + inform (loc, "source object %qE of size %s", ref, sizestr);
>
> return;
> }
> --- gcc/testsuite/gcc.dg/pr98721-1.c.jj 2021-01-19 12:15:03.825600828 +0100
> +++ gcc/testsuite/gcc.dg/pr98721-1.c 2021-01-19 12:14:24.730045488 +0100
> @@ -0,0 +1,14 @@
> +/* PR tree-optimization/98721 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +int
> +foo (int n)
> +{
> + if (n <= 0)
> + {
> + char vla[n]; /* { dg-message "source object 'vla' of size 0" } */
> + return __builtin_strlen (vla); /* { dg-warning "'__builtin_strlen' reading 1 or more bytes from a region of size 0" } */
> + }
> + return -1;
> +}
> --- gcc/testsuite/gcc.dg/pr98721-2.c.jj 2021-01-19 12:00:16.005742548 +0100
> +++ gcc/testsuite/gcc.dg/pr98721-2.c 2021-01-19 11:59:29.372275423 +0100
> @@ -0,0 +1,8 @@
> +/* PR tree-optimization/98721 */
> +/* { dg-do compile } */
> +
> +int
> +foo (void)
> +{
> + return __builtin_strlen (__builtin_alloca_with_align (0, 16)); /* { dg-warning "'__builtin_strlen' reading 1 or more bytes from a region of size 0" } */
> +} /* { dg-message "source object '<unknown>' of size 0" "" { target *-*-* } .-1 } */
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-01-20 8:18 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-20 7:52 [PATCH] builtins: Fix up two bugs in access_ref::inform_access [PR98721] Jakub Jelinek
2021-01-20 8:18 ` Richard Biener
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).