Bug 43075 - [4.5 Regression] 20_util/bind/ref2.cc FAILs
Summary: [4.5 Regression] 20_util/bind/ref2.cc FAILs
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.5.0
: P1 normal
Target Milestone: 4.5.0
Assignee: Jason Merrill
URL:
Keywords: wrong-code
Depends on:
Blocks: 43787
  Show dependency treegraph
 
Reported: 2010-02-15 11:26 UTC by Richard Biener
Modified: 2010-05-01 00:58 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-02-15 14:53:16


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Biener 2010-02-15 11:26:01 UTC
make check-target-libstdc++-v3 RUNTESTFLAGS="--target_board=unix/-O0 conformance.exp=20_util/bind/ref2.cc"

FAIL: 20_util/bind/ref2.cc execution test

                === libstdc++ Summary ===

# of expected passes            1
# of unexpected failures        1
Comment 1 Paolo Carlini 2010-02-15 11:44:54 UTC
If you really think this is a purely libstdc++ issue we gonna need *a lot* of help from the compiler people. Also consider that Jonathan, the author of the recent improvements to std::bind, will be in vacations for 2 weeks.

Anyway, how can this be a [4.5 Regression] if the testcase didn't exist in 4.4 and, more specifically, uses C++0x features which do not make sense together with the 4.4 std::bind, which is just was the std::tr1::bind in the std:: namespace?
Comment 2 Richard Biener 2010-02-15 12:19:14 UTC
(In reply to comment #1)
> If you really think this is a purely libstdc++ issue we gonna need *a lot* of
> help from the compiler people. Also consider that Jonathan, the author of the
> recent improvements to std::bind, will be in vacations for 2 weeks.

Well, usually FAILs at -O0 but not -O2 hint at lifetime problems such as
references to local vars being returned.  Note that a patch of mine exposes
this issue at -O2 ... so it's blocked by this issue.

> Anyway, how can this be a [4.5 Regression] if the testcase didn't exist in 4.4
> and, more specifically, uses C++0x features which do not make sense together
> with the 4.4 std::bind, which is just was the std::tr1::bind in the std::
> namespace?

So the testcase is bogus?  Then please remove it.

I marked it as a regression prematurely because the patch I really want to
apply will expose it at -O2 - which then makes it a regression against
an earlier revision of trunk.

And I'm quite lost in the myriads of variadic templates when trying to
figure out what is going wrong (I tried for several hours already).
Comment 3 Paolo Carlini 2010-02-15 12:27:51 UTC
> So the testcase is bogus?  Then please remove it.

Nobody said is bogus. I said it didn't exist in 4.4, thus can't be a regression. Maybe we should xfail it, if we cannot understand in time what's going on.

Comment 4 Richard Biener 2010-02-15 13:16:22 UTC
It also fails with -O1 and -O2 -fno-strict-aliasing and with -O2 -fno-inline.

Assembler differences for -O2 vs. -O2 with my patch (which effectively
makes us see more must-aliases, thus "accept" slightly invalid
strict-aliasing violating code):

--- ref2.s.good 2010-02-15 13:47:03.000000000 +0100
+++ ref2.s.bad  2010-02-15 13:46:34.000000000 +0100
@@ -90,15 +90,16 @@
        .cfi_startproc
        subq    $40, %rsp
        .cfi_def_cfa_offset 48
-       leaq    28(%rsp), %rsi
+       leaq    28(%rsp), %rdx
        movl    $1, 28(%rsp)
        movq    $_ZNK3Inc1fERi, (%rsp)
        movq    $0, 8(%rsp)
        leaq    16(%rsp), %rdi
-       movq    %rsi, 16(%rsp)
+       movq    %rdx, 16(%rsp)
        movb    %al, 16(%rsp)
        movl    $_ZNK3Inc1fERi, %eax
        testb   $1, %al
+       movq    16(%rsp), %rsi
        je      .L10
        movq    _ZNK3Inc1fERi-1(%rsi), %rax
 .L10:


that's _Z6test02v.  You can see the aliasing byte-store to 16(%rsp) and
the probably seemingly redundant load from 16(%rsp) that we maybe remove
with strict-aliasing on.

The tree code for the above is at .optimize time:

<bb 2>:
  counter = 1;
  D.29754 = {};
  __bound_args#0 = D.29754;
  D.25693._M_f.__pmf.__pfn = f;
  D.25693._M_f.__pmf.__delta = 0;
  D.25693._M_bound_args.D.25507.D.25257.D.25050._M_head_impl._M_data = &counter;
  this.24_34 = (struct Inc *) &D.25693._M_bound_args.D.25507;
  *this.24_34 = __bound_args#0;
  D.29831_51 = D.25693._M_bound_args.D.25507.D.25257.D.25050._M_head_impl._M_data;
  iftmp.27_55 = D.25693._M_f.__pmf.__pfn;
  D.29837_56 = (long int) iftmp.27_55;
  D.29838_57 = D.29837_56 & 1;
  if (D.29838_57 != 0)
    goto <bb 3>;
  else
    goto <bb 4>;
Comment 5 Paolo Carlini 2010-02-15 14:02:36 UTC
What I see right now is that Inc::f is called but &i != &counter. Instead, in the first test, that at line #53 of ref2.cc, Inc::operator() is called with &i == &counter, and everything is fine.

I also tried moving out of line the member functions at lines #540 and #570 of <functional>, and also Int::f, and nothing changes at -O2, still doesn't fail, thus it's the inlining of something earlier in the call chain which makes a difference, and where we have to chase the temporary, it looks like.
Comment 6 Richard Biener 2010-02-15 14:15:11 UTC
  D.25693._M_bound_args.D.25507.D.25257.D.25050._M_head_impl._M_data = &counter;
  this.24_34 = (struct Inc *) &D.25693._M_bound_args.D.25507;
  *this.24_34 = __bound_args#0;
  D.29831_51 = D.25693._M_bound_args.D.25507.D.25257.D.25050._M_head_impl._M_data;

Thus with my patch we no longer CSE the load from _M_data with &counter but
instead load it again because the store to D.25693._M_bound_args.D.25507
aliases it.

Note that appearantly struct Inc D.29754 has zero size (and is packed to
overlap with _M_data), and we expand it like

;; D.29754 = {};

(insn 6 5 0 /abuild/rguenther/trunk-g/x86_64-unknown-linux-gnu/libstdc++-v3/include/functional:1377 (clobber (reg:QI 77 [ D.29754 ])) -1 (nil))

;; __bound_args#0 = D.29754;

(insn 7 6 0 /abuild/rguenther/trunk-g/x86_64-unknown-linux-gnu/libstdc++-v3/include/functional:1377 (set (reg/v:QI 78 [ __bound_args#0 ])
        (reg:QI 77 [ D.29754 ])) -1 (nil))


but the copy and later the indirect store transfers 1 byte of garbage.
Comment 7 Paolo Carlini 2010-02-15 14:21:49 UTC
For sure Inc doesn't have any non-static data members...
Comment 8 Richard Biener 2010-02-15 14:53:16 UTC
The issue _seems_ to be that the indirect assignment

;; *this.24_34 = __bound_args#0;

is from

;; Function std::_Head_base<_Idx, _Head, true>::_Head_base(_UHead&&) [with _UHead = Inc, long unsigned int _Idx = 0ul, _Head = Inc] (null)
;; enabled by -tree-original

{
  <<cleanup_point <<< Unknown tree: expr_stmt
  (void) (*(struct Inc *) this = *(const struct Inc &) (const struct Inc *) std::forward<Inc> ((struct type &) (struct Inc *) __h)) >>>
>>;
}

where we do not see its zero-sizeness(?) and thus end up not removing the
zero-sized assignment during cp_genericize_r.

Bah, because it's an INIT_EXPR, not a MODIFY_EXPR.

I have a patch.
Comment 9 Richard Biener 2010-02-16 10:22:30 UTC
FE issue.
Comment 10 Mark Mitchell 2010-02-17 17:51:19 UTC
IIUC, this is not a regression.
Comment 11 Paolo Carlini 2010-02-17 17:57:25 UTC
Mark, with your permission, I'm restoring P3: indeed, strictly speaking the failure of that specific testcase isn't a regression, because it didn't exist in gcc4.4.x, but I understand it uncovered a rather subtle problem in the C++ front-end. Anyway, I agree that we should figure out a plain C++ testcase, outside the libstdc++ testsuite, which fails only in 4.5, otherwise, I will understand if you release managers eventually decide to xfail the library testcase. But Jason is on it already... ;)
Comment 12 Richard Biener 2010-02-17 21:32:19 UTC
(In reply to comment #11)
> Mark, with your permission, I'm restoring P3: indeed, strictly speaking the
> failure of that specific testcase isn't a regression, because it didn't exist
> in gcc4.4.x, but I understand it uncovered a rather subtle problem in the C++
> front-end. Anyway, I agree that we should figure out a plain C++ testcase,
> outside the libstdc++ testsuite, which fails only in 4.5, otherwise, I will
> understand if you release managers eventually decide to xfail the library
> testcase. But Jason is on it already... ;)

The C++ FE issue is a regression (we removed the expr_size langhook) that
causes wrong-code.
Comment 13 Jason Merrill 2010-02-17 22:39:03 UTC
Taking.
Comment 14 Jason Merrill 2010-02-17 22:52:10 UTC
Subject: Bug 43075

Author: jason
Date: Wed Feb 17 22:51:51 2010
New Revision: 156842

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=156842
Log:
	PR c++/43075
	* call.c (build_over_call): Don't create zero-sized assignments.
	* cp-gimplify.c (cp_genericize_r): Don't remove them here.
	* cp-objcp-common.c (cp_expr_size): Remove.
	* cp-tree.h: Remove prototype.

Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/call.c
    trunk/gcc/cp/cp-gimplify.c
    trunk/gcc/cp/cp-objcp-common.c
    trunk/gcc/cp/cp-tree.h

Comment 15 Jason Merrill 2010-02-17 22:58:58 UTC
Fixed.