[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

--- Comment #16 from Richard Biener  ---
Author: rguenth
Date: Thu Mar  2 13:42:05 2017
New Revision: 245840

URL: https://gcc.gnu.org/viewcvs?rev=245840=gcc=rev
Log:
2017-03-02  Richard Biener  

PR tree-optimization/79345
PR c++/42000
* tree-ssa-alias.c (walk_aliased_vdefs_1): Take a limit
param and abort the walk, returning -1 if it is hit.
(walk_aliased_vdefs): Take a limit param and pass it on.
* tree-ssa-alias.h (walk_aliased_vdefs): Add a limit param,
defaulting to 0 and return a signed int.
* tree-ssa-uninit.c (struct check_defs_data): New struct.
(check_defs): New helper.
(warn_uninitialized_vars): Use walk_aliased_vdefs to warn
about uninitialized memory.

* fixed-value.c (fixed_from_string): Use ulow/uhigh to avoid
bogus uninitialized warning.
(fixed_convert_from_real): Likewise.

* g++.dg/warn/Wuninitialized-7.C: New testcase.
* c-c++-common/ubsan/bounds-2.c: Add -Wno-uninitialized.
* gcc.dg/uninit-pr19430-2.c: Add expected warning.

Added:
trunk/gcc/testsuite/g++.dg/warn/Wuninitialized-7.C
Modified:
trunk/gcc/ChangeLog
trunk/gcc/fixed-value.c
trunk/gcc/testsuite/ChangeLog
trunk/gcc/testsuite/c-c++-common/ubsan/bounds-2.c
trunk/gcc/testsuite/gcc.dg/uninit-pr19430-2.c
trunk/gcc/tree-ssa-alias.c
trunk/gcc/tree-ssa-alias.h
trunk/gcc/tree-ssa-uninit.c

[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

--- Comment #15 from Jakub Jelinek  ---
Author: jakub
Date: Thu Mar  2 09:19:28 2017
New Revision: 245833

URL: https://gcc.gnu.org/viewcvs?rev=245833=gcc=rev
Log:
PR tree-optimization/79345
* gensupport.h (struct pattern_stats): Add min_scratch_opno field.
* gensupport.c (get_pattern_stats_1) : Update it.
(get_pattern_stats): Initialize it.
* genemit.c (gen_expand): Verify match_scratch numbers come after
match_operand/match_dup numbers.
* config/i386/i386.md (mul3_highpart): Swap match_dup and
match_scratch numbers.
* config/i386/sse.md (avx2_gathersi, avx2_gatherdi):
Likewise.
* config/s390/s390.md (trunctdsd2): Likewise.

Modified:
trunk/gcc/ChangeLog
trunk/gcc/config/i386/i386.md
trunk/gcc/config/i386/sse.md
trunk/gcc/config/s390/s390.md
trunk/gcc/genemit.c
trunk/gcc/gensupport.c
trunk/gcc/gensupport.h

[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

--- Comment #14 from rguenther at suse dot de  ---
On Wed, 1 Mar 2017, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345
> 
> --- Comment #13 from Jakub Jelinek  ---
> (In reply to Richard Biener from comment #9)
> > actually the genemit fix doensn't fully work:
> > 
> > /space/rguenther/src/svn/trunk/gcc/config/i386/i386.md: In function
> > ‘rtx_def* gen_smulsi3_highpart(rtx, rtx, rtx)’:
> > /space/rguenther/src/svn/trunk/gcc/config/i386/i386.md:7380:26: error:
> > ‘operands[3]’ is used uninitialized in this function [-Werror=uninitialized]
> > (any_extend:TI
> > ~~^
> > /space/rguenther/src/svn/trunk/gcc/config/i386/i386.md: In function
> > ‘rtx_def* gen_umulsi3_highpart(rtx, rtx, rtx)’:
> > /space/rguenther/src/svn/trunk/gcc/config/i386/i386.md:7380:26: error:
> > ‘operands[3]’ is used uninitialized in this function [-Werror=uninitialized]
> > (any_extend:TI
> > ~~^
> > ...
> > 
> > bah.
> 
> --- gcc/genemit.c.jj2017-01-01 12:45:35.0 +0100
> +++ gcc/genemit.c   2017-03-01 10:51:27.474179940 +0100
> @@ -518,6 +518,9 @@ gen_expand (md_rtx_info *info)
> {
>   for (i = 0; i < stats.num_operand_vars; i++)
> {
> + if (i > MAX (stats.max_opno, stats.max_dup_opno)
> + && i <= stats.max_scratch_opno)
> +   continue;
>   printf ("operand%d = operands[%d];\n", i, i);
>   printf ("(void) operand%d;\n", i);
> }
> 
> should fix that.  Even if some define_insn doesn't have any match_dups, it can
> still have normal operands.

Looks like we have patterns with dups after scratches:

(define_expand "mul3_highpart"
  [(parallel [(set (match_operand:SWI48 0 "register_operand")
   (truncate:SWI48
 (lshiftrt:
   (mult:
 (any_extend:
   (match_operand:SWI48 1 "nonimmediate_operand"))
 (any_extend:
   (match_operand:SWI48 2 "register_operand")))
   (match_dup 4
  (clobber (match_scratch:SWI48 3))
  (clobber (reg:CC FLAGS_REG))])]

which then still warns:

/space/rguenther/src/svn/trunk/gcc/config/i386/i386.md: In function 
‘rtx_def* gen_umulsi3_highpart(rtx, rtx, rtx)’:
/space/rguenther/src/svn/trunk/gcc/config/i386/i386.md:7380:14: warning: 
‘operands[3]’ is used uninitialized in this function [-Wuninitialized]
(any_extend:TI
~~^~~~ 
...

[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

--- Comment #13 from Jakub Jelinek  ---
(In reply to Richard Biener from comment #9)
> actually the genemit fix doensn't fully work:
> 
> /space/rguenther/src/svn/trunk/gcc/config/i386/i386.md: In function
> ‘rtx_def* gen_smulsi3_highpart(rtx, rtx, rtx)’:
> /space/rguenther/src/svn/trunk/gcc/config/i386/i386.md:7380:26: error:
> ‘operands[3]’ is used uninitialized in this function [-Werror=uninitialized]
> (any_extend:TI
> ~~^
> /space/rguenther/src/svn/trunk/gcc/config/i386/i386.md: In function
> ‘rtx_def* gen_umulsi3_highpart(rtx, rtx, rtx)’:
> /space/rguenther/src/svn/trunk/gcc/config/i386/i386.md:7380:26: error:
> ‘operands[3]’ is used uninitialized in this function [-Werror=uninitialized]
> (any_extend:TI
> ~~^
> ...
> 
> bah.

--- gcc/genemit.c.jj2017-01-01 12:45:35.0 +0100
+++ gcc/genemit.c   2017-03-01 10:51:27.474179940 +0100
@@ -518,6 +518,9 @@ gen_expand (md_rtx_info *info)
{
  for (i = 0; i < stats.num_operand_vars; i++)
{
+ if (i > MAX (stats.max_opno, stats.max_dup_opno)
+ && i <= stats.max_scratch_opno)
+   continue;
  printf ("operand%d = operands[%d];\n", i, i);
  printf ("(void) operand%d;\n", i);
}

should fix that.  Even if some define_insn doesn't have any match_dups, it can
still have normal operands.

[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

--- Comment #12 from Richard Biener  ---
Created attachment 40858
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=40858=edit
unreduced testcase for false positive

Bootstrap issue #2 (posted questionable workaround, not analyzed properly):

> ./cc1plus  -quiet -O2 -Wuninitialized -o /dev/null ~/ipa-cp.ii 
In file included from /space/rguenther/src/svn/trunk/gcc/coretypes.h:365:0,
 from /space/rguenther/src/svn/trunk/gcc/ipa-cp.c:105:
/space/rguenther/src/svn/trunk/gcc/wide-int.h: In function ‘unsigned int
ipcp_driver()’:
/space/rguenther/src/svn/trunk/gcc/wide-int.h:881:3: warning:
‘*((void*)& +8)’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
   storage::operator = (x);
   ^~~
/space/rguenther/src/svn/trunk/gcc/wide-int.h:881:3: warning:
‘*((void*)& +16)’ may be used uninitialized in this function
[-Wmaybe-uninitialized]

[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

--- Comment #11 from Richard Biener  ---
Created attachment 40857
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=40857=edit
unreduced testcase for false positive

Bootstrap issue #1 (I posted a reasonable workaround):

> ./cc1plus  -quiet -O2 -Wuninitialized -o /dev/null ~/fixed-value.ii 
In file included from /space/rguenther/src/svn/trunk/gcc/coretypes.h:365:0,
 from /space/rguenther/src/svn/trunk/gcc/fixed-value.c:22:
/space/rguenther/src/svn/trunk/gcc/wide-int.h: In function ‘void
fixed_from_string(fixed_value*, const char*, machine_mode)’:
/space/rguenther/src/svn/trunk/gcc/wide-int.h:818:40: warning: ‘*((void*)& w
+34359738360)’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
   unsigned HOST_WIDE_INT high = this->get_val ()[len - 1];
^
/space/rguenther/src/svn/trunk/gcc/wide-int.h: In function ‘bool
fixed_convert_from_real(fixed_value*, machine_mode, const real_value*, bool)’:
/space/rguenther/src/svn/trunk/gcc/wide-int.h:818:40: warning: ‘*((void*)& w
+34359738360)’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
   unsigned HOST_WIDE_INT high = this->get_val ()[len - 1];
^

[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

--- Comment #10 from Richard Biener  ---
https://gcc.gnu.org/ml/gcc-patches/2017-02/msg01284.html

[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

--- Comment #9 from Richard Biener  ---
actually the genemit fix doensn't fully work:

/space/rguenther/src/svn/trunk/gcc/config/i386/i386.md: In function ‘rtx_def*
gen_smulsi3_highpart(rtx, rtx, rtx)’:
/space/rguenther/src/svn/trunk/gcc/config/i386/i386.md:7380:26: error:
‘operands[3]’ is used uninitialized in this function [-Werror=uninitialized]
(any_extend:TI
~~^
/space/rguenther/src/svn/trunk/gcc/config/i386/i386.md: In function ‘rtx_def*
gen_umulsi3_highpart(rtx, rtx, rtx)’:
/space/rguenther/src/svn/trunk/gcc/config/i386/i386.md:7380:26: error:
‘operands[3]’ is used uninitialized in this function [-Werror=uninitialized]
(any_extend:TI
~~^
...

bah.

[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

--- Comment #8 from Richard Biener  ---
11:14 < richi> In function ‘void branch_target_load_optimize(bool)’:
11:14 < richi> cc1plus: error: ‘call_saved’ may be used uninitialized in this 
   function [-Werror=maybe-uninitialized]
11:14 < richi> cc1plus: error: ‘*((void*)& call_saved +8)’ may be used 
   uninitialized in this function [-Werror=maybe-uninitialized]

In file included from /space/rguenther/src/svn/trunk/gcc/coretypes.h:365:0,
 from /space/rguenther/src/svn/trunk/gcc/fixed-value.c:22:
/space/rguenther/src/svn/trunk/gcc/wide-int.h: In function ‘void
fixed_from_string(fixed_value*, const char*, machine_mode)’:
/space/rguenther/src/svn/trunk/gcc/wide-int.h:818:49: error: ‘*((void*)& w
+34359738360)’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
   unsigned HOST_WIDE_INT high = this->get_val ()[len - 1];
 ^
/space/rguenther/src/svn/trunk/gcc/wide-int.h: In function ‘bool
fixed_convert_from_real(fixed_value*, machine_mode, const real_value*, bool)’:
/space/rguenther/src/svn/trunk/gcc/wide-int.h:818:49: error: ‘*((void*)& w
+34359738360)’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
   unsigned HOST_WIDE_INT high = this->get_val ()[len - 1];
 ^

the above is from

 f->data.low = w.elt (0);

and

generic_wide_int ::elt (unsigned int i) const
{
  if (i >= this->get_len ())
return sign_mask ();
  else
return this->get_val ()[i];

resulting in 0 >= get_len () (which is get_len == 0).

And then we have

11:32 < jakub> richi: looks like a bug in genemit.c
11:33 < jakub> richi: the pattern has (clobber (match_scratch:QI 6))
11:33 < jakub> richi: and it emits:
11:33 < jakub> ...
11:33 < jakub>   rtx operand6 ATTRIBUTE_UNUSED;
11:33 < jakub> ...
11:34 < richi> jakub: and yes, the fixed-value.c is if (len == 0) { ... do 
   nonsense .. } else { .. here we always come ...} ( not sure yet 
   how we arrive there)
11:34 < jakub> operand6 = operands[6];
11:34 < jakub> (void) operand6;
11:34 < jakub> ..
11:34 < richi> shouldn't that be optimized away?
11:34 < jakub> and then:
11:34 < richi> ah, early uninit runs too early I guess
11:34 < jakub> gen_rtx_SCRATCH (QImode)) (rather than using operand6)
11:35 < richi> yeah, before any optimization ...
11:35 < richi> not so intelligent for the memory stuff


And

In file included from /space/rguenther/src/svn/trunk/gcc/coretypes.h:365:0,
 from /space/rguenther/src/svn/trunk/gcc/ipa-cp.c:105:
In member function ‘generic_wide_int&
generic_wide_int::operator=(const T&) [with T = wi::hwi_with_prec; storage =
wide_int_storage]’,
inlined from ‘void ipcp_store_vr_results()’ at
/space/rguenther/src/svn/trunk/gcc/ipa-cp.c:4962:49,
inlined from ‘unsigned int ipcp_driver()’ at
/space/rguenther/src/svn/trunk/gcc/ipa-cp.c:5003:25:
/space/rguenther/src/svn/trunk/gcc/wide-int.h:881:3: error:
‘*((void*)& +8)’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
   storage::operator = (x);
   ^~~

  vr.min = vr.max = wi::zero (INT_TYPE_SIZE);

generic_wide_int ::operator = (const T )
{
  storage::operator = (x);

can be avoided by removign the assignment (not sure about followup isssues
where we look at min/max for VR_VARYING).

That seems to be it, leaving the genemit issue.

Index: genemit.c
===
--- genemit.c   (revision 245601)
+++ genemit.c   (working copy)
@@ -518,6 +518,8 @@ gen_expand (md_rtx_info *info)
{
  for (i = 0; i < stats.num_operand_vars; i++)
{
+ if (i > stats.max_dup_opno && i <= stats.max_scratch_opno)
+   continue;
  printf ("operand%d = operands[%d];\n", i, i);
  printf ("(void) operand%d;\n", i);
}

results in

/space/rguenther/src/svn/gcc-7-branch/gcc/config/i386/i386.md: In function
‘rtx_def* gen_tls_global_dynamic_32(rtx, rtx, rtx, rtx)’:
/space/rguenther/src/svn/gcc-7-branch/gcc/config/i386/i386.md:13535:9: warning:
variable ‘operands’ set but not used [-Wunused-but-set-variable]
   return "call\t{*%p3@GOT(%1)|[DWORD PTR %p3@GOT[%1]]}";
 ^
/space/rguenther/src/svn/gcc-7-branch/gcc/config/i386/i386.md: In function
‘rtx_def* gen_tls_local_dynamic_base_32(rtx, rtx, rtx)’:
/space/rguenther/src/svn/gcc-7-branch/gcc/config/i386/i386.md:13655:9: warning:
variable ‘operands’ set but not used [-Wunused-but-set-variable]
   (clobber (match_scratch:SI 4))
 ^

we can put ATTRIBUTE_UNUSED on operands.

That allows us to build stage2 w/o new warnings...

[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

Richard Biener  changed:

   What|Removed |Added

   Assignee|jakub at gcc dot gnu.org   |rguenth at gcc dot 
gnu.org

--- Comment #7 from Richard Biener  ---
Created attachment 40796
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=40796=edit
untested patch w/o limiting the oracle walk

Extra warns for

FAIL: gcc.dg/uninit-B-O0.c (test for excess errors)
FAIL: gcc.dg/uninit-pr19430-2.c (test for excess errors)

Excess errors:
/space/rguenther/src/svn/gcc-7-branch/gcc/testsuite/gcc.dg/uninit-B-O0.c:13:5:
warning: 'i' may be used uninitialized in this function [-Wmaybe-uninitialized]

Excess errors:
/space/rguenther/src/svn/gcc-7-branch/gcc/testsuite/gcc.dg/uninit-pr19430-2.c:13:7:
warning: 'i' may be used uninitialized in this function [-Wmaybe-uninitialized]

which is kind-of expected (TREE_NO_WARNING wouldn't work on non-shared
trees in general).  Maybe we should try to mark the base if it is a decl?
But that might miss cases, say for

{
  X a;
  if (flag)
 ... = a.x;
  else
  = a.y;
}

we'd warn for a.x but not a.y.

To get an idea about verbosity I'll bootstrap the patch (w/o the limiting).

Comments welcome.

[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

--- Comment #6 from Jakub Jelinek  ---
Created attachment 40784
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=40784=edit
gcc7-pr79345.patch

Untested fix.  That said, I think (most likely just for GCC8) it would be very
much beneficial to walk_aliased_defs as the comment says, and if we run into
aliased clobber that must overlap the read ref, warn (and if we don't find any
aliased store too), with some cap on number of alias oracle queries.

[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

Jakub Jelinek  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
 CC||jakub at gcc dot gnu.org
   Assignee|unassigned at gcc dot gnu.org  |jakub at gcc dot gnu.org

--- Comment #5 from Jakub Jelinek  ---
I'll have a look.

[Bug tree-optimization/79345] [6/7 Regression] passing yet-uninitialized member as argument to base class constructor should warn (-Wunitialized)

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79345

Jason Merrill  changed:

   What|Removed |Added

  Component|c++ |tree-optimization

--- Comment #4 from Jason Merrill  ---
Changing component since this seems to be a tree-ssa-uninit.c issue.