Bug 30252 - [4.2 regression] miscompilation of sigc++-2.0 based code with -fstrict-aliasing
Summary: [4.2 regression] miscompilation of sigc++-2.0 based code with -fstrict-aliasing
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.2.0
: P1 blocker
Target Milestone: 4.2.1
Assignee: Richard Biener
URL:
Keywords: alias, wrong-code
Depends on: 22488
Blocks: 32303
  Show dependency treegraph
 
Reported: 2006-12-18 15:56 UTC by Serge Belyshev
Modified: 2007-07-18 09:44 UTC (History)
13 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.3.0
Known to fail:
Last reconfirmed: 2007-05-01 14:00:47


Attachments
preprocessed and somewhat reduced testcase (1.63 KB, text/plain)
2006-12-18 15:57 UTC, Serge Belyshev
Details
self contained preprocessed testcase (1.64 KB, text/plain)
2007-04-29 09:58 UTC, Serge Belyshev
Details
slightly simplified testcase (1.48 KB, text/plain)
2007-05-01 13:35 UTC, Richard Biener
Details
patch (2.88 KB, patch)
2007-06-05 15:01 UTC, Richard Biener
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Serge Belyshev 2006-12-18 15:56:04 UTC
gcc miscompiles this testcase (reduced from rtorrent) since r111639, compile with -O1 -fstrict-aliasing:

#include <sigc++/bind.h>
#include <sigc++/slot.h>

static long dummy;

struct A
{
  static void *foo (void *p) { return p; }
  typedef sigc::slot <void *> C;
  C bar();
};

A::C A::bar ()
{
  return sigc::bind (sigc::ptr_fun (&A::foo), &dummy);
}

int main (void)
{
  A a;
  if (a.bar ()() != &dummy)
    abort ();
  return 0;
}


The program will crash in operator() because we store garbage instead of function pointer inside A::bar():

--- O1	2006-12-18 18:44:34.000000000 +0300
+++ O1-fstrict-aliasing	2006-12-18 18:44:41.000000000 +0300
@@ -248,8 +248,8 @@
 	movq	sigc::internal::typed_slot_rep<sigc::bind_functor<-1, sigc::pointer_functor1<void*, void*>, long*, sigc::nil, sigc::nil, sigc::nil, sigc::nil, sigc::nil, sigc::nil> >::dup(void*), 24(%rbx)
 	movq	$0, 32(%rbx)
 	movq	$0, 40(%rbx)
+	movq	%rbx, 64(%rbx)
 	movq	$dummy, 72(%rbx)
-	movq	A::foo(void*), 64(%rbx)
 	movq	%rbx, (%rsp)
 	leaq	48(%rbx), %rsi
 	movq	%rsp, %rdi

I will attach preprocessed and somewhat reduced testcase in a moment.

Caused by this patch:

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 111638)
+++ gcc/ChangeLog	(revision 111639)
@@ -1,3 +1,8 @@
+2006-03-02  Richard Guenther  <rguenther@suse.de>
+
+	* tree-ssa-alias.c (find_used_portions): Consider taking
+	the address as making the variable not write-only.
+
 2006-03-02  Nick Clifton  <nickc@redhat.com>
 
 	* config.gcc (default_use_cxa_atexit): Extend the description of
Index: gcc/tree-ssa-alias.c
===================================================================
--- gcc/tree-ssa-alias.c	(revision 111638)
+++ gcc/tree-ssa-alias.c	(revision 111639)
@@ -3071,6 +3071,8 @@ find_used_portions (tree *tp, int *walk_
 	    up->minused = 0;
 	    up->maxused = TREE_INT_CST_LOW (DECL_SIZE (var));
 	    up->implicit_uses = true;
+	    if (!lhs_p)
+	      up->write_only = false;
 
 	    up_insert (uid, up);
 	    *walk_subtrees = 0;
Comment 1 Serge Belyshev 2006-12-18 15:57:41 UTC
Created attachment 12825 [details]
preprocessed and somewhat reduced testcase
Comment 2 Andrew Pinski 2006-12-18 19:04:16 UTC
Hmm,
	typedef typed_slot_rep<T_functor> typed_slot;
	typed_slot *typed_rep = static_cast<typed_slot*>(rep);
	return (typed_rep->functor_)();

This code could violate C++ aliasing rules.
Comment 3 Serge Belyshev 2006-12-19 11:11:01 UTC
This does not fail on mainline since mem-ssa merge (r119760).
Comment 4 Richard Biener 2006-12-27 16:18:04 UTC
The patch referenced only could have uncovered a latent problem.  mem-ssa might simply have hidden it again.

The reduced testcase doesn't link for me btw:

/tmp/ccGXFf9y.o: In function `A::bar()':
589.ii:(.text+0x2c): undefined reference to `sigc::slot_base::slot_base(sigc::internal::slot_rep*)'
collect2: ld returned 1 exit status
Comment 5 Serge Belyshev 2006-12-27 20:19:03 UTC
(In reply to comment #4)
> The reduced testcase doesn't link for me btw:

sorry, forgot to mention: both tests require -lsigc-2.0
Comment 6 Mark Mitchell 2007-02-19 20:52:02 UTC
Before we can prioritize this problem, we need to establish that it doesn't violate the aliasing rules.  As Andrew says in Comment #2, this looks likely to be a problem in the input program.
Comment 7 Andrew Pinski 2007-04-29 08:01:53 UTC
Can you try this again, I think this was related to PR 30567 which was just fixed?
Comment 8 Serge Belyshev 2007-04-29 09:58:03 UTC
Created attachment 13461 [details]
self contained preprocessed testcase

(In reply to comment #7)
> Can you try this again, I think this was related to PR 30567 which was just
> fixed?

Tested r124272, bug is still there.

I'm attaching testcase which doesn't require -lsigc-2.0. Tested on i386, amd64, alpha with same result.

Not sure about aliasing rules here. There is only two casts here, first one (reinterpret_cast around line 19) looks innocent, the second (static_cast, line 222) is suspicious. Someone knowledgeable should re-examine it.
Comment 9 Wolfgang Bangerth 2007-04-30 03:25:25 UTC
I can't reproduce this with neither r124272 nor r124284. What flags exactly do
you use?

W.
Comment 10 Pawel Sikora 2007-04-30 15:16:42 UTC
i can reproduce testcase4 on 4.2.0-20070425@x86-64.

$ g++ 30252.cpp -O1 -fstrict-aliasing -g3 && ./a.out
zsh: segmentation fault  ./a.out

(gdb) bt
#0  0x00002adee263dbc0 in _rtld_local_ro () from /lib64/ld-linux-x86-64.so.2
#1  0x0000000000400654 in sigc::internal::slot_call0<sigc::bind_functor<-1, sigc::pointer_functor1<void*, void*>, long*> >::call_it (
    rep=0x601010) at 30252.cpp:73
#2  0x000000000040062b in main () at 30252.cpp:238

it works with -O1 -fno-strict-aliasing.
Comment 11 Pawel Sikora 2007-04-30 15:29:10 UTC
--- 30252.cpp.099t.optimized.aliasing-OFF
+++ 30252.cpp.099t.optimized.aliasing-ON

 sigc::slot0 A::bar() (this)
 {
(...)
+  void * (*<T5b7>) (void *) SR.114;
(...)
+  this->functor_.D.2915.functor_.functor_.func_ptr_ = SR.114;
   this->functor_.bound1_.visited_ = &dummy;
-  this->functor_.D.2915.functor_.functor_.func_ptr_ = foo;
(...)

with strict aliasing the func_ptr_ is initialized with uninitialized
pointer (SR.114) instead of `foo'.
Comment 12 Wolfgang Bangerth 2007-04-30 15:42:50 UTC
(In reply to comment #10)
> i can reproduce testcase4 on 4.2.0-20070425@x86-64.

I still can't reproduce on i686, sorry. Someone else will
have to look at this and try to further reduce the sources...

W.
Comment 13 Giovanni Bajo 2007-05-01 02:11:45 UTC
(In reply to comment #2)

> Hmm,
>         typedef typed_slot_rep<T_functor> typed_slot;
>         typed_slot *typed_rep = static_cast<typed_slot*>(rep);
>         return (typed_rep->functor_)();
> 
> This code could violate C++ aliasing rules.

But:
    template <class T_functor>
    struct typed_slot_rep : public slot_rep

so it looks like it might be valid.

Comment 14 bangerth@math.tamu.edu 2007-05-01 02:39:17 UTC
Subject: Re:  [4.2 regression] miscompilation
 of sigc++-2.0 based code with -fstrict-aliasing


> >         typedef typed_slot_rep<T_functor> typed_slot;
> >         typed_slot *typed_rep = static_cast<typed_slot*>(rep);
> >         return (typed_rep->functor_)();
> >
> > This code could violate C++ aliasing rules.
>
> But:
>     template <class T_functor>
>     struct typed_slot_rep : public slot_rep
>
> so it looks like it might be valid.

But only, of course, if the pointer actually points to a typed_slot_rep.
Maybe someone who can reproduce the bug can make the following test:
- add a virtual destructor to slot_rep
- change the static_cast to a dynamic_cast
If the object pointed to is not a typed_slot_rep (in which the static_cast
in the original code would have been in error), then the code should
probably segfault or so...

W.

-------------------------------------------------------------------------
Wolfgang Bangerth                email:            bangerth@math.tamu.edu
                                 www: http://www.math.tamu.edu/~bangerth/

Comment 15 Pawel Sikora 2007-05-01 08:58:58 UTC
(In reply to comment #14)

typed_rep points to: N4sigc8internal14typed_slot_repINS_12bind_functorILin1ENS_16pointer_functor1IPvS4_EEPlEEEE
Comment 16 Richard Biener 2007-05-01 13:35:21 UTC
Created attachment 13468 [details]
slightly simplified testcase

I don't see anything wrong with the testcase.  I changed it to not take the
address of dummy, but pass zero instead.  This reduces the diff of initial aliasing to

-  #   _A_b1_61 = V_MAY_DEF <_A_b1_54>;
-  #   SFT.70_62 = V_MAY_DEF <SFT.70_55>;
-  #   SFT.71_63 = V_MAY_DEF <SFT.71_56>;
-  #   SFT.72_64 = V_MAY_DEF <SFT.72_57>;
-  #   SFT.73_65 = V_MAY_DEF <SFT.73_58>;
-  #   NONLOCAL.79_66 = V_MAY_DEF <NONLOCAL.79_60>;
+  #   SFT.79_61 = V_MAY_DEF <SFT.79_57>;
   this_41->rep_ = rep_42;

but before DCE (which makes a mess out of this testcase) we have practically
indentical dumps for A::bar() - Note that A::bar() is the offending function
that gets mis-optimized.  We DCE the initialization of the function pointer so
we segfault at the indirect call to it.

@@ -78,21 +78,21 @@
 
 sigc::slot0 A::bar() (this)
 {
...
@@ -129,7 +129,7 @@
   _A_func_2 = foo;
   this_3 = (struct functor_base *) &D.2378;
   this_4 = this_3;
-  #   SFT.73_6 = V_MUST_DEF <SFT.73_5>;
+  #   SFT.80_6 = V_MUST_DEF <SFT.80_5>;
   D.2378.func_ptr_ = foo;
   _A_functor_7 = &D.2378;
   #   _A_b1_9 = V_MUST_DEF <_A_b1_8>;
@@ -151,33 +151,33 @@
   this_24 = (struct functor_base *) this_23;
   this_25 = this_24;
   #   D.2951_27 = V_MUST_DEF <D.2951_26>;
-  #   VUSE <SFT.73_6>;
+  #   VUSE <SFT.80_6>;
   D.2951 = D.2378;
-  #   SFT.70_50 = V_MAY_DEF <SFT.70_49>;
+  #   SFT.77_50 = V_MAY_DEF <SFT.77_49>;
   #   VUSE <D.2951_27>;
   this_20->functor_ = D.2951;
   this_28 = &D.2514.bound1_;
   this_29 = &D.2514.bound1_;
   _A_argument_30 = &_A_b1;
   D.2953_31 = 0B;
-  #   SFT.70_68 = V_MUST_DEF <SFT.70_50>;
+  #   SFT.77_63 = V_MUST_DEF <SFT.77_50>;
   D.2514.bound1_.visited_ = D.2953_31;
   functor_32 = &D.2514;
   #   _A_b1_54 = V_MAY_DEF <_A_b1_9>;
-  #   SFT.70_55 = V_MAY_DEF <SFT.70_68>;
-  #   SFT.71_56 = V_MAY_DEF <SFT.71_52>;
-  #   SFT.72_57 = V_MAY_DEF <SFT.72_45>;
-  #   SFT.73_58 = V_MAY_DEF <SFT.73_6>;
-  #   NONLOCAL.79_59 = V_MAY_DEF <NONLOCAL.79_53>;
-  D.3052_33 = operator new (20);
-  this_34 = (struct typed_slot_rep<sigc::bind_functor<-0x000000001, sigc::pointer_functor1<void*, void*>, void*> > *) D.3052_33;
+  #   SFT.77_55 = V_MAY_DEF <SFT.77_63>;
+  #   SFT.78_56 = V_MAY_DEF <SFT.78_52>;
+  #   SFT.79_57 = V_MAY_DEF <SFT.79_45>;
+  #   SFT.80_58 = V_MAY_DEF <SFT.80_6>;
+  #   NONLOCAL.86_59 = V_MAY_DEF <NONLOCAL.86_53>;
+  D.3059_33 = operator new (20);
+  this_34 = (struct typed_slot_rep<sigc::bind_functor<-0x000000001, sigc::pointer_functor1<void*, void*>, void*> > *) D.3059_33;
   this_35 = this_34;
   functor_36 = &D.2514;
   this_37 = &this_35->D.2649;
   this_38 = this_37;
-  #   NONLOCAL.79_60 = V_MAY_DEF <NONLOCAL.79_59>;
-  #   VUSE <SFT.70_55>;
-  #   VUSE <SFT.71_56>;
+  #   NONLOCAL.86_60 = V_MAY_DEF <NONLOCAL.86_59>;
+  #   VUSE <SFT.77_55>;
+  #   VUSE <SFT.78_56>;
   this_35->functor_ = D.2514;
   rep_39 = (struct slot_rep *) this_34;
   this_40 = &<retval>.D.2330;
@@ -185,14 +185,14 @@
   rep_42 = rep_39;
   this_43 = (struct functor_base *) &<retval>.D.2330;
   this_44 = this_43;
-  #   SFT.72_69 = V_MUST_DEF <SFT.72_57>;
+  #   SFT.79_64 = V_MUST_DEF <SFT.79_57>;
   <retval>.D.2330.rep_ = rep_42;
-  D.3058_46 = rep_39;
-  D.3060_47 = call_it;
-  D.3060_48 = call_it;
-  #   NONLOCAL.79_67 = V_MAY_DEF <NONLOCAL.79_60>;
-  D.3058_46->call_ = call_it;
-  #   VUSE <SFT.72_69>;
+  D.3065_46 = rep_39;
+  D.3067_47 = call_it;
+  D.3067_48 = call_it;
+  #   NONLOCAL.86_62 = V_MAY_DEF <NONLOCAL.86_60>;
+  D.3065_46->call_ = call_it;
+  #   VUSE <SFT.79_64>;
   return <retval>;
 
 }

Still the DCE diff contains

 <bb 2>:
-  #   SFT.73_6 = V_MUST_DEF <SFT.73_5>;
+  #   SFT.80_6 = V_MUST_DEF <SFT.80_5>;
   D.2378.func_ptr_ = foo;
   #   _A_b1_9 = V_MUST_DEF <_A_b1_8>;
   _A_b1 = 0B;
-  this_19 = &D.2514.D.2511.functor_;
-  this_20 = this_19;
-  #   D.2951_27 = V_MUST_DEF <D.2951_26>;
-  #   VUSE <SFT.73_6>;
-  D.2951 = D.2378;
-  #   SFT.70_50 = V_MAY_DEF <SFT.70_49>;
-  #   VUSE <D.2951_27>;
-  this_20->functor_ = D.2951;
   D.2953_31 = 0B;
-  #   SFT.70_68 = V_MUST_DEF <SFT.70_50>;
+  #   SFT.77_63 = V_MUST_DEF <SFT.77_49>;
   D.2514.bound1_.visited_ = D.2953_31;

where we removed the assignment

  D.2951 = D.2378;
Comment 17 Richard Biener 2007-05-01 14:00:47 UTC
Without -fstrict-aliasing we mark this_20->functor_ = D.2946; obviously necessary
because it is is_hidden_global_store ():

 ;; Function sigc::slot0 A::bar() (_ZN1A3barEv)
 
-Marking useful stmt: this_20->functor_ = D.2946;
-
-Marking useful stmt: D.3047_33 = operator new (20);
+Marking useful stmt: D.3054_33 = operator new (20);
 
 Marking useful stmt: this_35->functor_ = D.2514;
 
-Marking useful stmt: D.3053_46->call_ = call_it;
+Marking useful stmt: D.3060_46->call_ = call_it;
 
 Marking useful stmt: return <retval>;

without that we DCE too much as

  #   SFT.80_6 = V_MUST_DEF <SFT.80_5>;
  D.2378.func_ptr_ = foo;
...
  #   D.2951_27 = V_MUST_DEF <D.2951_26>;
  #   VUSE <SFT.80_6>;
  D.2951 = D.2378;
  #   SFT.77_50 = V_MAY_DEF <SFT.77_49>;
  #   VUSE <D.2951_27>;
  this_20->functor_ = D.2951;
  #   SFT.77_63 = V_MUST_DEF <SFT.77_50>;
  D.2514.bound1_.visited_ = D.2953_31;

the last store kills the assignment to this_20->functor_.  Note that while
we have uses for D.2951_27 and SFT.80_6 the structure assignment to
this_20->functor_ for some reason shares the SFT with the assignment
to D.2514.bound1_.visited_.  So this may be as well a frontend bug.
Comment 18 Richard Biener 2007-05-01 14:05:06 UTC
So my guess is this is related to PR22488 which is rotting since some time...
Comment 19 Mark Mitchell 2007-05-02 21:59:40 UTC
If this is a valid program, as we seem to think it is, then this is a serious bug.  However, if it's the front-end bug that Richard suspects, there's not much we can do about it in the near term.  The ball is firmly in Jason's court.
Comment 20 Jason Merrill 2007-05-07 16:59:09 UTC
Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0
 based code with -fstrict-aliasing

Does reverting the patch for 25895 fix this bug?
Comment 21 Serge Belyshev 2007-05-07 18:02:39 UTC
(In reply to comment #20)
> Does reverting the patch for 25895 fix this bug?

No.
Comment 22 İsmail Dönmez 2007-05-13 01:43:27 UTC
This problem also breaks inkscape, it segfaults on startup when compiled with gcc 4.2.0
Comment 23 Richard Biener 2007-05-13 11:34:04 UTC
There are no overlapping fields created from push_fields_onto_fieldstack, so my
pointing to PR22488 may be wrong.
Comment 24 Richard Biener 2007-05-13 13:02:41 UTC
That leaves aliasing and DCE.

The following statement connects to the initializing of the function pointer.

  #   SFT.78D.2952_55 = V_MAY_DEF <SFT.78D.2952_31>;
  #   VUSE <D.2838_29>;
  thisD.2828_22->functor_D.2399 = D.2838;

but,

  D.2839_30 = 0B;
  #   SFT.78D.2952_32 = V_MUST_DEF <SFT.78D.2952_55>;
  D.2831.bound1_D.2423 = D.2839_30;

kills it.  Still they look like they are accessing different fields
(propagating pointers for the first expression):

  #   SFT.78D.2952_55 = V_MAY_DEF <SFT.78D.2952_31>;
  #   VUSE <D.2838_29>;
  D.2831.D.2432.functor_D.2382.functor_D.2399 = D.2838;

so my hint at a frontend problem with type-layout.  Scheduling some more
early cleanup passes before the first DCE shows this (and fixes the segfault!?):

  #   SFT.79D.2951_66 = V_MUST_DEF <SFT.79D.2951_49>;
  #   VUSE <D.2836_27>;
  D.2435.D.2432.functor_D.2382.functor_D.2399 = D.2836;
  D.2837_28 = 0B;
  #   SFT.78D.2950_30 = V_MUST_DEF <SFT.78D.2950_29>;
  D.2435.bound1_D.2423 = D.2837_28;

note how we no longer thing the second store kills the first one.  Which
makes it look like an aliasing problem again.  Scheduling another may_alias
pass before the first DCE also fixes the problem, as we then have

  #   SFT.88D.2960_48 = V_MAY_DEF <SFT.88D.2960_29>;
  #   SFT.89D.2961_61 = V_MAY_DEF <SFT.89D.2961_49>;
  #   VUSE <D.2836_27>;
  thisD.2827_20->functor_D.2399 = D.2836;
  D.2837_28 = 0B;
  #   SFT.88D.2960_30 = V_MUST_DEF <SFT.88D.2960_48>;
  D.2435.bound1_D.2423 = D.2837_28;

aliasing is fixed as long as the extra may_alias pass is after CCP, but that
CCP pass doesn't do too much interesting stuff to the above stmts:

   #   SFT.88D.2960_48 = V_MAY_DEF <SFT.88D.2960_29>;
   #   VUSE <D.2836_27>;
   thisD.2827_20->functor_D.2399 = D.2836;
   #   VUSE <_A_b1D.2819_9>;
-  D.2837_28 = *_A_bound1D.2820_11;
+  D.2837_28 = _A_b1D.2819;
   #   SFT.88D.2960_30 = V_MUST_DEF <SFT.88D.2960_48>;
   D.2435.bound1_D.2423 = D.2837_28;

and if you diff a may_alias pass before and after CCP you get

@@ -487,20 +489,20 @@
 _A_bound1_11 = { _A_b1 }
 this_12 = { D.2435 D.2435.bound1_ }
 D.2435 = { }
-D.2435.bound1_ = { NULL }
+D.2435.bound1_ = { NULL foo }
 this_13 = { D.2435 D.2435.bound1_ }
 _A_functor_14 = { D.2328 }
 this_15 = { D.2435 D.2435.bound1_ }
 this_16 = { D.2435 D.2435.bound1_ }
 this_17 = { D.2435 D.2435.bound1_ }
 this_18 = { D.2435 D.2435.bound1_ }
-this_19 = { D.2435.bound1_ }
-this_20 = { D.2435.bound1_ }
+this_19 = { D.2435 D.2435.bound1_ }
+this_20 = { D.2435 D.2435.bound1_ }
 _A_functor_21 = { D.2328 }
-this_22 = { D.2435.bound1_ }
-this_23 = { D.2435.bound1_ }
-this_24 = { D.2435.bound1_ }
-this_25 = { D.2435.bound1_ }
+this_22 = { D.2435 D.2435.bound1_ }
+this_23 = { D.2435 D.2435.bound1_ }
+this_24 = { D.2435 D.2435.bound1_ }
+this_25 = { D.2435 D.2435.bound1_ }
 D.2836 = { foo }
 D.2837_28 = { NULL }
 functor_31 = { D.2435 D.2435.bound1_ }
@@ -510,8 +512,6 @@
 functor_35 = { D.2435 D.2435.bound1_ }
 this_36 = { NULL NONLOCAL.105 foo call_it }
 this_37 = { NULL NONLOCAL.105 foo call_it }
-structcopydereftmp.127 = { NULL }
-structcopydereftmp.127.bound1_ = { NULL }
 rep_38 = { NULL NONLOCAL.105 foo call_it }
 this_39 = { D.2301 }
 D.2301 = { NULL NONLOCAL.105 foo call_it }
@@ -524,7 +524,7 @@
 call_it = { NULL NONLOCAL.105 foo call_it }
 D.2967_47 = { call_it }

which should be the most interesting parts of the diff (apart from the
extra vops that prevent the DCE).

Danny?  Any hints on what can go wrong here?
Comment 25 Daniel Berlin 2007-05-14 20:01:32 UTC
> which should be the most interesting parts of the diff (apart from the
> extra vops that prevent the DCE).
> 
> Danny?  Any hints on what can go wrong here?
> 

The first pass is obviously generating wrong answers, unless someone changed the subvars.  
It looks like it's getting confused about where to use the whole object or the first field.

Is that a diff of the dump that includes -details?

I'm trying to figure out if the constraints are bad, or the solving is bad.


Comment 26 Mark Mitchell 2007-05-14 22:25:20 UTC
Will not be fixed in 4.2.0; retargeting at 4.2.1.
Comment 27 Richard Biener 2007-05-15 08:59:24 UTC
Yes, that's a (part of!) a diff of a -details dump with the following patch

Index: passes.c
===================================================================
--- passes.c    (revision 124501)
+++ passes.c    (working copy)
@@ -496,7 +496,9 @@ init_optimization_passes (void)
   NEXT_PASS (pass_early_warn_uninitialized);
 
   /* Initial scalar cleanups.  */
+  NEXT_PASS (pass_may_alias);
   NEXT_PASS (pass_ccp);
+  NEXT_PASS (pass_may_alias);
   NEXT_PASS (pass_fre);
   NEXT_PASS (pass_dce);
   NEXT_PASS (pass_forwprop);

(so it's easy to reproduce the full dump)
Comment 28 Richard Biener 2007-06-04 15:28:10 UTC
solution_set_add () looks like the culprit (thx micha).  So, the following will
fix it in case we have offsets only from COMPONENT_REFs, not from regular
pointer arithmetic (which is not true):

Index: tree-ssa-structalias.c
===================================================================
--- tree-ssa-structalias.c      (revision 125310)
+++ tree-ssa-structalias.c      (working copy)
@@ -715,13 +715,10 @@ solution_set_add (bitmap set, unsigned H
         less than end.  Otherwise, it is globbed to a single
         variable.  */
 
-      if ((get_varinfo (i)->offset + offset) < get_varinfo (i)->fullsize)
+      if (get_varinfo (i)->offset + get_varinfo (i)->size - 1 >= offset
+         && get_varinfo (i)->offset + offset < get_varinfo (i)->fullsize)
        {
-         unsigned HOST_WIDE_INT fieldoffset = get_varinfo (i)->offset + offset;
-         varinfo_t v = first_vi_for_offset (get_varinfo (i), fieldoffset);
-         if (!v)
-           continue;
-         bitmap_set_bit (result, v->id);
+         bitmap_set_bit (result, i);
        }
       else if (get_varinfo (i)->is_artificial_var
               || get_varinfo (i)->has_union
Comment 29 Richard Biener 2007-06-04 15:45:40 UTC
We can make it safe if we only really handle + in pointer arithmetic.  Like with

@@ -3258,7 +3255,7 @@ handle_ptr_arith (VEC (ce_s, heap) *lhsc
   unsigned int i = 0;
   unsigned int j = 0;
   VEC (ce_s, heap) *temp = NULL;
-  unsigned int rhsoffset = 0;
+  unsigned HOST_WIDE_INT rhsoffset = 0;
 
   if (TREE_CODE (expr) != PLUS_EXPR
       && TREE_CODE (expr) != MINUS_EXPR)
@@ -3269,9 +3266,12 @@ handle_ptr_arith (VEC (ce_s, heap) *lhsc
 
   get_constraint_for (op0, &temp);
   if (POINTER_TYPE_P (TREE_TYPE (op0))
-      && TREE_CODE (op1) == INTEGER_CST
+      && host_integerp (op1, 1)
       && TREE_CODE (expr) == PLUS_EXPR)
     {
+      if ((TREE_INT_CST_LOW (op1) * BITS_PER_UNIT) / BITS_PER_UNIT
+         != TREE_INT_CST_LOW (op1))
+       return false;
       rhsoffset = TREE_INT_CST_LOW (op1) * BITS_PER_UNIT;
     }
   else
Comment 30 Richard Biener 2007-06-05 09:27:32 UTC
See this testcase (reduced from spec2k6 dealII by Micha):

  struct I {
    int ix;
    struct II {
      int j,i;
      void *ptr;
    } ii;
  };// inner;
struct O : public I {
//  int x;
  int y;
};

static int geti (struct O *o)
{
  return o->ii.i;
}
extern void init(struct O*);
extern void deinit(struct O*);
extern int getcond(void);
struct O::I *getinner (struct O*);
int reset (int cond)
{

  struct O cell;
  struct O *o;
  struct I::II *in;
  int *pi;
  o = &cell;
  init (o);
  in = &o->ii;
  while (getcond()) {
    if (o->y)
      cell.ii.i = -2;
    if (!getcond()) {
      in->i = -1;
      in->j = o->ii.i;
      break;
    }
  }
  pi = &in->j;
  pi++;
  deinit(&cell);
  return *pi;
}

and note how we compute the wrong points-to solution for in:

o_3 = { cell cell.y cell.ptr cell.i cell.j }
in_4 = { cell.y cell.ptr cell.j }

we are missing cell.i in the points-to set.  This is because with looking
simply for vars with oldoffset + offset is appearantly not enough here.

  cell + 64    ->  cell.j
  cell.y + 64  ->  off structure
  cell.ptr + 64 -> cell.y
  cell.i + 64  ->  cell.ptr (actually in the middle of it)
  cell.j + 64  ->  cell.ptr

so, there is nothing in the structure that, if added 64, will point to
cell.i.  But of course we are taking the address of the substructure and
so we are indeed pointing to cell.i.

So a different solution would be to add _each_ var starting with the
first one + offset.  Which would be simply "reversing" the logic like
I did with the patch (it needs some fixing still, as pruning at the
end is bogus in it as vars may be still reached from others).

The thing that fixes this (30252) bug is that with the original patch we
keep the parent var in the solution.  With something like

Index: tree-ssa-structalias.c
===================================================================
--- tree-ssa-structalias.c      (revision 125310)
+++ tree-ssa-structalias.c      (working copy)
@@ -708,6 +708,21 @@ solution_set_add (bitmap set, unsigned H
   bitmap result = BITMAP_ALLOC (&iteration_obstack);
   unsigned int i;
   bitmap_iterator bi;
+  unsigned HOST_WIDE_INT min = -1, max = 0;
+
+  /* Compute set of vars we can reach from set + offset.  */
+
+  EXECUTE_IF_SET_IN_BITMAP (set, 0, i, bi)
+    {
+      if (get_varinfo (i)->offset + offset < min)
+       min = get_varinfo (i)->offset + offset;
+      if (get_varinfo (i)->offset + get_varinfo (i)->size + offset > max)
+       {
+         max = get_varinfo (i)->offset + get_varinfo (i)->size + offset;
+         if (max > get_varinfo (i)->fullsize)
+           max = get_varinfo (i)->fullsize;
+       }
+    }
 
   EXECUTE_IF_SET_IN_BITMAP (set, 0, i, bi)
     {
@@ -715,13 +730,10 @@ solution_set_add (bitmap set, unsigned H
         less than end.  Otherwise, it is globbed to a single
         variable.  */
 
-      if ((get_varinfo (i)->offset + offset) < get_varinfo (i)->fullsize)
+      if (get_varinfo (i)->offset >= min
+         && get_varinfo (i)->offset < max)
        {
-         unsigned HOST_WIDE_INT fieldoffset = get_varinfo (i)->offset + offset;
-         varinfo_t v = first_vi_for_offset (get_varinfo (i), fieldoffset);
-         if (!v)
-           continue;
-         bitmap_set_bit (result, v->id);
+         bitmap_set_bit (result, i);
        }
       else if (get_varinfo (i)->is_artificial_var
               || get_varinfo (i)->has_union

we don't retain it and this bug is not fixed, but the SPEC bug should be.
Comment 31 Richard Biener 2007-06-05 12:05:16 UTC
The problem in this PR is, that the structure we mess up points-to info has no
fields in its lower half, but only two varinfos (D.2860, offset 128 size 64
and D.2860.visited_, offset 192 size 64 -- the first one is actually D.2860.func_ptr_, but we name the first after the decl always).  Now, there _is_
a structure at offset zero, D.2857 (size 192), which contains D.2860.func_ptr_.

So we fail to have a varinfo for the offset the expression &D.2860.D.2857 points
to and we loose the first varinfo wrongly during the &this_18->functor_
addition of 64.

Now, either we need to generate fake varinfos for the 'holes' we end up with,
or just punt here.
Comment 32 Richard Biener 2007-06-05 15:01:15 UTC
Created attachment 13657 [details]
patch

Patch in testing.  It makes sure to create fields for all addressable components
(such as empty bases) and adjusts solution_set_add to only "remove" no longer
necessary vars (iff we ever would need to add vars, we would be screwed anyway).

Basically all pointer arithmetic (which includes COMPONENT_REFs) handling looks
_very_ fragile in the current setup.
Comment 33 Richard Biener 2007-06-05 15:57:35 UTC
Actually,

+  if (minoffset != 0)

must be changed to

+  if (minoffset != 0 && count != 0)
Comment 34 Daniel Berlin 2007-06-05 16:20:20 UTC
Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0 based code with -fstrict-aliasing

On 5 Jun 2007 09:27:34 -0000, rguenth at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #30 from rguenth at gcc dot gnu dot org  2007-06-05 09:27 -------
> See this testcase (reduced from spec2k6 dealII by Micha):
>
>   struct I {
>     int ix;
>     struct II {
>       int j,i;
>       void *ptr;
>     } ii;
>   };// inner;
> struct O : public I {
> //  int x;
>   int y;
> };
>
> static int geti (struct O *o)
> {
>   return o->ii.i;
> }
> extern void init(struct O*);
> extern void deinit(struct O*);
> extern int getcond(void);
> struct O::I *getinner (struct O*);
> int reset (int cond)
> {
>
>   struct O cell;
>   struct O *o;
>   struct I::II *in;
>   int *pi;
>   o = &cell;
>   init (o);
>   in = &o->ii;
>   while (getcond()) {
>     if (o->y)
>       cell.ii.i = -2;
>     if (!getcond()) {
>       in->i = -1;
>       in->j = o->ii.i;
>       break;
>     }
>   }
>   pi = &in->j;
>   pi++;
>   deinit(&cell);
>   return *pi;
> }
>
> and note how we compute the wrong points-to solution for in:
>
> o_3 = { cell cell.y cell.ptr cell.i cell.j }
> in_4 = { cell.y cell.ptr cell.j }
>

> we are missing cell.i in the points-to set.  This is because with looking
> simply for vars with oldoffset + offset is appearantly not enough here.
>
>   cell + 64    ->  cell.j
>   cell.y + 64  ->  off structure
>   cell.ptr + 64 -> cell.y
>   cell.i + 64  ->  cell.ptr (actually in the middle of it)
>   cell.j + 64  ->  cell.ptr
>
> so, there is nothing in the structure that, if added 64, will point to
> cell.i.  But of course we are taking the address of the substructure and
> so we are indeed pointing to cell.i.
We are pointing to cell, and using that to access cell.i
Not quite the same thing.
But I guess in our world we have to make them the same thing.

> So a different solution would be to add _each_ var starting with the
> first one + offset.

You don't want to play in solution_set_add, which is common to a bunch of code.

The only cases where the above problem comes into play is in offseted
copy constraints, line 1520 (IE the last part of
do_complex_constraint).

In actuality, this is a bug in constraint building.

When we generate the constraints for in = &o->ii, we should be adding
an offsetted copy constraint for each offset of type *o.

This will fix the problem.


> I did with the patch (it needs some fixing still, as pruning at the
> end is bogus in it as vars may be still reached from others).
>

You can never get back to the parent structure from a pointer to the
substructure.
I asked this explicitly and was told it was undefined behavior.
Comment 35 rguenther@suse.de 2007-06-05 16:30:29 UTC
Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0
 based code with -fstrict-aliasing

On Tue, 5 Jun 2007, dberlin at dberlin dot org wrote:

> Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0 based code with
> -fstrict-aliasing
> 
> On 5 Jun 2007 09:27:34 -0000, rguenth at gcc dot gnu dot org
> <gcc-bugzilla@gcc.gnu.org> wrote:
> >
> >
> > ------- Comment #30 from rguenth at gcc dot gnu dot org  2007-06-05 09:27 -------
> > See this testcase (reduced from spec2k6 dealII by Micha):
> >
> >   struct I {
> >     int ix;
> >     struct II {
> >       int j,i;
> >       void *ptr;
> >     } ii;
> >   };// inner;
> > struct O : public I {
> > //  int x;
> >   int y;
> > };
> >
> > static int geti (struct O *o)
> > {
> >   return o->ii.i;
> > }
> > extern void init(struct O*);
> > extern void deinit(struct O*);
> > extern int getcond(void);
> > struct O::I *getinner (struct O*);
> > int reset (int cond)
> > {
> >
> >   struct O cell;
> >   struct O *o;
> >   struct I::II *in;
> >   int *pi;
> >   o = &cell;
> >   init (o);
> >   in = &o->ii;
> >   while (getcond()) {
> >     if (o->y)
> >       cell.ii.i = -2;
> >     if (!getcond()) {
> >       in->i = -1;
> >       in->j = o->ii.i;
> >       break;
> >     }
> >   }
> >   pi = &in->j;
> >   pi++;
> >   deinit(&cell);
> >   return *pi;
> > }
> >
> > and note how we compute the wrong points-to solution for in:
> >
> > o_3 = { cell cell.y cell.ptr cell.i cell.j }
> > in_4 = { cell.y cell.ptr cell.j }
> >
> 
> > we are missing cell.i in the points-to set.  This is because with looking
> > simply for vars with oldoffset + offset is appearantly not enough here.
> >
> >   cell + 64    ->  cell.j
> >   cell.y + 64  ->  off structure
> >   cell.ptr + 64 -> cell.y
> >   cell.i + 64  ->  cell.ptr (actually in the middle of it)
> >   cell.j + 64  ->  cell.ptr
> >
> > so, there is nothing in the structure that, if added 64, will point to
> > cell.i.  But of course we are taking the address of the substructure and
> > so we are indeed pointing to cell.i.
> We are pointing to cell, and using that to access cell.i
> Not quite the same thing.
> But I guess in our world we have to make them the same thing.
> 
> > So a different solution would be to add _each_ var starting with the
> > first one + offset.
> 
> You don't want to play in solution_set_add, which is common to a bunch of code.
> 
> The only cases where the above problem comes into play is in offseted
> copy constraints, line 1520 (IE the last part of
> do_complex_constraint).
> 
> In actuality, this is a bug in constraint building.
> 
> When we generate the constraints for in = &o->ii, we should be adding
> an offsetted copy constraint for each offset of type *o.
> 
> This will fix the problem.

How so?  The solution_set_add method is still wrong.

> > I did with the patch (it needs some fixing still, as pruning at the
> > end is bogus in it as vars may be still reached from others).
> >
> 
> You can never get back to the parent structure from a pointer to the
> substructure.
> I asked this explicitly and was told it was undefined behavior.

Yep, this is what I convinced myself aswell.

Richard.
Comment 36 Daniel Berlin 2007-06-05 17:45:13 UTC
Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0 based code with -fstrict-aliasing

On 5 Jun 2007 16:30:32 -0000, rguenther at suse dot de
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #35 from rguenther at suse dot de  2007-06-05 16:30 -------
> Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0
>  based code with -fstrict-aliasing
>
> On Tue, 5 Jun 2007, dberlin at dberlin dot org wrote:
>
> > Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0 based code with
> > -fstrict-aliasing
> >
> > On 5 Jun 2007 09:27:34 -0000, rguenth at gcc dot gnu dot org
> > <gcc-bugzilla@gcc.gnu.org> wrote:
> > >
> > >
> > > ------- Comment #30 from rguenth at gcc dot gnu dot org  2007-06-05 09:27 -------
> > > See this testcase (reduced from spec2k6 dealII by Micha):
> > >
> > >   struct I {
> > >     int ix;
> > >     struct II {
> > >       int j,i;
> > >       void *ptr;
> > >     } ii;
> > >   };// inner;
> > > struct O : public I {
> > > //  int x;
> > >   int y;
> > > };
> > >
> > > static int geti (struct O *o)
> > > {
> > >   return o->ii.i;
> > > }
> > > extern void init(struct O*);
> > > extern void deinit(struct O*);
> > > extern int getcond(void);
> > > struct O::I *getinner (struct O*);
> > > int reset (int cond)
> > > {
> > >
> > >   struct O cell;
> > >   struct O *o;
> > >   struct I::II *in;
> > >   int *pi;
> > >   o = &cell;
> > >   init (o);
> > >   in = &o->ii;
> > >   while (getcond()) {
> > >     if (o->y)
> > >       cell.ii.i = -2;
> > >     if (!getcond()) {
> > >       in->i = -1;
> > >       in->j = o->ii.i;
> > >       break;
> > >     }
> > >   }
> > >   pi = &in->j;
> > >   pi++;
> > >   deinit(&cell);
> > >   return *pi;
> > > }
> > >
> > > and note how we compute the wrong points-to solution for in:
> > >
> > > o_3 = { cell cell.y cell.ptr cell.i cell.j }
> > > in_4 = { cell.y cell.ptr cell.j }
> > >
> >
> > > we are missing cell.i in the points-to set.  This is because with looking
> > > simply for vars with oldoffset + offset is appearantly not enough here.
> > >
> > >   cell + 64    ->  cell.j
> > >   cell.y + 64  ->  off structure
> > >   cell.ptr + 64 -> cell.y
> > >   cell.i + 64  ->  cell.ptr (actually in the middle of it)
> > >   cell.j + 64  ->  cell.ptr
> > >
> > > so, there is nothing in the structure that, if added 64, will point to
> > > cell.i.  But of course we are taking the address of the substructure and
> > > so we are indeed pointing to cell.i.
> > We are pointing to cell, and using that to access cell.i
> > Not quite the same thing.
> > But I guess in our world we have to make them the same thing.
> >
> > > So a different solution would be to add _each_ var starting with the
> > > first one + offset.
> >
> > You don't want to play in solution_set_add, which is common to a bunch of code.
> >
> > The only cases where the above problem comes into play is in offseted
> > copy constraints, line 1520 (IE the last part of
> > do_complex_constraint).
> >
> > In actuality, this is a bug in constraint building.
> >
> > When we generate the constraints for in = &o->ii, we should be adding
> > an offsetted copy constraint for each offset of type *o.
> >
> > This will fix the problem.
>
> How so?  The solution_set_add method is still wrong.

The constraints say to add this offset to the members of the points to
sets of these variables, and put the result over here.
As long as solution_set_add is doing that, it is not the problem.

If it does not generate correct results, it is the constraints that
are wrong, not solution_set_add
Comment 37 Michael Matz 2007-06-05 18:24:52 UTC
> We are pointing to cell, and using that to access cell.i

No.  We are pointing to cell.ii, and use that pointer to access cell.ii.i
(via in->i).  Hence of course the points-to set of 'in' needs to include
cell.in.i (or cell.i, as the point-to debug dump likes to call it).  At least
how the points-to solver is used.  Theoretically it would of course be more
sensible to only note that 'in' can point to 'cell.ii', and make the user
of that information recognize that this actually is a structure, and hence
over that pointer you can access all members of it.  But that's not
how the info is used, so we have to include cell.ii.i in the points to set
(which in our case rather is a can-access set).

Now, regarding where to fix it properly: if you say that solution_set_add()
is not the right place (and I don't buy that yet, because something is wrong
with it no matter what, e.g. looking for subfields starting from the things
in the pt set, which themself might be subfields feels wrong), then we must
not use pointer arithmetic (or at least '+' constraints) to represent pointing
to substructures.  And I'm not sure that's easily doable.  Because the insn
we see is not "in = &cell.in", but "in = &o->in", and only the pt solver
itself sees that 'o' points to cell.  So what constraints do you suggest
should be added instead of offseted constraints for this insn?

> You don't want to play in solution_set_add, which is common to a bunch of 
> code.

Well, it's common to exactly the code which handles offsetted constraints.
And I don't see why the problem we face here (with offseted copy constraints)
doesn't also occur with any other of the offseted constraints, e.g.
"*x = y+32", handled in do_ds_constraint.

> > You can never get back to the parent structure from a pointer to the
> > substructure.
> > I asked this explicitly and was told it was undefined behavior.
> 
> Yep, this is what I convinced myself aswell.

Well, it's perhaps illegal in C (I'm not sure about that actually), but
certainly occurs in C++, and even if it didn't occur there we are talking
about the middle-end, for which such invalidity shouldn't matter (except we
choose to define it as invalid).  To see why this is valid, consider this
C++ program:

struct A {int a;};
struct B {int b;};
struct C : public A, B { int c;};
C* b2c (B* b) { return (C*)b; }
C* get_c (void)
{
  static C c;
  B *pb;
  C *pc;
  pb = &c;
  pc = (C*)pb;
  return pc;
}

This is completely valid (suppose b2c is only ever called with pointers
to the B-in-C base of any C), and results in this insn:

  b.2_3 = (struct C *) b_2(D);
  D.2496_4 = b.2_3 - 4;

A better example might be the other function 'get_c', which also has the
subtraction:

  pb_3 = (struct CD.2478 *) &cD.2495.D.2482;
  pc_4 = pb_3 - 4;
  return pc_4;

This currently only works because points-to gives up on subtraction and 
adds INTEGER to the access set of pc_4 (additionally to c.b and c.c; c.a
or c itself are missing).
Comment 38 Daniel Berlin 2007-06-05 19:07:14 UTC
Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0 based code with -fstrict-aliasing

On 5 Jun 2007 18:24:54 -0000, matz at gcc dot gnu dot org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #37 from matz at gcc dot gnu dot org  2007-06-05 18:24 -------
> > We are pointing to cell, and using that to access cell.i
>
> No.  We are pointing to cell.ii, and use that pointer to access cell.ii.i
> (via in->i).  Hence of course the points-to set of 'in' needs to include
> cell.in.i (or cell.i, as the point-to debug dump likes to call it).  At least
> how the points-to solver is used.
>  Theoretically it would of course be more
> sensible to only note that 'in' can point to 'cell.ii', and make the user
> of that information recognize that this actually is a structure, and hence
> over that pointer you can access all members of it.  But that's not
> how the info is used, so we have to include cell.ii.i in the points to set
> (which in our case rather is a can-access set).

Or we have to fix how it is used to handle these issues there.  :)

Honestly, doing the second would result in much much saner points-to
constraint generation and solving.

Probably better results, too.
Now that we calculate the set of loaded/stored symbols on a
per-statement basis, it may be quite easy to do.

>
> Now, regarding where to fix it properly: if you say that solution_set_add()
> is not the right place (and I don't buy that yet, because something is wrong
> with it no matter what, e.g. looking for subfields starting from the things
> in the pt set, which themself might be subfields feels wrong)
You keep thinking the solver knows anything about types or structures
or fields or programming languages or fields.
It doesn't.
The only thing it knows is that constraint variable + 5 is some other
constraint variable.

Clear your mind of any relation to the original program variables.
The variables the solver is talking about need to be mapped back to
original program variables.  They are simply a unique set of
variables, some of which may be accessed by offseting something else.

All relations of the original variables need to be expressed to the
solver in constraints. The solver does not, and should not, know
anything about programming languages and their type systems :)

> not use pointer arithmetic (or at least '+' constraints) to represent pointing
> to substructures.  And I'm not sure that's easily doable.  Because the insn
> we see is not "in = &cell.in", but "in = &o->in", and only the pt solver
> itself sees that 'o' points to cell.

for in = &o->in, we should add

in = o + <offset> constraints

for every offset that can exist in the typeof("o->in").

And we should be doing this inside the various structure copy
constraint builders, but it's quite possible i fucked it up.

This would give you what you want, and make in point to cell.in.ii

Note that this is all a side-effect of trying to shoehorn may-alias
info directly into points-to sets, because our system thinks that a->i
is an access only to whatever a can point-to, instead of what a->i can
point to.

> > You don't want to play in solution_set_add, which is common to a bunch of
> > code.
>
> Well, it's common to exactly the code which handles offsetted constraints.
> And I don't see why the problem we face here (with offseted copy constraints)
> doesn't also occur with any other of the offseted constraints, e.g.
> "*x = y+32", handled in do_ds_constraint.

*x = y + 32 is an invalid constraint.

do_ds_constraint can't actually ever have a call where roffset is not
zero.  I've actually made this explicit on the pta-dev branch.

(you'll note do_sd_constraint also can't have loffset != zero).


>
> > > You can never get back to the parent structure from a pointer to the
> > > substructure.
> > > I asked this explicitly and was told it was undefined behavior.
> >
> > Yep, this is what I convinced myself aswell.
>
> Well, it's perhaps illegal in C (I'm not sure about that actually),

Joseph told me it was
>  but
> certainly occurs in C++, and even if it didn't occur there we are talking
> about the middle-end, for which such invalidity shouldn't matter (except we
> choose to define it as invalid).  To see why this is valid, consider this
> C++ program:
This is done through subtraction in C++, and we explicitly don't support it.

If we can't figure out what is going on, we collapse the whole thing
to a single variable and call it a day.
Comment 39 rguenther@suse.de 2007-06-05 19:59:30 UTC
Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0
 based code with -fstrict-aliasing

On Tue, 5 Jun 2007, dberlin at dberlin dot org wrote:

> ------- Comment #38 from dberlin at gcc dot gnu dot org  2007-06-05 19:07 -------
> Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0 based code with
> -fstrict-aliasing
> 
> On 5 Jun 2007 18:24:54 -0000, matz at gcc dot gnu dot org
> <gcc-bugzilla@gcc.gnu.org> wrote:
> >
> > Now, regarding where to fix it properly: if you say that solution_set_add()
> > is not the right place (and I don't buy that yet, because something is wrong
> > with it no matter what, e.g. looking for subfields starting from the things
> > in the pt set, which themself might be subfields feels wrong)
> You keep thinking the solver knows anything about types or structures
> or fields or programming languages or fields.
> It doesn't.
> The only thing it knows is that constraint variable + 5 is some other
> constraint variable.

But there are cases there is no constraint variable for p + 5.  Like
the problem with the empty bases that we take the address of but don't
generate constraint variables for.  Or consider

int p[2];
void *q = p;
int *p2;
q = q + 1;
q = q + 1;
q = q + 1;
q = q + 1;
p2 = q;
*p2 = 1;

which is a valid way to store into p[1].  We obviously don't have
constraint variables for q + 1 -- we have one for &p[0] and one
for &p[1], but we cannot represent q + 1 exactly.  Somehow it
doesn't cause problems to "fall back" to &p[0] for &p[0] + 1B,
but it would be nice to have some more confidence in that parts :)

Richard.
Comment 40 Daniel Berlin 2007-06-05 22:44:05 UTC
Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0 based code with -fstrict-aliasing

On 5 Jun 2007 19:59:32 -0000, rguenther at suse dot de
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #39 from rguenther at suse dot de  2007-06-05 19:59 -------
> Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0
>  based code with -fstrict-aliasing
>
> On Tue, 5 Jun 2007, dberlin at dberlin dot org wrote:
>
> > ------- Comment #38 from dberlin at gcc dot gnu dot org  2007-06-05 19:07 -------
> > Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0 based code with
> > -fstrict-aliasing
> >
> > On 5 Jun 2007 18:24:54 -0000, matz at gcc dot gnu dot org
> > <gcc-bugzilla@gcc.gnu.org> wrote:
> > >
> > > Now, regarding where to fix it properly: if you say that solution_set_add()
> > > is not the right place (and I don't buy that yet, because something is wrong
> > > with it no matter what, e.g. looking for subfields starting from the things
> > > in the pt set, which themself might be subfields feels wrong)
> > You keep thinking the solver knows anything about types or structures
> > or fields or programming languages or fields.
> > It doesn't.
> > The only thing it knows is that constraint variable + 5 is some other
> > constraint variable.
>
> But there are cases there is no constraint variable for p + 5.  Like
> the problem with the empty bases that we take the address of but don't
> generate constraint variables for.  Or consider
>
> int p[2];
> void *q = p;
> int *p2;
> q = q + 1;
> q = q + 1;
> q = q + 1;
> q = q + 1;
> p2 = q;
> *p2 = 1;
>
> which is a valid way to store into p[1].  We obviously don't have
> constraint variables for q + 1 -- we have one for &p[0] and one
> for &p[1], but we cannot represent q + 1 exactly.

q_2 = q_1 + 1
q_3 = q_2 + 1
q_4 = q_3 + 1
q_5 = q_4 + 1
p2 = q_5

Will become the set of constraints
q_2 = q_1
q_2 = q_1 + 1
q_3 = q_2
q_3 = q_2 + 1
q_4 = q_3
q_4 = q_3 + 1
q_5 = q_4
q_5 = q_4 +1

Hmmm.
You are right, we should give up here and collapse the variable.
Sigh.

Well, we can always just make it give up on all direct pointer
arithmetic, rather than try to handle it at all :)

> Somehow it
> doesn't cause problems to "fall back" to &p[0] for &p[0] + 1B,
> but it would be nice to have some more confidence in that parts :)

TBQH, the only way i'll ever have confidence in these parts is if we
make it stop trying to come up with everything a pointer to a
structure could deref.
Note that this also prevents us from doing cool things.

In particular, because we say a pointer to struct p points to all it's
members, we can never simply propagate the object it does point to if
it points to a single object.
Comment 41 rguenther@suse.de 2007-06-06 08:49:49 UTC
Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0
 based code with -fstrict-aliasing

On Wed, 5 Jun 2007, dberlin at dberlin dot org wrote:

> q_2 = q_1 + 1
> q_3 = q_2 + 1
> q_4 = q_3 + 1
> q_5 = q_4 + 1
> p2 = q_5
> 
> Will become the set of constraints
> q_2 = q_1
> q_2 = q_1 + 1
> q_3 = q_2
> q_3 = q_2 + 1
> q_4 = q_3
> q_4 = q_3 + 1
> q_5 = q_4
> q_5 = q_4 +1
> 
> Hmmm.
> You are right, we should give up here and collapse the variable.
> Sigh.
> 
> Well, we can always just make it give up on all direct pointer
> arithmetic, rather than try to handle it at all :)
> 
> > Somehow it
> > doesn't cause problems to "fall back" to &p[0] for &p[0] + 1B,
> > but it would be nice to have some more confidence in that parts :)
> 
> TBQH, the only way i'll ever have confidence in these parts is if we
> make it stop trying to come up with everything a pointer to a
> structure could deref.

Now, this is of course only because of aliasing.  It should be
completely irrelevant for points-to.  So, if points-to comes up with
"only" &struct as a solution we would need some intermediate processing
to translate it to the correct alias set, possibly looking at the
type hierarchy again.

> Note that this also prevents us from doing cool things.

Too bad ;)

> In particular, because we say a pointer to struct p points to all it's
> members, we can never simply propagate the object it does point to if
> it points to a single object.

So we seem to agree the current solution is ugly and doesn't really
work.  Enough time to fix it for 4.3 then ;)

Btw, how does a patch to disable field sensitivity look like for the 4.2
branch?

Richard.
Comment 42 Daniel Berlin 2007-06-06 15:52:26 UTC
Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0 based code with -fstrict-aliasing

On 6 Jun 2007 08:49:50 -0000, rguenther at suse dot de
<gcc-bugzilla@gcc.gnu.org> wrote:
>
>
> ------- Comment #41 from rguenther at suse dot de  2007-06-06 08:49 -------
> Subject: Re:  [4.2 regression] miscompilation of sigc++-2.0
>  based code with -fstrict-aliasing
>
> On Wed, 5 Jun 2007, dberlin at dberlin dot org wrote:
>
> > q_2 = q_1 + 1
> > q_3 = q_2 + 1
> > q_4 = q_3 + 1
> > q_5 = q_4 + 1
> > p2 = q_5
> >
> > Will become the set of constraints
> > q_2 = q_1
> > q_2 = q_1 + 1
> > q_3 = q_2
> > q_3 = q_2 + 1
> > q_4 = q_3
> > q_4 = q_3 + 1
> > q_5 = q_4
> > q_5 = q_4 +1
> >
> > Hmmm.
> > You are right, we should give up here and collapse the variable.
> > Sigh.
> >
> > Well, we can always just make it give up on all direct pointer
> > arithmetic, rather than try to handle it at all :)
> >
> > > Somehow it
> > > doesn't cause problems to "fall back" to &p[0] for &p[0] + 1B,
> > > but it would be nice to have some more confidence in that parts :)
> >
> > TBQH, the only way i'll ever have confidence in these parts is if we
> > make it stop trying to come up with everything a pointer to a
> > structure could deref.
>
> Now, this is of course only because of aliasing.  It should be
> completely irrelevant for points-to.  So, if points-to comes up with
> "only" &struct as a solution we would need some intermediate processing
> to translate it to the correct alias set, possibly looking at the
> type hierarchy again.
>

Well, actually, you don't.

At the point where you process it, we either have created SFT's or we
haven't (if we haven't than the deref is going to touch the one
variable we've created for the struct)
The rest is more or less equivalent to what access_can_touch_variable does :)
> So we seem to agree the current solution is ugly and doesn't really
> work.  Enough time to fix it for 4.3 then ;)
>
> Btw, how does a patch to disable field sensitivity look like for the 4.2
> branch?

The patch is trivial.  We have static bool use_field_sensitive = true
at the top of the file.  Just set it to false and away you go.
Comment 43 Richard Biener 2007-06-19 09:24:47 UTC
Subject: Bug 30252

Author: rguenth
Date: Tue Jun 19 09:24:35 2007
New Revision: 125844

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=125844
Log:
2007-06-19  Richard Guenther  <rguenther@suse.de>
	Michael Matz  <matz@suse.de>

	PR tree-optimization/30252
	* tree-ssa-structalias.c (solution_set_add): Make sure to
	preserve all relevant vars.
	(handle_ptr_arith): Make sure to only handle positive
	offsets.
	(push_fields_onto_fieldstack): Create fields for empty
	bases.

	* g++.dg/opt/pr30252.C: New testcase.

Added:
    branches/gcc-4_2-branch/gcc/testsuite/g++.dg/opt/pr30252.C
Modified:
    branches/gcc-4_2-branch/gcc/ChangeLog
    branches/gcc-4_2-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_2-branch/gcc/tree-ssa-structalias.c

Comment 44 Richard Biener 2007-06-19 09:45:50 UTC
Fixed on the 4.2 branch.  Danny will fix this in a different way on the trunk.