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 <ja...@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 <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
