* [PATCH] Fix up --enable-checking=fold (PR middle-end/89503)
@ 2019-03-01 7:54 Jakub Jelinek
2019-03-01 8:49 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2019-03-01 7:54 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
Hi!
Some of the builtin folding sets TREE_NO_WARNING flags, which triggers as
--enable-checking=fold errors. I think we should allow setting of
TREE_NO_WARNING flag when folding, so this patch arranges that.
Tested on:
../configure --enable-languages=c,c++,fortran --enable-checking=fold --disable-bootstrap
make -jN && make -jN -k check
No extra FAILs compared to normal regtest. Ok for trunk?
2019-03-01 Jakub Jelinek <jakub@redhat.com>
PR middle-end/89503
* fold-const.c (fold_checksum_tree): Ignore TREE_NO_WARNING bit
on DECL_P and EXPR_P.
--- gcc/fold-const.c.jj 2019-02-21 00:01:05.714931643 +0100
+++ gcc/fold-const.c 2019-02-27 12:27:38.749353680 +0100
@@ -12130,6 +12130,7 @@ fold_checksum_tree (const_tree expr, str
memcpy ((char *) &buf, expr, tree_size (expr));
SET_DECL_ASSEMBLER_NAME ((tree)&buf, NULL);
buf.decl_with_vis.symtab_node = NULL;
+ buf.base.nowarning_flag = 0;
expr = (tree) &buf;
}
else if (TREE_CODE_CLASS (code) == tcc_type
@@ -12155,6 +12156,13 @@ fold_checksum_tree (const_tree expr, str
TYPE_CACHED_VALUES (tmp) = NULL;
}
}
+ else if (TREE_NO_WARNING (expr) && (DECL_P (expr) || EXPR_P (expr)))
+ {
+ /* Allow TREE_NO_WARNING to be set. */
+ memcpy ((char *) &buf, expr, tree_size (expr));
+ buf.base.nowarning_flag = 0;
+ expr = (tree) &buf;
+ }
md5_process_bytes (expr, tree_size (expr), ctx);
if (CODE_CONTAINS_STRUCT (code, TS_TYPED))
fold_checksum_tree (TREE_TYPE (expr), ctx, ht);
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix up --enable-checking=fold (PR middle-end/89503)
2019-03-01 7:54 [PATCH] Fix up --enable-checking=fold (PR middle-end/89503) Jakub Jelinek
@ 2019-03-01 8:49 ` Richard Biener
2019-03-01 9:11 ` Jakub Jelinek
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2019-03-01 8:49 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Fri, 1 Mar 2019, Jakub Jelinek wrote:
> Hi!
>
> Some of the builtin folding sets TREE_NO_WARNING flags, which triggers as
> --enable-checking=fold errors. I think we should allow setting of
> TREE_NO_WARNING flag when folding, so this patch arranges that.
>
> Tested on:
> ../configure --enable-languages=c,c++,fortran --enable-checking=fold --disable-bootstrap
> make -jN && make -jN -k check
> No extra FAILs compared to normal regtest. Ok for trunk?
Hmm, I guess it's OK though for EXPR_P I think we may want the
TREE_NO_WARNING setter to unshare the tree? Because IIRC it is
exactly because of tree sharing that we do this kind of checking?
Richard.
> 2019-03-01 Jakub Jelinek <jakub@redhat.com>
>
> PR middle-end/89503
> * fold-const.c (fold_checksum_tree): Ignore TREE_NO_WARNING bit
> on DECL_P and EXPR_P.
>
> --- gcc/fold-const.c.jj 2019-02-21 00:01:05.714931643 +0100
> +++ gcc/fold-const.c 2019-02-27 12:27:38.749353680 +0100
> @@ -12130,6 +12130,7 @@ fold_checksum_tree (const_tree expr, str
> memcpy ((char *) &buf, expr, tree_size (expr));
> SET_DECL_ASSEMBLER_NAME ((tree)&buf, NULL);
> buf.decl_with_vis.symtab_node = NULL;
> + buf.base.nowarning_flag = 0;
> expr = (tree) &buf;
> }
> else if (TREE_CODE_CLASS (code) == tcc_type
> @@ -12155,6 +12156,13 @@ fold_checksum_tree (const_tree expr, str
> TYPE_CACHED_VALUES (tmp) = NULL;
> }
> }
> + else if (TREE_NO_WARNING (expr) && (DECL_P (expr) || EXPR_P (expr)))
> + {
> + /* Allow TREE_NO_WARNING to be set. */
> + memcpy ((char *) &buf, expr, tree_size (expr));
> + buf.base.nowarning_flag = 0;
> + expr = (tree) &buf;
> + }
> md5_process_bytes (expr, tree_size (expr), ctx);
> if (CODE_CONTAINS_STRUCT (code, TS_TYPED))
> fold_checksum_tree (TREE_TYPE (expr), ctx, ht);
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix up --enable-checking=fold (PR middle-end/89503)
2019-03-01 8:49 ` Richard Biener
@ 2019-03-01 9:11 ` Jakub Jelinek
2019-03-01 9:18 ` Richard Biener
2019-03-06 18:00 ` Martin Sebor
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Jelinek @ 2019-03-01 9:11 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
On Fri, Mar 01, 2019 at 09:49:03AM +0100, Richard Biener wrote:
> On Fri, 1 Mar 2019, Jakub Jelinek wrote:
> > Some of the builtin folding sets TREE_NO_WARNING flags, which triggers as
> > --enable-checking=fold errors. I think we should allow setting of
> > TREE_NO_WARNING flag when folding, so this patch arranges that.
> >
> > Tested on:
> > ../configure --enable-languages=c,c++,fortran --enable-checking=fold --disable-bootstrap
> > make -jN && make -jN -k check
> > No extra FAILs compared to normal regtest. Ok for trunk?
>
> Hmm, I guess it's OK though for EXPR_P I think we may want the
> TREE_NO_WARNING setter to unshare the tree? Because IIRC it is
> exactly because of tree sharing that we do this kind of checking?
Maybe, I just wonder if it wouldn't be too expensive.
These TREE_NO_WARNING sets are typically on arguments of builtin functions
so we'd need to unshare the argument (shallow copy) as well as the whole
builtin. Some spots (usually older) in builtins.c do create new trees,
but some TREE_NO_WARNING setting is hidden even in APIs like c_strlen
which actually doesn't return the argument. Plus, many of those functions
that set it are used both during expansion (where supposedly we don't care
about modifying in place) and folding. What a mess?
So, do you prefer to drop this patch, or just take the || EXPR_P (expr)
part away from it and let Martin (who has added pretty much all of these
TREE_NO_WARNING) fix it later? c_strlen is probably the oldest with this
issue and most likely very hard to change; the thing is, it is usually used
during expansion and we heavily rely in that case that it modifies it
in-place and thus doesn't warn multiple times. Just use it also in
fold_builtin_strlen :(.
> > 2019-03-01 Jakub Jelinek <jakub@redhat.com>
> >
> > PR middle-end/89503
> > * fold-const.c (fold_checksum_tree): Ignore TREE_NO_WARNING bit
> > on DECL_P and EXPR_P.
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix up --enable-checking=fold (PR middle-end/89503)
2019-03-01 9:11 ` Jakub Jelinek
@ 2019-03-01 9:18 ` Richard Biener
2019-03-06 18:00 ` Martin Sebor
1 sibling, 0 replies; 6+ messages in thread
From: Richard Biener @ 2019-03-01 9:18 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Fri, 1 Mar 2019, Jakub Jelinek wrote:
> On Fri, Mar 01, 2019 at 09:49:03AM +0100, Richard Biener wrote:
> > On Fri, 1 Mar 2019, Jakub Jelinek wrote:
> > > Some of the builtin folding sets TREE_NO_WARNING flags, which triggers as
> > > --enable-checking=fold errors. I think we should allow setting of
> > > TREE_NO_WARNING flag when folding, so this patch arranges that.
> > >
> > > Tested on:
> > > ../configure --enable-languages=c,c++,fortran --enable-checking=fold --disable-bootstrap
> > > make -jN && make -jN -k check
> > > No extra FAILs compared to normal regtest. Ok for trunk?
> >
> > Hmm, I guess it's OK though for EXPR_P I think we may want the
> > TREE_NO_WARNING setter to unshare the tree? Because IIRC it is
> > exactly because of tree sharing that we do this kind of checking?
>
> Maybe, I just wonder if it wouldn't be too expensive.
> These TREE_NO_WARNING sets are typically on arguments of builtin functions
> so we'd need to unshare the argument (shallow copy) as well as the whole
> builtin. Some spots (usually older) in builtins.c do create new trees,
> but some TREE_NO_WARNING setting is hidden even in APIs like c_strlen
> which actually doesn't return the argument. Plus, many of those functions
> that set it are used both during expansion (where supposedly we don't care
> about modifying in place) and folding. What a mess?
Uh.
> So, do you prefer to drop this patch, or just take the || EXPR_P (expr)
> part away from it and let Martin (who has added pretty much all of these
> TREE_NO_WARNING) fix it later? c_strlen is probably the oldest with this
> issue and most likely very hard to change; the thing is, it is usually used
> during expansion and we heavily rely in that case that it modifies it
> in-place and thus doesn't warn multiple times. Just use it also in
> fold_builtin_strlen :(.
No, as said the patch is probably OK (setting TREE_NO_WARNING is harmless
from a tree-sharing perspective but still undesired IMHO).
Maybe you can add a comment reflecting the above and open a bug so we
can eventually revisit this in future.
Thanks,
Richard.
> > > 2019-03-01 Jakub Jelinek <jakub@redhat.com>
> > >
> > > PR middle-end/89503
> > > * fold-const.c (fold_checksum_tree): Ignore TREE_NO_WARNING bit
> > > on DECL_P and EXPR_P.
>
> Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix up --enable-checking=fold (PR middle-end/89503)
2019-03-01 9:11 ` Jakub Jelinek
2019-03-01 9:18 ` Richard Biener
@ 2019-03-06 18:00 ` Martin Sebor
2019-03-06 18:38 ` Jakub Jelinek
1 sibling, 1 reply; 6+ messages in thread
From: Martin Sebor @ 2019-03-06 18:00 UTC (permalink / raw)
To: Jakub Jelinek, Richard Biener; +Cc: gcc-patches
On 3/1/19 2:11 AM, Jakub Jelinek wrote:
> On Fri, Mar 01, 2019 at 09:49:03AM +0100, Richard Biener wrote:
>> On Fri, 1 Mar 2019, Jakub Jelinek wrote:
>>> Some of the builtin folding sets TREE_NO_WARNING flags, which triggers as
>>> --enable-checking=fold errors. I think we should allow setting of
>>> TREE_NO_WARNING flag when folding, so this patch arranges that.
>>>
>>> Tested on:
>>> ../configure --enable-languages=c,c++,fortran --enable-checking=fold --disable-bootstrap
>>> make -jN && make -jN -k check
>>> No extra FAILs compared to normal regtest. Ok for trunk?
>>
>> Hmm, I guess it's OK though for EXPR_P I think we may want the
>> TREE_NO_WARNING setter to unshare the tree? Because IIRC it is
>> exactly because of tree sharing that we do this kind of checking?
>
> Maybe, I just wonder if it wouldn't be too expensive.
> These TREE_NO_WARNING sets are typically on arguments of builtin functions
> so we'd need to unshare the argument (shallow copy) as well as the whole
> builtin. Some spots (usually older) in builtins.c do create new trees,
> but some TREE_NO_WARNING setting is hidden even in APIs like c_strlen
> which actually doesn't return the argument. Plus, many of those functions
> that set it are used both during expansion (where supposedly we don't care
> about modifying in place) and folding. What a mess?
>
> So, do you prefer to drop this patch, or just take the || EXPR_P (expr)
> part away from it and let Martin (who has added pretty much all of these
> TREE_NO_WARNING) fix it later?
I read through this thread but I'm not sure I understand the problem
(or really the tree sharing business -- different GIMPLE statements
sharing the same tree node?)
Is the problem here that the checking computes a checksum of the bits
in a tree node before folding and compares it to a checksum after
folding and setting the NO_WARNING bit in between causes the checking
to fail?
If that's accurate, if this patch is not the right fix, what would be?
Martin
> c_strlen is probably the oldest with this
> issue and most likely very hard to change; the thing is, it is usually used
> during expansion and we heavily rely in that case that it modifies it
> in-place and thus doesn't warn multiple times. Just use it also in
> fold_builtin_strlen :(.
>
>>> 2019-03-01 Jakub Jelinek <jakub@redhat.com>
>>>
>>> PR middle-end/89503
>>> * fold-const.c (fold_checksum_tree): Ignore TREE_NO_WARNING bit
>>> on DECL_P and EXPR_P.
>
> Jakub
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix up --enable-checking=fold (PR middle-end/89503)
2019-03-06 18:00 ` Martin Sebor
@ 2019-03-06 18:38 ` Jakub Jelinek
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Jelinek @ 2019-03-06 18:38 UTC (permalink / raw)
To: Martin Sebor; +Cc: Richard Biener, gcc-patches
On Wed, Mar 06, 2019 at 10:15:33AM -0700, Martin Sebor wrote:
> I read through this thread but I'm not sure I understand the problem
> (or really the tree sharing business -- different GIMPLE statements
> sharing the same tree node?)
>
> Is the problem here that the checking computes a checksum of the bits
> in a tree node before folding and compares it to a checksum after
> folding and setting the NO_WARNING bit in between causes the checking
> to fail?
>
> If that's accurate, if this patch is not the right fix, what would be?
The --enable-checking=fold is an attempt to find bugs in fold and friends.
Unlike the gimplifier, which is destructive on the trees passed to it (and
that is why e.g. function bodies are unshared before being passed to the
gimplifier), fold is invoked often by the FE and trees can be heavily shared
at that point, so if fold modifies the arguments passed to it rather than
creating new trees, it could modify some other tree in the same (or another)
function. fold_checksum_tree has some exceptions on what is allowed to
change, e.g. the various caches of types etc.
My patch has treated the TREE_NO_WARNING flag similarly to that, as an
exception, but Richard expressed the opinion that perhaps that is not the
right thing and we should instead unshare trees containing it if we want to
set TREE_NO_WARNING. That would mean either don't warn from within the
folder (e.g. warn during gimplification or gimple-fold instead), or unshare.
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-06 18:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-01 7:54 [PATCH] Fix up --enable-checking=fold (PR middle-end/89503) Jakub Jelinek
2019-03-01 8:49 ` Richard Biener
2019-03-01 9:11 ` Jakub Jelinek
2019-03-01 9:18 ` Richard Biener
2019-03-06 18:00 ` Martin Sebor
2019-03-06 18:38 ` Jakub Jelinek
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).