This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: predicate aware uninitialized variable analysis


On Wed, Jul 22, 2009 at 2:29 PM, Richard
Guenther<richard.guenther@gmail.com> wrote:
> 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?
>

The patch mainly does false warning pruning -- so it should be
orthogonal to the memory operands handling. When you say memory
operands, I assume you mean stack memory -- otherwise interprocedural
analysis is needed.


> +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? ;)

A minor inconvenience ;)

>
> 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?
>
I added the timevar for the analysis. The time depends on the number
of possible uninitialized variables -- this is usually low. For each
possible uninitialized var, the computation mainly involves computing
all control dep chains from a root BB to the use BB (similarly to the
def). In extreme cases, the number of chains can be large so a
parameter is used to bound the limit which is set to a low value (do
not really care about really complicated predicates -- just bail out
and give the warning).

My experience (from gcc bootstrap, app testing etc) is that the
overhead is negligible.

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

Things that converts control dependency into data dependency will help
sharpen the analysis -- but I doubt the implementation will be simpler
-- as this one is already simple enough.


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


Thanks,

David

>
> 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;
>> }
>>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]