Re: predicate aware uninitialized variable analysis

[Richard Guenther <richard dot guenther at gmail dot com>]

On Wed, Jul 22, 2009 at 11:19 PM, Xinliang David Li<davidxl@google.com> wrote:
> Hi, this was a unsubmitted patch from last year. The patch implements
> predicate aware uninitialized variable warning in GCC. The
> implementation changed the way potentially uninitialized variables are
> warned as of today. Before the change, GCC gave a warning when an
> empty def is used by an phi operand. This is not only noisy, but also
> does not have precise source information on where the uninit reference
> is. After the change, the warning is only given on actual uses of phi
> defs that are potentially undefined (with empty operands or possibly
> empty operands -- defined transitively). In the analysis, The
> predicate guarding the use of potentially uninitialized variable is
> compared against the predicate guarding the definition. The predicate
> expressions are formed using both control dependence and data
> dependence information. ?Some notes:
>
> 1. In current GCC, many of the false positives can be eliminated if
> jump threading kicks in, but slightly more complicated cases (as in
> the examples and test cases attached) will defeat it.
> 2. The predicate expression comparison/evaluation in the
> implementation does/can not rely on expr tree simplification/folding
> to do the job, and is a little rusty -- but good enough for the
> applications tested (and test cases accumulated).
> 3. Diego has spent lot of time reviewing the code last year.
> 4. bootstrap and tested on i686/linux. SPEC06/SPEC2k/internal apps.
>
> Let me know if this ok enough for mainline. If not, I will create a
> public branch and whoever is interested in enhancing the analysis can
> start contributing.

A few questions without thoroughly lookin at the patch yet.

Can the pass possibly be extended to handle uninitialized use of
memory operands?

+static inline basic_block
+find_pdom (basic_block block)
+{
+   if (block == EXIT_BLOCK_PTR)
+     return EXIT_BLOCK_PTR;
...

can we finally fix dominator information for the entry/exit blocks please? ;)

Can you say something about the compile-time complexity of the pass?
It will be enabled by default for most of our users (that happen to use
-Wall), so what is the compile-time effect?

If we'd have predicated value-numbering would that be able to provide
infrastructure comparable to what this pass adds?

I've queued the patch for a more detailed review.

Thanks,
Richard.

> Thanks,
>
> David
>
> ==============================================================
>
> The following are some examples. For more, take a look at the test
> cases included in the patch file.
>
>
> Example 1:
> ======================
>
> int g;
> void bar (void);
> void blah (int);
>
> int foo (int n, int m, int r)
> {
> ?int flag = 0;
> ?int v;
>
> ?if (n)
> ? ?{
> ? ? ?v = r;
> ? ? ?flag = 1;
> ? ?}
>
> ?if (m)
> ? ?g++;
> ?else
> ? ?bar();
>
> ?if (flag)
> ? ?blah(v); /* { dg-bogus "uninitialized" "bogus uninitialized var
> warning" } */
>
> ?return 0;
> }
>
>
>
> // Example 2 (flag check after inlining -- very common scenario)
> ==================================================================
>
> typedef long long int64;
> void incr ();
> bool is_valid (int);
> int ?get_time ();
>
> class A
> {
> public:
> ?A ();
> ?~A () {
> ? ?if (I) delete I;
> ?}
>
> private:
> ?int* I;
> };
>
> bool get_url (A *);
>
> class M {
>
> ?public:
> __attribute__ ((always_inline)) ?int GetC (int *c) ?{
>
> ? ?A details_str;
> ? ?if (!get_url (&details_str))
> ? ? ?{
> ? ? ? ?incr ();
> ? ? ? ?return 1;
> ? ? ?}
>
> ? ?*c = get_time ();
> ? ?return -1;
> ?}
>
> ?void do_sth();
> ?void do_sth2();
>
> ?void P (int64 t)
> ? ?{
> ? ? ?int cc; /* { dg-bogus "uninitialized" "uninitialized variable
> warning" } ?*/
> ? ? ?if (GetC (&cc) >= 0 )
> ? ? ? ?return;
>
> ? ? ?if (t && cc <= 0 ) ?/* { dg-bogus "uninitialized" "uninitialized
> variable warning" } */
> ? ? ? ?{
> ? ? ? ? ?this->do_sth();
> ? ? ? ? ?return;
> ? ? ? ?}
>
> ? ?do_sth2();
> ?}
> };
>
> M* m;
> void foo(int x)
> {
> ?m = new M;
> ?m->P(x);
> }
>
> // Example 3
> ========================
> int g;
> void bar();
> void blah(int);
>
> int foo (int n, int l, int m, int r)
> {
> ?int v;
>
> ?if (n > 10)
> ? ?v = r;
>
> ?if (m) g++;
> ?else ? bar();
>
> ?if ( (n > 10) && (l < 100))
> ? ? ?blah(v); /* { dg-bogus "uninitialized" "bogus warning" } */
>
> ?if ( n > 100 )
> ? ? ?blah(v); /* { dg-bogus "uninitialized" "bogus warning" } */
>
> ?return 0;
> }
>
> int foo_2 (int n, int l, int m, int r)
> {
> ?int v;
>
> ?if (n > 10)
> ? ?v = r;
>
> ?if (m) g++;
> ?else ? bar();
>
> ?if ( n < 10)
> ? ? ?blah (v); /* { dg-warning "uninitialized" "warning" } */
>
>
> ?return 0;
> }
>