[Bug libstdc++/42832] New: Revisit std::function for aliasing issues

[Bug libstdc++/42832] New: Revisit std::function for aliasing issues

[paolo dot carlini at oracle dot com]

This is to keep track of this exchange with Richard:

  http://gcc.gnu.org/ml/gcc-patches/2010-01/msg01154.html


-- 
           Summary: Revisit std::function for aliasing issues
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: paolo dot carlini at oracle dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[paolo dot carlini at oracle dot com]

-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2010-01-21 16:47:22
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[paolo dot carlini at oracle dot com]

------- Comment #1 from paolo dot carlini at oracle dot com  2010-01-21 16:58 -------
Let's add Richard too.


-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[paolo dot carlini at oracle dot com]

------- Comment #2 from paolo dot carlini at oracle dot com  2010-01-24 18:42 -------
Richard, I'm sorry, I realize now that I'm confused about an important point:
does your analysis of function::swap mean that we are *already* miscompiling
it? Or, are we going to commit patches which will lead to miscompilations in
4.5?

Because otherwise, I don't really think it makes sense to have an interim
version of the code using std::memcpy (at least not for the C+0x version): for
4.6.0 we could as well move directly to the optimized but correct solution - in
other terms we didn't really understand each other the last week, and this
issue should not depend on 42834, on your 42845 instead and should be targeted
to 4.6.0.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[rguenther at suse dot de]

------- Comment #3 from rguenther at suse dot de  2010-01-24 20:50 -------
Subject: Re:  Revisit std::function for aliasing
 issues

On Sun, 24 Jan 2010, paolo dot carlini at oracle dot com wrote:

> ------- Comment #2 from paolo dot carlini at oracle dot com  2010-01-24 18:42 -------
> Richard, I'm sorry, I realize now that I'm confused about an important point:
> does your analysis of function::swap mean that we are *already* miscompiling
> it?

I did not yet observe miscompilations - it for now is a latent issue only.

> Or, are we going to commit patches which will lead to miscompilations in
> 4.5?

No, I planned to - but after the analysis I decided to postpone them.
The issue is the patches only improve RTL analysis - I see nothing that
blocks the issue to show up on the tree level, but we seem to be
lucky there.  On RTL we do instruction scheduling which does
include unusual movement of instructions, like sinking loads or
hoisting stores - that might be the reason we are lucky on the
tree level.

> Because otherwise, I don't really think it makes sense to have an interim
> version of the code using std::memcpy (at least not for the C+0x version): for
> 4.6.0 we could as well move directly to the optimized but correct solution - in
> other terms we didn't really understand each other the last week, and this
> issue should not depend on 42834, on your 42845 instead and should be targeted
> to 4.6.0.

Well - that's up to you.  libstdc++ violates aliasing rules, and I
expect that sooner or later there will be a testcase that is miscompiled
with the existing 4.5 codebase.

A fix using memcpy certainly depends on fixing 42834, thus the
dependency.  It blocks PR42617 whose fixes expose this bug, that
bug isn't marked as regression though it is (heh - nobody besides
me noticed that).

Thus, it is up to you - if you want to fix this for 4.5 then
I have to fix PR42834 (because you do not consider a fix not
using memcpy).  PR42834 isn't a regression either - 4.4 was much
less compliant wrt C++ dynamic types.

Hope this helps and doesn't add more confusion ;)

Richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[rguenth at gcc dot gnu dot org]

------- Comment #4 from rguenth at gcc dot gnu dot org  2010-01-24 21:22 -------
Blocks improvement/regression fix for PR42617 (has patches attached to
reproduce
this bug).


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
OtherBugsDependingO|                            |42617
              nThis|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[paolo dot carlini at oracle dot com]

------- Comment #5 from paolo dot carlini at oracle dot com  2010-01-25 10:10 -------
Ok, I see that asking was a good idea, the tangle of issues is quite complex.
Anyway, this is my current proposal: let' use for now your mildly optimized and
in any case correct solution using a loop of char-wise std::swap, the first
solution you proposed on the gcc-patches discussion. For both std and tr1.
Likely tr1 will stay like that forever. Let's remove the dependency of this bug
on the overzealous memcpy optimization bug and the blocking of the optimization
issue which started this discussion; let's add instead dependency on the
"uninitialized_swap" enhancement. Let's keep the present bug open of course, as
enhancement.

Are you willing to prepare a patch including a comment, better two different
comments one simpler for the tr1 version and one slightly longer for the std
version? 


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[rguenther at suse dot de]

------- Comment #6 from rguenther at suse dot de  2010-01-25 16:29 -------
Subject: Re:  Revisit std::function for aliasing
 issues

On Mon, 25 Jan 2010, paolo dot carlini at oracle dot com wrote:

> ------- Comment #5 from paolo dot carlini at oracle dot com  2010-01-25 10:10 -------
> Ok, I see that asking was a good idea, the tangle of issues is quite complex.
> Anyway, this is my current proposal: let' use for now your mildly optimized and
> in any case correct solution using a loop of char-wise std::swap, the first
> solution you proposed on the gcc-patches discussion. For both std and tr1.
> Likely tr1 will stay like that forever. Let's remove the dependency of this bug
> on the overzealous memcpy optimization bug and the blocking of the optimization
> issue which started this discussion; let's add instead dependency on the
> "uninitialized_swap" enhancement. Let's keep the present bug open of course, as
> enhancement.
> 
> Are you willing to prepare a patch including a comment, better two different
> comments one simpler for the tr1 version and one slightly longer for the std
> version? 

I'll let the dust settle a bit on the gcc-patches discussion;  I can
then post a patch along the above lines.

Thanks,
Richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[paolo dot carlini at oracle dot com]

------- Comment #7 from paolo dot carlini at oracle dot com  2010-01-25 17:36 -------
Ageeed. Indeed, I see there is a discussion ongoing. In the present time frame
let's figure out something minimally invasive, decently (if not perfectly)
optimized, and minimally coupled one way or the other to the other open issues.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[rguenth at gcc dot gnu dot org]

------- Comment #9 from rguenth at gcc dot gnu dot org  2010-01-26 14:05 -------
Mine.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |rguenth at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2010-01-21 16:47:22         |2010-01-26 14:05:03
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[rguenth at gcc dot gnu dot org]

------- Comment #8 from rguenth at gcc dot gnu dot org  2010-01-26 14:04 -------
Created an attachment (id=19711)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=19711&action=view)
patch

Candidate fix.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[paolo dot carlini at oracle dot com]

------- Comment #10 from paolo dot carlini at oracle dot com  2010-01-26 14:25 -------
I'm not completely sure that the tr1 version actually includes indirectly
std::swap. Please check that and in case just add bits/move.h, is enough and
works also in non-C++0x mode.

Otherwise, you are cleared to commit at your ease (please remember to update
the dependencies)

Thanks!


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[rguenther at suse dot de]

------- Comment #11 from rguenther at suse dot de  2010-01-26 14:34 -------
Subject: Re:  Revisit std::function for aliasing
 issues

On Tue, 26 Jan 2010, paolo dot carlini at oracle dot com wrote:

> ------- Comment #10 from paolo dot carlini at oracle dot com  2010-01-26 14:25 -------
> I'm not completely sure that the tr1 version actually includes indirectly
> std::swap. Please check that and in case just add bits/move.h, is enough and
> works also in non-C++0x mode.
> 
> Otherwise, you are cleared to commit at your ease (please remember to update
> the dependencies)

Actually the tr1 version does but the std version does not.  I'll adjust
that instead.  What dependencies do you mean?

richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[paolo dot carlini at oracle dot com]

------- Comment #12 from paolo dot carlini at oracle dot com  2010-01-26 14:37 -------
Are you sure? The std version should be fine, via <utility> (remember the std
version is enabled *only* in C++0x mode).

I mean the "depends on" and "blocks" fields.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[rguenther at suse dot de]

------- Comment #13 from rguenther at suse dot de  2010-01-26 14:57 -------
Subject: Re:  Revisit std::function for aliasing
 issues

On Tue, 26 Jan 2010, paolo dot carlini at oracle dot com wrote:

> ------- Comment #12 from paolo dot carlini at oracle dot com  2010-01-26 14:37 -------
> Are you sure? The std version should be fine, via <utility> (remember the std
> version is enabled *only* in C++0x mode).

Ah, with -std=c++0x it's there indeed.

> I mean the "depends on" and "blocks" fields.

Ok

Richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues

[rguenth at gcc dot gnu dot org]

------- Comment #14 from rguenth at gcc dot gnu dot org  2010-01-27 15:49 -------
Subject: Bug 42832

Author: rguenth
Date: Wed Jan 27 15:49:00 2010
New Revision: 156290

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=156290
Log:
2010-01-27  Richard Guenther  <rguenther@suse.de>

        PR libstdc++/42832
        * include/std/functional (function<>::swap): Perform bytewise
        swap of _M_functor.
        * include/tr1/functional (function<>::swap): Likewise.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/std/functional
    trunk/libstdc++-v3/include/tr1/functional


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues and efficiency

[rguenth at gcc dot gnu dot org]

------- Comment #15 from rguenth at gcc dot gnu dot org  2010-01-27 15:51 -------
Doesn't depend on fixed memcpy, no longer blocks 42617.  Depends on 42845
for enhancing the fix.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  BugsThisDependsOn|42834                       |42845
OtherBugsDependingO|42617                       |
              nThis|                            |
         AssignedTo|rguenth at gcc dot gnu dot  |unassigned at gcc dot gnu
                   |org                         |dot org
           Severity|normal                      |enhancement
             Status|ASSIGNED                    |NEW
            Summary|Revisit std::function for   |Revisit std::function for
                   |aliasing issues             |aliasing issues and
                   |                            |efficiency


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues and efficiency

[paolo dot carlini at oracle dot com]

------- Comment #16 from paolo dot carlini at oracle dot com  2010-01-27 15:57 -------
Thanks!


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues and efficiency

[jason at gcc dot gnu dot org]

------- Comment #17 from jason at gcc dot gnu dot org  2010-05-20 15:13 -------
Hmm...I tried reverting the change to functional::swap, but all the libstdc++
tests still pass on x86_64-linux, and I haven't been able to write a failing
testcase either.  Anyone else have a testcase that still fails with 4.5.1pre or
trunk?  For instance, this doesn't break when compiled either as C or C++:

union __attribute ((aligned (__alignof (float)))) U { char c[32]; };

int main ()
{
  union U u1, u2;
  *(float*)(u1.c) = 42.0;
  u2 = u1;
  if (*(float*)(&u2) != 42.0)
    return 1;
  return 0;
}


-- 

jason at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues and efficiency

[rguenther at suse dot de]

------- Comment #18 from rguenther at suse dot de  2010-05-20 15:17 -------
Subject: Re:  Revisit std::function for aliasing issues
 and efficiency

On Thu, 20 May 2010, jason at gcc dot gnu dot org wrote:

> ------- Comment #17 from jason at gcc dot gnu dot org  2010-05-20 15:13 -------
> Hmm...I tried reverting the change to functional::swap, but all the libstdc++
> tests still pass on x86_64-linux, and I haven't been able to write a failing
> testcase either.  Anyone else have a testcase that still fails with 4.5.1pre or
> trunk?  For instance, this doesn't break when compiled either as C or C++:
> 
> union __attribute ((aligned (__alignof (float)))) U { char c[32]; };

That's because the union now effectively uses alias-set zero after

2010-04-15  Richard Guenther  <rguenther@suse.de>

        * alias.c (alias_set_subset_of): Handle alias-set zero
        child properly.

which is also a pessimization (because we use alias-set zero for
structs with chars, not only char lvalues).

A testcase that breaks would use a union without a character member
(where you'd still need to copy the object representation).

So like

union { int i; } u,v;

float *f = new (&u) float(0.0;
v = u;
return (float *)&v;

Richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues and efficiency

[jason at gcc dot gnu dot org]

------- Comment #19 from jason at gcc dot gnu dot org  2010-05-20 15:40 -------
Aha.  That sounds good to me; I don't expect your testcase to work, because it
doesn't involve a char buffer.  The copy does need to use the object
representation, but I think the new-expression should be undefined (though the
standard needs fixing to make it so--this is core issue 1027).

So, can we revert the function::swap change for 4.6, then?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues and efficiency

[rguenther at suse dot de]

------- Comment #20 from rguenther at suse dot de  2010-05-20 15:44 -------
Subject: Re:  Revisit std::function for aliasing issues
 and efficiency

On Thu, 20 May 2010, jason at gcc dot gnu dot org wrote:

> ------- Comment #19 from jason at gcc dot gnu dot org  2010-05-20 15:40 -------
> Aha.  That sounds good to me; I don't expect your testcase to work, because it
> doesn't involve a char buffer.  The copy does need to use the object
> representation, but I think the new-expression should be undefined (though the
> standard needs fixing to make it so--this is core issue 1027).
> 
> So, can we revert the function::swap change for 4.6, then?

Well.  It is probably safe (though I'd like to know how 44164 is
exactly broken - backporting that change doesn't fix that testcase).

Richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues and efficiency

[jason at gcc dot gnu dot org]

------- Comment #21 from jason at gcc dot gnu dot org  2010-05-20 16:49 -------
The alias_set_subset_of change isn't in 4.5?  Then that's not what's making the
libstdc++ function tests pass with the swap change reverted...:)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues and efficiency

[rguenther at suse dot de]

------- Comment #22 from rguenther at suse dot de  2010-05-21 08:28 -------
Subject: Re:  Revisit std::function for aliasing issues
 and efficiency

On Thu, 20 May 2010, jason at gcc dot gnu dot org wrote:

> ------- Comment #21 from jason at gcc dot gnu dot org  2010-05-20 16:49 -------
> The alias_set_subset_of change isn't in 4.5?  Then that's not what's making the
> libstdc++ function tests pass with the swap change reverted...:)

Well, yeah - I wouldn't have expected that (IIRC it was a scheduler
issue as well).

Richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues and efficiency

[jason at gcc dot gnu dot org]

------- Comment #23 from jason at gcc dot gnu dot org  2010-05-27 18:40 -------
Subject: Bug 42832

Author: jason
Date: Thu May 27 18:39:46 2010
New Revision: 159940

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=159940
Log:
        Revert:
        PR libstdc++/42832
        * include/std/functional (function<>::swap): Perform bytewise
        swap of _M_functor.
        * include/tr1/functional (function<>::swap): Likewise.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/std/functional
    trunk/libstdc++-v3/include/tr1/functional


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832

[Bug libstdc++/42832] Revisit std::function for aliasing issues and efficiency

[paolo dot carlini at oracle dot com]

------- Comment #24 from paolo dot carlini at oracle dot com  2010-05-27 20:52 -------
Can be closed.


-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42832