Bug 85301 - bitfield check causes maybe-uninitialized warning
Summary: bitfield check causes maybe-uninitialized warning
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 8.0.1
: P3 normal
Target Milestone: 13.0
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic, missed-optimization
Depends on:
Blocks: Wuninitialized
  Show dependency treegraph
 
Reported: 2018-04-09 12:17 UTC by Arnd Bergmann
Modified: 2022-11-28 22:36 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 10.2.0, 11.0, 8.3.0, 9.3.0
Last reconfirmed: 2021-04-05 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Arnd Bergmann 2018-04-09 12:17:04 UTC
A Linux kernel patch that changed a few flags from type 'int' to a single-bit bitfield caused a false-positive warning. I reduced a test case to

struct tick_sched {
#ifdef USE_BITFIELD
  unsigned int tick_stopped : 1;
  unsigned int idle_active : 1;
#else
  int tick_stopped;
  int idle_active;
#endif
};
long ktime_get();
void __tick_nohz_idle_restart_tick(long);
struct tick_sched tick_nohz_idle_exit_ts;
void tick_nohz_idle_exit(void) {
  long now;
  if (tick_nohz_idle_exit_ts.idle_active || tick_nohz_idle_exit_ts.tick_stopped)
    now = ktime_get();
  if (tick_nohz_idle_exit_ts.tick_stopped)
    __tick_nohz_idle_restart_tick(now);
}

$ gcc  -c tick-sched.c -Wall -O2 -DUSE_BITFIELD
tick-sched.c: In function ‘tick_nohz_idle_exit’:
tick-sched.c:19:5: warning: ‘now’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     __tick_nohz_idle_restart_tick(now);
$ gcc  -c tick-sched.c -Wall -O2
# no warning

It's easy to work around the warning, e.g. by copying the flag into a temporary variable, but it looks like this is something that gcc could handle better.

I looked through the list of false-positive Wmaybe-uninitialized bug reports, but couldn't find one that looks related to this particular one.
Comment 1 Richard Biener 2018-04-09 12:42:07 UTC
Hmm, this looks like a missed jump threading to me - possibly DOM being confused about the BIT_FIELD_REF and the & 3 that appears with the bitfield test due to
my much beloved optimize_bit_field_compare:

  if ((BIT_FIELD_REF <tick_nohz_idle_exit_ts, 8, 0> & 3) != 0)
    {
      now = ktime_get ();
    }
  if ((BIT_FIELD_REF <tick_nohz_idle_exit_ts, 8, 0> & 1) != 0)
    {
      __tick_nohz_idle_restart_tick (now);
    }

vs.

  if (tick_nohz_idle_exit_ts.idle_active != 0 || tick_nohz_idle_exit_ts.tick_stopped != 0)
    {
      now = ktime_get ();
    }
  if (tick_nohz_idle_exit_ts.tick_stopped != 0)
    {
      __tick_nohz_idle_restart_tick (now);
    }
Comment 2 Richard Biener 2018-04-09 12:46:19 UTC
That is, jump-threading fails to do

  if (x & 3)
    ...
  else
    thread -->
  if (x & 1)
    ...
  else
    <--- here

with the extra complication of that clobbering ktime_get () call on the
true path and x being memory references where DOM sees it.  PRE "cleans"
that up on the interesting path but still late DOM doesn't perform the
threading:

  <bb 2> [local count: 1073741825]:
  _1 = BIT_FIELD_REF <tick_nohz_idle_exit_ts, 8, 0>;
  _2 = _1 & 3;
  if (_2 != 0)
    goto <bb 3>; [33.00%]
  else
    goto <bb 4>; [67.00%]

  <bb 3> [local count: 354334802]:
  now_11 = ktime_get ();
  pretmp_14 = BIT_FIELD_REF <tick_nohz_idle_exit_ts, 8, 0>;

  <bb 4> [local count: 1073741825]:
  # now_5 = PHI <now_11(3), now_9(D)(2)>
  # prephitmp_15 = PHI <pretmp_14(3), _1(2)>
  _4 = prephitmp_15 & 1;
  if (_4 != 0)
    goto <bb 5>; [33.00%]
  else
  else
    goto <bb 6>; [67.00%]

  <bb 5> [local count: 354334802]:
  __tick_nohz_idle_restart_tick (now_5);

  <bb 6> [local count: 1073741825]:
Comment 3 Jakub Jelinek 2018-04-09 16:53:40 UTC
Maybe uninit just doesn't understand that in:
  <bb 2> [local count: 1073741825]:
  _1 = BIT_FIELD_REF <tick_nohz_idle_exit_ts, 8, 0>;
  _2 = _1 & 3;
  if (_2 != 0)
    goto <bb 3>; [33.00%]
  else
    goto <bb 7>; [67.00%]

  <bb 7> [local count: 719407025]:
  goto <bb 4>; [100.00%]

  <bb 3> [local count: 354334802]:
  now_11 = ktime_get ();
  pretmp_14 = BIT_FIELD_REF <tick_nohz_idle_exit_ts, 8, 0>;

  <bb 4> [local count: 1073741825]:
  # now_5 = PHI <now_11(3), now_9(D)(7)>
  # prephitmp_15 = PHI <pretmp_14(3), _1(7)>
  _4 = prephitmp_15 & 1;
  if (_4 != 0)
    goto <bb 5>; [33.00%]
  else
    goto <bb 8>; [67.00%]

  <bb 8> [local count: 719407025]:
  goto <bb 6>; [100.00%]

  <bb 5> [local count: 354334802]:
  __tick_nohz_idle_restart_tick (now_5); [tail call]

  <bb 6> [local count: 1073741825]:
  return;
if (_1 & 3) != 0 it implies (_1 & 1) != 0.

Though, if I change the testcase to:
void tick_nohz_idle_exit(void) {
  long now;
  struct tick_sched t = tick_nohz_idle_exit_ts;
  if (t.idle_active || t.tick_stopped)
    now = ktime_get();
  if (t.tick_stopped)
    __tick_nohz_idle_restart_tick(now);
}
so that it doesn't have to reread tick_nohz_idle_exit_ts.tick_stopped after the ktime_get call, it doesn't warn anymore.
In that case now is always initialized though, so it shouldn't matter what value we read for the uninit warning.
Comment 4 Jakub Jelinek 2018-04-09 16:56:09 UTC
Note, r142464 still doesn't warn, r142514 does, probably r142484 enabled the (premature) BIT_FIELD_REF comparison optimization in fold-const.c.
Comment 5 Jeffrey A. Law 2018-04-09 16:58:18 UTC
FWIW, there's another similar bug where DOM doesn't do a particularly good job at tracking the state of objects implied the results of logical operations which in turn causes missed optimizations.  I've poked at it a few times but haven't come up with anything good to address the deficiency.  It's on the Wuninitialized tracker, so I'll certainly take a look over the coming months.
Comment 6 Arnd Bergmann 2018-04-09 20:40:48 UTC
I found that older versions (gcc-5 and before) did not warn when the type gets changed to bitfield of '_Bool' rather than 'unsigned int'. It seems that this was only because they tested each bit separately in the _Bool case rather than combining the first two comparisons into a &3 mask.
Comment 7 Jeffrey A. Law 2018-04-09 22:07:05 UTC
So I went and found pr68548 which is related, but clearly different.

Just thinking out loud here, I think we can pick up this missed jump thread by recording _1 & 1 == 0 on the false edge of the first conditional (when we know that _1 & 3 == 0).  It doesn't scale well in the general case, but when there's just a few bits in the mask it might be reasonable.

That ought to allow us to determine that _4 == 0 when bb4 is reached via bb2 and thread the jump.  Not planning to look at any implementation for gcc-8 though.
Comment 8 Martin Sebor 2021-04-05 20:34:34 UTC
Reconfirmed with GCC 11 with the ever-so-slightly slightly simplified program below and enhanced output.  The warning has been issued since at least GCC 4.1 and so is not a regression.

$ cat pr85301.c && gcc -O2 -S -Wall -DUSE_BITFIELD  pr85301.c
struct A
{
#ifdef USE_BITFIELD
  unsigned i : 1;
  unsigned j : 1;
#else
  unsigned i;
  unsigned j;
#endif
};

int z, f (void);

struct A a;

void h (void)
{
  int y;

  if (a.j || a.i)
    y = f ();

  if (a.i)
    z = y;
}

pr85301.c: In function ‘h’:
pr85301.c:24:7: warning: ‘y’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   24 |     z = y;
      |     ~~^~~
pr85301.c:18:7: note: when ‘!(((unsigned char*)&a)[0] & 3)’
   18 |   int y;
      |       ^
pr85301.c:18:7: note: used when ‘prephitmp_15 = PHI <_1(7), pretmp_14(3)> & 1’
pr85301.c:18:7: note: ‘y’ was declared here

Jump threading and other optimization opportunities aside (I have raised pr99918 for one of those), there does also seem to be a limitation in the warning code in that it doesn't understand BIT_FIELD_REF expressions.  The warning sees the IL below from which it should be able to determine that y's use is conditional on its definition (i.e., the use predicate a strict subset of the predicate controlling the definition).

void h ()
{
  int y;
  unsigned char _1;
  unsigned char _2;
  unsigned char _4;
  unsigned char pretmp_14;
  unsigned char prephitmp_15;

  <bb 2> [local count: 1073741824]:
  # VUSE <.MEM_8(D)>
  _1 = BIT_FIELD_REF <a, 8, 0>;
  _2 = _1 & 3;
  if (_2 != 0)
    goto <bb 3>; [33.00%]
  else
    goto <bb 7>; [67.00%]

  <bb 7> [local count: 719407024]:
  goto <bb 4>; [100.00%]

  <bb 3> [local count: 354334800]:
  # .MEM_10 = VDEF <.MEM_8(D)>
  y_11 = f ();
  # VUSE <.MEM_10>
  pretmp_14 = BIT_FIELD_REF <a, 8, 0>;

  <bb 4> [local count: 1073741824]:
  # y_5 = PHI <y_9(D)(7), y_11(3)>
  # .MEM_6 = PHI <.MEM_8(D)(7), .MEM_10(3)>
  # prephitmp_15 = PHI <_1(7), pretmp_14(3)>
  _4 = prephitmp_15 & 1;
  if (_4 != 0)
    goto <bb 5>; [50.00%]
  else
    goto <bb 8>; [50.00%]

  <bb 8> [local count: 536870912]:
  goto <bb 6>; [100.00%]

  <bb 5> [local count: 536870913]:
  ## y_5 = PHI <y_9(D)(7), y_11(3)>
  ## uninit when: _2 == 0
  ##            : BIT_FIELD_REF <a, 8, 0> & 3 == 0
  ##
  ## used when: prephitmp_15 & != 0
  ##          : PHI <_1(7), pretmp_14(3)>
  ##          : _1(7) != 0 || pretmp_14 != 0
  ##          : BIT_FIELD_REF <a, 8, 0> != 0 || BIT_FIELD_REF <a, 8, 0> != 0
  ##          : BIT_FIELD_REF <a, 8, 0> != 0
  # .MEM_12 = VDEF <.MEM_6>
  z = y_5;

  <bb 6> [local count: 1073741824]:
  # .MEM_7 = PHI <.MEM_6(8), .MEM_12(5)>
  # VUSE <.MEM_7>
  return;

}
Comment 9 Jeffrey A. Law 2022-11-20 04:17:50 UTC
This has been fixed on the trunk.