Question on ipa_ref->referring and ipa_ref->stmt on all_late_ipa_passes
Hi, I would like to use ipa_ref in the PASS_LIST all_late_ipa_passes to query the statement (ref->stmt) of where a global variable is used. However, I am having some problems achieving this. What I do is: 1. Check that ipa_ref->referring has a body and is not inlined. 2. get_body 3. try to print out the gimple statement using print_gimple_stmt (dump_file, ref->stmt, 0, TDF_NONE). This all seems correct to me, but I have been receiving errors that print is trying to print a tree with an incorrect TREE_CODE. I am assuming here that ref->stmt is not updated after all_regular_ipa_passes, much like how when looking at cgraph_edge the call statement is also not updated. Can someone please tell me if this is indeed the case or what is happening here? Also, while I think that the gimple statements might not be maintained, I see that ipa_ref is still used in the ipa_pta pass during all_late_ipa_passes. I see that ipa_ref->referring and ipa_ref->stmt are not used. Instead the tree of the referred is obtained in the following way: ref->referred->decl. I am assuming that it would be possible to use ref->referred->decl and search for this tree everywhere in referring to find the uses. Can someone confirm this? Thanks! -Erick
Re: Question on ipa_ref->referring and ipa_ref->stmt on all_late_ipa_passes
> Hi, > > I would like to use ipa_ref in the PASS_LIST all_late_ipa_passes to query > the statement (ref->stmt) of where a global variable is used. However, I am > having some problems achieving this. > > What I do is: > > 1. Check that ipa_ref->referring has a body and is not inlined. > 2. get_body > 3. try to print out the gimple statement using print_gimple_stmt > (dump_file, ref->stmt, 0, TDF_NONE). > > This all seems correct to me, but I have been receiving errors that print > is trying to print a tree with an incorrect TREE_CODE. I am assuming here > that ref->stmt is not updated after all_regular_ipa_passes, much like how > when looking at cgraph_edge the call statement is also not updated. Can > someone please tell me if this is indeed the case or what is happening here? Yes, while body materialization we keep cgraph edges up to date but we do not keep references. We probably should remove them earlier. Keeping them up to date would need relatively some work. We do so for calls since they hold important information (i.e. where to redirect the call). For references we don't have such machinery in place (even though I was thinking implementing it). Main difficulty is that inlining also performs statement folding that moves referneces around statements quite freely > > Also, while I think that the gimple statements might not be maintained, I > see that ipa_ref is still used in the ipa_pta pass during > all_late_ipa_passes. I see that ipa_ref->referring and ipa_ref->stmt are > not used. Instead the tree of the referred is obtained in the following > way: ref->referred->decl. I am assuming that it would be possible to use > ref->referred->decl and search for this tree everywhere in referring to > find the uses. Can someone confirm this? I will check what ipa-pta uses here. I suppose it works since you still have all references from the pre-IPA-transform stage, so it is consistent... Honza > > Thanks! > -Erick
Re: Question on ipa_ref->referring and ipa_ref->stmt on all_late_ipa_passes
On Mon, 14 Feb 2022 at 10:57, Jan Hubicka wrote: > > Hi, > > > > I would like to use ipa_ref in the PASS_LIST all_late_ipa_passes to query > > the statement (ref->stmt) of where a global variable is used. However, I > am > > having some problems achieving this. > > > > What I do is: > > > > 1. Check that ipa_ref->referring has a body and is not inlined. > > 2. get_body > > 3. try to print out the gimple statement using print_gimple_stmt > > (dump_file, ref->stmt, 0, TDF_NONE). > > > > This all seems correct to me, but I have been receiving errors that print > > is trying to print a tree with an incorrect TREE_CODE. I am assuming here > > that ref->stmt is not updated after all_regular_ipa_passes, much like how > > when looking at cgraph_edge the call statement is also not updated. Can > > someone please tell me if this is indeed the case or what is happening > here? > > Yes, while body materialization we keep cgraph edges up to date but we > do not keep references. We probably should remove them earlier. > Keeping them up to date would need relatively some work. We do so for > calls since they hold important information (i.e. where to redirect the > call). For references we don't have such machinery in place (even though > I was thinking implementing it). Main difficulty is that inlining also > performs statement folding that moves referneces around statements quite > freely > Hi Honza, just a bit of clarification here, when you are saying that references are not updated do you mean just ipa_ref->stmt or do you also include other things like ipa_ref->referring, ipa_ref->use? Thanks! > > > > Also, while I think that the gimple statements might not be maintained, I > > see that ipa_ref is still used in the ipa_pta pass during > > all_late_ipa_passes. I see that ipa_ref->referring and ipa_ref->stmt are > > not used. Instead the tree of the referred is obtained in the following > > way: ref->referred->decl. I am assuming that it would be possible to use > > ref->referred->decl and search for this tree everywhere in referring to > > find the uses. Can someone confirm this? > > I will check what ipa-pta uses here. I suppose it works since you still > have all references from the pre-IPA-transform stage, so it is > consistent... > > Honza > > > > Thanks! > > -Erick >
GSoC: Working on the static analyzer
Hello,
Mir Immad asked:
Should the analyzer warn for code like this "when open fails" (like strchr
does when 'strchr' returns NULL)
int fd = open("NOFILE", O_RDONLY);
write(fd, "a", 1);
because of the bad file descriptor.
unless it is written like this:
if (!errno)
write(fd, "a", 1);
My opinion is yes, in most cases. BTW, the write should fail for a
read-only file descriptor.
A case (on Linux) where a check is probably not needed: isint
fd=open("/proc/self/exe", O_RDONLY); or int fd=open ("/dev/random",
O_RDONLY); done *near the beginning* of main. There are only
pathological cases where they won't succeed. I suspect that except for
very critical executable, testing such failures is practically useless.
And your analyzer might start from https://github.com/bstarynk/bismon/
or use https://frama-c.com/
Re: GSoC: Working on the static analyzer
On 2/14/22 13:59, Basile Starynkevitch wrote:
Hello,
Mir Immad asked:
Should the analyzer warn for code like this "when open fails" (like strchr
does when 'strchr' returns NULL)
int fd = open("NOFILE", O_RDONLY);
write(fd, "a", 1);
because of the bad file descriptor.
unless it is written like this:
if (!errno)
write(fd, "a", 1);
My opinion is yes, in most cases. BTW, the write should fail for a
read-only file descriptor.
A case (on Linux) where a check is probably not needed: isint
fd=open("/proc/self/exe", O_RDONLY); or int fd=open ("/dev/random",
O_RDONLY); done *near the beginning* of main. There are only
pathological cases where they won't succeed. I suspect that except for
very critical executable, testing such failures is practically useless.
And your analyzer might start from https://github.com/bstarynk/bismon/
or use https://frama-c.com/ 😁 so don't expect
writing the ultimate, perfect, static source code (or Gimple code) analyzer.
Cheers
--
Basile Starynkevitch
(only mine opinions / les opinions sont miennes uniquement)
92340 Bourg-la-Reine, France
web page: starynkevitch.net/Basile/
Uninit warnings due to optimizing short-circuit conditionals
[CCing Mark in the hopes of insight from the valgrind side of things]
There is a false positive from -Wanalyzer-use-of-uninitialized-value on
gcc.dg/analyzer/pr102692.c here:
‘fix_overlays_before’: events 1-3
|
| 75 | while (tail
| |
| 76 | && (tem = make_lisp_ptr (tail, 5),
| | ^~
| | |
| | (1) following ‘false’ branch (when ‘tail’ is NULL)...
| 77 | (end = marker_position (XOVERLAY (tem)->end)) >= pos))
| | ~
|..
| 82 | if (!tail || end < prev || !tail->next)
| | ~~~
| | ||
| | |(3) use of uninitialized value ‘end’ here
| | (2) ...to here
|
The issue is that inner || of the conditionals have been folded within the
frontend from a chain of control flow:
5 │ if (tail == 0B) goto ; else goto ;
6 │ :
7 │ if (end < prev) goto ; else goto ;
8 │ :
9 │ _1 = tail->next;
10 │ if (_1 == 0B) goto ; else goto ;
11 │ :
to an OR expr (and then to a bitwise-or by the gimplifier):
5 │ _1 = tail == 0B;
6 │ _2 = end < prev;
7 │ _3 = _1 | _2;
8 │ if (_3 != 0) goto ; else goto ;
9 │ :
10 │ _4 = tail->next;
11 │ if (_4 == 0B) goto ; else goto ;
This happens for sufficiently simple conditionals in fold_truth_andor.
In particular, the (end < prev) is short-circuited without optimization,
but is evaluated with optimization, leading to the false positive.
Given how early this folding occurs, it seems the simplest fix is to
try to detect places where this optimization appears to have happened,
and suppress uninit warnings within the statement that would have
been short-circuited (and thus e.g. ignoring them when evaluating _2
above for the case where _1 is known to be true at the (_1 | _2) , and
thus _2 being redundant).
Attached is a patch that implements this.
There are some more details in the patch, but I'm wondering if this is a
known problem, and how e.g. valgrind copes with such code. My patch
feels like something of a hack, but I'm not sure of any other way around
it given that the conditional is folded directly within the frontend.
I've successfully bootstrapped & regrtested the patch on
x86_64-pc-linux-gnu; it fixes the false positive.
Thoughts? Thanks for reading
Dave
analyzer: fix uninit false +ve due to optimized conditionals [PR102692]
gcc/analyzer/ChangeLog:
PR analyzer/102692
* exploded-graph.h (impl_region_model_context::get_stmt): New.
* region-model.cc: Include "gimple-ssa.h", "tree-phinodes.h",
"tree-ssa-operands.h", and "ssa-iterators.h".
(within_short_circuited_stmt_p): New.
(region_model::check_for_poison): Don't warn about uninit values
if within_short_circuited_stmt_p.
* region-model.h (region_model_context::get_stmt): New vfunc.
(noop_region_model_context::get_stmt): New.
gcc/testsuite/ChangeLog:
PR analyzer/102692
* gcc.dg/analyzer/pr102692-2.c: New test.
* gcc.dg/analyzer/pr102692.c: Remove xfail. Remove -O2 from
options and move to...
* gcc.dg/analyzer/torture/pr102692.c: ...here.
Signed-off-by: David Malcolm
---
gcc/analyzer/exploded-graph.h | 2 +
gcc/analyzer/region-model.cc | 112 ++
gcc/analyzer/region-model.h | 5 +
gcc/testsuite/gcc.dg/analyzer/pr102692-2.c| 22
.../gcc.dg/analyzer/{ => torture}/pr102692.c | 4 +-
5 files changed, 143 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr102692-2.c
rename gcc/testsuite/gcc.dg/analyzer/{ => torture}/pr102692.c (94%)
diff --git a/gcc/analyzer/exploded-graph.h b/gcc/analyzer/exploded-graph.h
index 1854193c65b..1f52725dc98 100644
--- a/gcc/analyzer/exploded-graph.h
+++ b/gcc/analyzer/exploded-graph.h
@@ -90,6 +90,8 @@ class impl_region_model_context : public region_model_context
const state_machine **out_sm,
unsigned *out_sm_idx) FINAL OVERRIDE;
+ const gimple *get_stmt () const OVERRIDE { return m_stmt; }
+
exploded_graph *m_eg;
log_user m_logger;
exploded_node *m_enode_for_diag;
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index e659cf03a86..8a509c44178 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -68,6 +68,11 @@ along with GCC; see the file COPYING3. If not see
#include "stor-layout.h"
#include "attribs.h"
#include "tree-object-size.h"
+#include "gimple-ssa.h"
+#include "tree-phinodes.h"
+#include "tree-ssa-operands.h"
+#include "ssa-iterators.h"
+#include "gimple-pretty-print.h"
#if ENABLE_ANALYZER
@@ -829,6 +834,108 @@ region_model::get_g
Re: Uninit warnings due to optimizing short-circuit conditionals
On 2/14/2022 8:57 AM, David Malcolm via Gcc wrote: [CCing Mark in the hopes of insight from the valgrind side of things] There is a false positive from -Wanalyzer-use-of-uninitialized-value on gcc.dg/analyzer/pr102692.c here: ‘fix_overlays_before’: events 1-3 | | 75 | while (tail | | | 76 | && (tem = make_lisp_ptr (tail, 5), | | ^~ | | | | | (1) following ‘false’ branch (when ‘tail’ is NULL)... | 77 | (end = marker_position (XOVERLAY (tem)->end)) >= pos)) | | ~ |.. | 82 | if (!tail || end < prev || !tail->next) | | ~~~ | | || | | |(3) use of uninitialized value ‘end’ here | | (2) ...to here | The issue is that inner || of the conditionals have been folded within the frontend from a chain of control flow: 5 │ if (tail == 0B) goto ; else goto ; 6 │ : 7 │ if (end < prev) goto ; else goto ; 8 │ : 9 │ _1 = tail->next; 10 │ if (_1 == 0B) goto ; else goto ; 11 │ : to an OR expr (and then to a bitwise-or by the gimplifier): 5 │ _1 = tail == 0B; 6 │ _2 = end < prev; 7 │ _3 = _1 | _2; 8 │ if (_3 != 0) goto ; else goto ; 9 │ : 10 │ _4 = tail->next; 11 │ if (_4 == 0B) goto ; else goto ; This happens for sufficiently simple conditionals in fold_truth_andor. In particular, the (end < prev) is short-circuited without optimization, but is evaluated with optimization, leading to the false positive. Given how early this folding occurs, it seems the simplest fix is to try to detect places where this optimization appears to have happened, and suppress uninit warnings within the statement that would have been short-circuited (and thus e.g. ignoring them when evaluating _2 above for the case where _1 is known to be true at the (_1 | _2) , and thus _2 being redundant). Attached is a patch that implements this. There are some more details in the patch, but I'm wondering if this is a known problem, and how e.g. valgrind copes with such code. My patch feels like something of a hack, but I'm not sure of any other way around it given that the conditional is folded directly within the frontend. Presumably when "tail ==0", "end" is initialized somewhere? If so, yes, this is a known issue. There's a BZ about it somewhere (I don' t have the # handy, but it's probably on the Wuninitialized tracker). Jeff
Re: Uninit warnings due to optimizing short-circuit conditionals
Hi David, On Mon, 2022-02-14 at 10:57 -0500, David Malcolm wrote: > [CCing Mark in the hopes of insight from the valgrind side of things] Adding Julian to CC so he can correct me if I say something silly. > There is a false positive from -Wanalyzer-use-of-uninitialized-value on > gcc.dg/analyzer/pr102692.c here: > > ‘fix_overlays_before’: events 1-3 > | > | 75 | while (tail > | | > | 76 | && (tem = make_lisp_ptr (tail, 5), > | | ^~ > | | | > | | (1) following ‘false’ branch (when ‘tail’ is NULL)... > | 77 | (end = marker_position (XOVERLAY (tem)->end)) >= > pos)) > | | > ~ > |.. > | 82 | if (!tail || end < prev || !tail->next) > | | ~~~ > | | || > | | |(3) use of uninitialized value ‘end’ here > | | (2) ...to here > | > > The issue is that inner || of the conditionals have been folded within the > frontend from a chain of control flow: > >5 │ if (tail == 0B) goto ; else goto ; >6 │ : >7 │ if (end < prev) goto ; else goto ; >8 │ : >9 │ _1 = tail->next; > 10 │ if (_1 == 0B) goto ; else goto ; > 11 │ : > > to an OR expr (and then to a bitwise-or by the gimplifier): > >5 │ _1 = tail == 0B; >6 │ _2 = end < prev; >7 │ _3 = _1 | _2; >8 │ if (_3 != 0) goto ; else goto ; >9 │ : > 10 │ _4 = tail->next; > 11 │ if (_4 == 0B) goto ; else goto ; > > This happens for sufficiently simple conditionals in fold_truth_andor. > In particular, the (end < prev) is short-circuited without optimization, > but is evaluated with optimization, leading to the false positive. > > Given how early this folding occurs, it seems the simplest fix is to > try to detect places where this optimization appears to have happened, > and suppress uninit warnings within the statement that would have > been short-circuited (and thus e.g. ignoring them when evaluating _2 > above for the case where _1 is known to be true at the (_1 | _2) , and > thus _2 being redundant). > > Attached is a patch that implements this. > > There are some more details in the patch, but I'm wondering if this is a > known problem, and how e.g. valgrind copes with such code. My patch > feels like something of a hack, but I'm not sure of any other way around > it given that the conditional is folded directly within the frontend. As far as I know this is what valgrind memcheck also does with an bitwise or. It knows that _3 is defined and true if either _1 or _2 is defined and true. Or more generically that the result bits of a bitwise or are defined for those bits that are both defined or where one is defined and has the value 1. Cheers, Mark
Re: Uninit warnings due to optimizing short-circuit conditionals
On Mon, 2022-02-14 at 09:26 -0700, Jeff Law wrote:
>
>
> On 2/14/2022 8:57 AM, David Malcolm via Gcc wrote:
> > [CCing Mark in the hopes of insight from the valgrind side of things]
> >
> > There is a false positive from -Wanalyzer-use-of-uninitialized-value
> > on
> > gcc.dg/analyzer/pr102692.c here:
> >
> > ‘fix_overlays_before’: events 1-3
> > |
> > | 75 | while (tail
> > | |
> > | 76 | && (tem = make_lisp_ptr (tail, 5),
> > | | ^~
> > | | |
> > | | (1) following ‘false’ branch (when ‘tail’ is
> > NULL)...
> > | 77 | (end = marker_position (XOVERLAY (tem)-
> > >end)) >= pos))
> > | |
> > ~
> > |..
> > | 82 | if (!tail || end < prev || !tail->next)
> > | | ~ ~~
> > | | | |
> > | | | (3) use of uninitialized value ‘end’
> > here
> > | | (2) ...to here
> > |
> >
> > The issue is that inner || of the conditionals have been folded
> > within the
> > frontend from a chain of control flow:
> >
> > 5 │ if (tail == 0B) goto ; else goto ;
> > 6 │ :
> > 7 │ if (end < prev) goto ; else goto ;
> > 8 │ :
> > 9 │ _1 = tail->next;
> > 10 │ if (_1 == 0B) goto ; else goto ;
> > 11 │ :
> >
> > to an OR expr (and then to a bitwise-or by the gimplifier):
> >
> > 5 │ _1 = tail == 0B;
> > 6 │ _2 = end < prev;
> > 7 │ _3 = _1 | _2;
> > 8 │ if (_3 != 0) goto ; else goto ;
> > 9 │ :
> > 10 │ _4 = tail->next;
> > 11 │ if (_4 == 0B) goto ; else goto ;
> >
> > This happens for sufficiently simple conditionals in
> > fold_truth_andor.
> > In particular, the (end < prev) is short-circuited without
> > optimization,
> > but is evaluated with optimization, leading to the false positive.
> >
> > Given how early this folding occurs, it seems the simplest fix is to
> > try to detect places where this optimization appears to have
> > happened,
> > and suppress uninit warnings within the statement that would have
> > been short-circuited (and thus e.g. ignoring them when evaluating _2
> > above for the case where _1 is known to be true at the (_1 | _2) ,
> > and
> > thus _2 being redundant).
> >
> > Attached is a patch that implements this.
> >
> > There are some more details in the patch, but I'm wondering if this
> > is a
> > known problem, and how e.g. valgrind copes with such code. My patch
> > feels like something of a hack, but I'm not sure of any other way
> > around
> > it given that the conditional is folded directly within the frontend.
> Presumably when "tail ==0", "end" is initialized somewhere?
I don't think it does, but it shouldn't be a problem in the code as
written, due to short-circuiting (as I understand things) - but the
short-circuiting is being removed by the optimizer.
"end" only gets initialized at line 71 of the source below, for the
case where the initial value of "tail" is non-NULL:
63 │ void
64 │ fix_overlays_before (struct buffer *bp, long prev, long pos)
65 │ {
66 │ struct Lisp_Overlay *tail = bp->overlays_before, *parent = 0,
*right_pair;
67 │ struct lisp *tem;
68 │ long end;
69 │ while (tail
70 │ && (tem = make_lisp_ptr (tail, 5),
71 │ (end = marker_position (XOVERLAY (tem)->end)) >= pos))
72 │ {
73 │ parent = tail;
74 │ tail = tail->next;
75 │ }
76 │ if (!tail || end < prev || !tail->next) /* { dg-bogus "use of
uninitialized value 'end'" "uninit" } */
77 │ /* { dg-bogus "dereference of NULL 'tail'" "null deref" { target
*-*-* } .-1 } */
78 │ return;
At -O2 we have this at pr102692.c.022t.ssa:
59 │:
60 │ tail_23 = bp_22(D)->overlays_before;
61 │ parent_24 = 0B;
62 │ goto ; [INV]
63 │
64 │:
65 │ parent_32 = tail_11;
66 │ tail_33 = tail_11->next;
67 │
68 │:
69 │ # tail_11 = PHI
70 │ # parent_13 = PHI
71 │ # end_15 = PHI
72 │ if (tail_11 != 0B)
73 │ goto ; [INV]
74 │ else
75 │ goto ; [INV]
76 │
77 │:
78 │ tem_27 = make_lisp_ptr (tail_11, 5);
79 │ _1 = XOVERLAY (tem_27);
80 │ _2 = _1->end;
81 │ end_30 = marker_position (_2);
82 │ if (end_30 >= pos_31(D))
83 │ goto ; [INV]
84 │ else
85 │ goto ; [INV]
86 │
87 │:
88 │ # end_16 = PHI
89 │ _3 = tail_11 == 0B;
90 │ _4 = end_16 < prev_34(D);
91 │ _5 = _3 | _4;
92 │ if (_5 != 0)
93 │ goto ; [INV]
94 │ else
95 │ goto ; [INV]
96 │
97 │:
98 │ _6 = tail_11->next;
99 │ if (_6 == 0B)
100 │ goto ; [INV]
10
Re: Uninit warnings due to optimizing short-circuit conditionals
On Mon, 2022-02-14 at 17:57 +0100, Mark Wielaard wrote: > Hi David, > > On Mon, 2022-02-14 at 10:57 -0500, David Malcolm wrote: > > [CCing Mark in the hopes of insight from the valgrind side of > > things] > > Adding Julian to CC so he can correct me if I say something silly. > > > There is a false positive from -Wanalyzer-use-of-uninitialized- > > value on > > gcc.dg/analyzer/pr102692.c here: > > > > ‘fix_overlays_before’: events 1-3 > > | > > | 75 | while (tail > > | | > > | 76 | && (tem = make_lisp_ptr (tail, 5), > > | | ^~ > > | | | > > | | (1) following ‘false’ branch (when ‘tail’ is > > NULL)... > > | 77 | (end = marker_position (XOVERLAY (tem)- > > >end)) >= pos)) > > | | > > ~ > > |.. > > | 82 | if (!tail || end < prev || !tail->next) > > | | ~ ~~ > > | | | | > > | | | (3) use of uninitialized value > > ‘end’ here > > | | (2) ...to here > > | > > > > The issue is that inner || of the conditionals have been folded > > within the > > frontend from a chain of control flow: > > > > 5 │ if (tail == 0B) goto ; else goto ; > > 6 │ : > > 7 │ if (end < prev) goto ; else goto ; > > 8 │ : > > 9 │ _1 = tail->next; > > 10 │ if (_1 == 0B) goto ; else goto ; > > 11 │ : > > > > to an OR expr (and then to a bitwise-or by the gimplifier): > > > > 5 │ _1 = tail == 0B; > > 6 │ _2 = end < prev; > > 7 │ _3 = _1 | _2; > > 8 │ if (_3 != 0) goto ; else goto ; > > 9 │ : > > 10 │ _4 = tail->next; > > 11 │ if (_4 == 0B) goto ; else goto ; > > > > This happens for sufficiently simple conditionals in > > fold_truth_andor. > > In particular, the (end < prev) is short-circuited without > > optimization, > > but is evaluated with optimization, leading to the false positive. > > > > Given how early this folding occurs, it seems the simplest fix is > > to > > try to detect places where this optimization appears to have > > happened, > > and suppress uninit warnings within the statement that would have > > been short-circuited (and thus e.g. ignoring them when evaluating > > _2 > > above for the case where _1 is known to be true at the (_1 | _2) , > > and > > thus _2 being redundant). > > > > Attached is a patch that implements this. > > > > There are some more details in the patch, but I'm wondering if this > > is a > > known problem, and how e.g. valgrind copes with such code. My > > patch > > feels like something of a hack, but I'm not sure of any other way > > around > > it given that the conditional is folded directly within the > > frontend. > > As far as I know this is what valgrind memcheck also does with an > bitwise or. It knows that _3 is defined and true if either _1 or _2 > is > defined and true. Or more generically that the result bits of a > bitwise > or are defined for those bits that are both defined or where one is > defined and has the value 1. Aha - thanks. I think the distinction here is that: * GCC's -fanalyzer complains about uninitialized values immediately when it sees one being fetched for use in any expression (replacing the value with a safe one to avoid further complaints), without considering how they are going to be used in the expression, whereas * it sounds like valgrind keeps track of uninitialized bits, propagates the "uninit-ness" of the bits, and complains at certain times when uninitialized bits are used in certain ways. Dave
Re: Uninit warnings due to optimizing short-circuit conditionals
On Mon, 2022-02-14 at 12:20 -0500, David Malcolm wrote: > On Mon, 2022-02-14 at 17:57 +0100, Mark Wielaard wrote: > > On Mon, 2022-02-14 at 10:57 -0500, David Malcolm wrote: > > > [CCing Mark in the hopes of insight from the valgrind side of > > > things] > > > > Adding Julian to CC so he can correct me if I say something silly. > > > > > There is a false positive from -Wanalyzer-use-of-uninitialized- > > > value on > > > gcc.dg/analyzer/pr102692.c here: > > > > > > ‘fix_overlays_before’: events 1-3 > > > | > > > | 75 | while (tail > > > | | > > > | 76 | && (tem = make_lisp_ptr (tail, 5), > > > | | ^~ > > > | | | > > > | | (1) following ‘false’ branch (when ‘tail’ is > > > NULL)... > > > | 77 | (end = marker_position (XOVERLAY (tem)- > > > > end)) >= pos)) > > > > > > | | > > > ~ > > > |.. > > > | 82 | if (!tail || end < prev || !tail->next) > > > | | ~~~ > > > | | || > > > | | |(3) use of uninitialized value > > > ‘end’ here > > > | | (2) ...to here > > > | > > > > > > The issue is that inner || of the conditionals have been folded > > > within the > > > frontend from a chain of control flow: > > > > > >5 │ if (tail == 0B) goto ; else goto ; > > >6 │ : > > >7 │ if (end < prev) goto ; else goto ; > > >8 │ : > > >9 │ _1 = tail->next; > > > 10 │ if (_1 == 0B) goto ; else goto ; > > > 11 │ : > > > > > > to an OR expr (and then to a bitwise-or by the gimplifier): > > > > > >5 │ _1 = tail == 0B; > > >6 │ _2 = end < prev; > > >7 │ _3 = _1 | _2; > > >8 │ if (_3 != 0) goto ; else goto ; > > >9 │ : > > > 10 │ _4 = tail->next; > > > 11 │ if (_4 == 0B) goto ; else goto ; > > > > > > This happens for sufficiently simple conditionals in > > > fold_truth_andor. > > > In particular, the (end < prev) is short-circuited without > > > optimization, > > > but is evaluated with optimization, leading to the false positive. > > > > > > Given how early this folding occurs, it seems the simplest fix is > > > to > > > try to detect places where this optimization appears to have > > > happened, > > > and suppress uninit warnings within the statement that would have > > > been short-circuited (and thus e.g. ignoring them when evaluating > > > _2 > > > above for the case where _1 is known to be true at the (_1 | _2) , > > > and > > > thus _2 being redundant). > > > > > > Attached is a patch that implements this. > > > > > > There are some more details in the patch, but I'm wondering if this > > > is a > > > known problem, and how e.g. valgrind copes with such code. My > > > patch > > > feels like something of a hack, but I'm not sure of any other way > > > around > > > it given that the conditional is folded directly within the > > > frontend. > > > > As far as I know this is what valgrind memcheck also does with an > > bitwise or. It knows that _3 is defined and true if either _1 or _2 > > is > > defined and true. Or more generically that the result bits of a > > bitwise > > or are defined for those bits that are both defined or where one is > > defined and has the value 1. > > Aha - thanks. I think the distinction here is that: > > * GCC's -fanalyzer complains about uninitialized values immediately > when it sees one being fetched for use in any expression (replacing the > value with a safe one to avoid further complaints), without considering > how they are going to be used in the expression, whereas > > * it sounds like valgrind keeps track of uninitialized bits, propagates > the "uninit-ness" of the bits, and complains at certain times when > uninitialized bits are used in certain ways. Yes. valgrind keeps track of uninitialized bits and propagates them around till "use". Where use is anything that might alter the observable behavior of the program. Which is control flow transfers, conditional moves, addresses used in memory accesses, and data passed to system calls. This paper describes some of the memcheck tricks: https://valgrind.org/docs/memcheck2005.pdf Cheers, Mark
Re: Uninit warnings due to optimizing short-circuit conditionals
On Mon, Feb 14, 2022 at 6:38 PM Mark Wielaard wrote: > > On Mon, 2022-02-14 at 12:20 -0500, David Malcolm wrote: > > On Mon, 2022-02-14 at 17:57 +0100, Mark Wielaard wrote: > > > On Mon, 2022-02-14 at 10:57 -0500, David Malcolm wrote: > > > > [CCing Mark in the hopes of insight from the valgrind side of > > > > things] > > > > > > Adding Julian to CC so he can correct me if I say something silly. > > > > > > > There is a false positive from -Wanalyzer-use-of-uninitialized- > > > > value on > > > > gcc.dg/analyzer/pr102692.c here: > > > > > > > > ‘fix_overlays_before’: events 1-3 > > > > | > > > > | 75 | while (tail > > > > | | > > > > | 76 | && (tem = make_lisp_ptr (tail, 5), > > > > | | ^~ > > > > | | | > > > > | | (1) following ‘false’ branch (when ‘tail’ is > > > > NULL)... > > > > | 77 | (end = marker_position (XOVERLAY (tem)- > > > > > end)) >= pos)) > > > > > > > > | | > > > > ~ > > > > |.. > > > > | 82 | if (!tail || end < prev || !tail->next) > > > > | | ~~~ > > > > | | || > > > > | | |(3) use of uninitialized value > > > > ‘end’ here > > > > | | (2) ...to here > > > > | > > > > > > > > The issue is that inner || of the conditionals have been folded > > > > within the > > > > frontend from a chain of control flow: > > > > > > > >5 │ if (tail == 0B) goto ; else goto ; > > > >6 │ : > > > >7 │ if (end < prev) goto ; else goto ; > > > >8 │ : > > > >9 │ _1 = tail->next; > > > > 10 │ if (_1 == 0B) goto ; else goto ; > > > > 11 │ : > > > > > > > > to an OR expr (and then to a bitwise-or by the gimplifier): > > > > > > > >5 │ _1 = tail == 0B; > > > >6 │ _2 = end < prev; > > > >7 │ _3 = _1 | _2; > > > >8 │ if (_3 != 0) goto ; else goto ; > > > >9 │ : > > > > 10 │ _4 = tail->next; > > > > 11 │ if (_4 == 0B) goto ; else goto ; > > > > > > > > This happens for sufficiently simple conditionals in > > > > fold_truth_andor. > > > > In particular, the (end < prev) is short-circuited without > > > > optimization, > > > > but is evaluated with optimization, leading to the false positive. > > > > > > > > Given how early this folding occurs, it seems the simplest fix is > > > > to > > > > try to detect places where this optimization appears to have > > > > happened, > > > > and suppress uninit warnings within the statement that would have > > > > been short-circuited (and thus e.g. ignoring them when evaluating > > > > _2 > > > > above for the case where _1 is known to be true at the (_1 | _2) , > > > > and > > > > thus _2 being redundant). > > > > > > > > Attached is a patch that implements this. > > > > > > > > There are some more details in the patch, but I'm wondering if this > > > > is a > > > > known problem, and how e.g. valgrind copes with such code. My > > > > patch > > > > feels like something of a hack, but I'm not sure of any other way > > > > around > > > > it given that the conditional is folded directly within the > > > > frontend. > > > > > > As far as I know this is what valgrind memcheck also does with an > > > bitwise or. It knows that _3 is defined and true if either _1 or _2 > > > is > > > defined and true. Or more generically that the result bits of a > > > bitwise > > > or are defined for those bits that are both defined or where one is > > > defined and has the value 1. > > > > Aha - thanks. I think the distinction here is that: > > > > * GCC's -fanalyzer complains about uninitialized values immediately > > when it sees one being fetched for use in any expression (replacing the > > value with a safe one to avoid further complaints), without considering > > how they are going to be used in the expression, whereas > > > > * it sounds like valgrind keeps track of uninitialized bits, propagates > > the "uninit-ness" of the bits, and complains at certain times when > > uninitialized bits are used in certain ways. > > Yes. valgrind keeps track of uninitialized bits and propagates them > around till "use". Where use is anything that might alter the > observable behavior of the program. Which is control flow transfers, > conditional moves, addresses used in memory accesses, and data passed > to system calls. > > This paper describes some of the memcheck tricks: > https://valgrind.org/docs/memcheck2005.pdf That probably means bugs like https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63311 could be resolved as fixed (in valgrind). But indeed GCCs own uninit diagnostics fall foul of this as well. I suppose one could track uses and if they are used in bitwise operations could demote them to "maybe" even though they appear unconditionally. Now, we cannot distinguish
