The following code (definitions of mem_fun* taken from <functional>) works fine when compiled with -O1, but causes a bus error crash when compiled with -O2. -------------------- // Templates taken from <functional> template <class _Ret, class _Tp> class const_mem_fun_t { public: explicit const_mem_fun_t(_Ret (_Tp::*__pf)() const) : _M_f(__pf) {} _Ret operator()(const _Tp* __p) const { return (__p->*_M_f)(); } private: _Ret (_Tp::*_M_f)() const; }; template <class _Ret, class _Tp> class const_mem_fun_ref_t { public: explicit const_mem_fun_ref_t(_Ret (_Tp::*__pf)() const) : _M_f(__pf) {} _Ret operator()(const _Tp& __r) const { return (__r.*_M_f)(); } private: _Ret (_Tp::*_M_f)() const; }; template <class _Ret, class _Tp, class _Arg> class const_mem_fun1_t { public: explicit const_mem_fun1_t(_Ret (_Tp::*__pf)(_Arg) const) : _M_f(__pf) {} _Ret operator()(const _Tp* __p, _Arg __x) const { return (__p->*_M_f)(__x); } private: _Ret (_Tp::*_M_f)(_Arg) const; }; template <class _Ret, class _Tp, class _Arg> class const_mem_fun1_ref_t { public: explicit const_mem_fun1_ref_t(_Ret (_Tp::*__pf)(_Arg) const) : _M_f(__pf) {} _Ret operator()(const _Tp& __r, _Arg __x) const { return (__r.*_M_f)(__x); } private: _Ret (_Tp::*_M_f)(_Arg) const; }; template <class _Ret, class _Tp> inline const_mem_fun_t<_Ret, _Tp> mem_fun(_Ret (_Tp::*__f)() const) { return const_mem_fun_t<_Ret, _Tp>(__f); } template <class _Ret, class _Tp> inline const_mem_fun_ref_t<_Ret, _Tp> mem_fun_ref(_Ret (_Tp::*__f)() const) { return const_mem_fun_ref_t<_Ret, _Tp>(__f); } template <class _Ret, class _Tp, class _Arg> inline const_mem_fun1_t<_Ret, _Tp, _Arg> mem_fun(_Ret (_Tp::*__f)(_Arg) const) { return const_mem_fun1_t<_Ret, _Tp, _Arg>(__f); } template <class _Ret, class _Tp, class _Arg> inline const_mem_fun1_ref_t<_Ret, _Tp, _Arg> mem_fun_ref(_Ret (_Tp::*__f)(_Arg) const) { return const_mem_fun1_ref_t<_Ret, _Tp, _Arg>(__f); } class Class { public: void vf0c() const; void vf1c(const int&) const; }; int main() { Class obj; const Class& objc = obj; mem_fun(&Class::vf0c)(&objc); mem_fun(&Class::vf1c)(&objc, 1); mem_fun_ref(&Class::vf0c)(objc); mem_fun_ref(&Class::vf1c)(objc, 1); } void Class::vf0c() const {} void Class::vf1c(const int&) const {}
I think this has already been fixed in 4.0.2 but I have not tested it.
Confirmed with 4.0.x.
Investigating.
This cannot be right. :-) 0x00010720 <main+20>: ld [ %fp + -17 ], %g1
Really interesting: it's a combination of TARGET_PTRMEMFUNC_VBIT_LOCATION, inlining, efficient stack slot allocation and delay slot scheduling! SPARC doesn't define TARGET_PTRMEMFUNC_VBIT_LOCATION so the compiler selects ptrmemfunc_vbit_in_pfn. This means typechk.c:get_member_function_from_ptrfunc will be building an expression of the form: __pfn & 1 ? *(*(p + __delta) + __pfn - 1) : __pfn (p + __delta) Now for the minimal class "Class" in the testcase, the function is not virtual and the alignment of p is 1. Thanks to inlining, the above expression boils down to: func & 1 ? *(*p + func - 1) : func (p) Of course func is a multiple of 4 so the first branch will not be executed, although it is compiled. Then delay slot scheduling kicks in: it hoists *p in the delay slot of the ? branch because register liveness analysis allows it to do so (the register set in the insn is dead in the second branch): sethi %hi(_ZNK5Class4vf0cEv), %l5 or %l5, %lo(_ZNK5Class4vf0cEv), %l0 andcc %l0, 1, %l3 bne .LL6 ld [%fp-17], %g1 mov %l0, %g1 and the game is over. The only approach to fixing this I can think of is to modify the selection logic of TARGET_PTRMEMFUNC_VBIT_LOCATION: if the target has strict alignment and delay slots, the macro should be set to ptrmemfunc_vbit_in_delta. What do you think, Mark? Thanks in advance.
> The only approach to fixing this I can think of is to modify the selection > logic of TARGET_PTRMEMFUNC_VBIT_LOCATION: if the target has strict alignment > and delay slots, the macro should be set to ptrmemfunc_vbit_in_delta. Hum, no, I'm afraid this won't change anything. I guess the culprit is delay slot scheduling then.
Yes, I think the problem is in delay slot scheduling too. COND_EXPR means that either branch must not be evaluated because it could be illegal; if you hoist a mem from a branch into the delay slot of the condition, you are effectively partially evaluting the branch.
> Yes, I think the problem is in delay slot scheduling too. COND_EXPR means that > either branch must not be evaluated because it could be illegal; if you hoist a > mem from a branch into the delay slot of the condition, you are effectively > partially evaluting the branch. Yes, that makes sense. At the moment the predicate used in reorg.c is may_trap_p so we would need to detect faulting instructions too.
Subject: Re: [4.0 regression] mem_fun* code fine with -O1, bus error with -O2 On Oct 11, 2005, at 9:51 AM, ebotcazou at gcc dot gnu dot org wrote: > > > ------- Comment #8 from ebotcazou at gcc dot gnu dot org 2005-10-11 > 13:51 ------- >> Yes, I think the problem is in delay slot scheduling too. COND_EXPR >> means that >> either branch must not be evaluated because it could be illegal; if >> you hoist a >> mem from a branch into the delay slot of the condition, you are >> effectively >> partially evaluting the branch. > > Yes, that makes sense. At the moment the predicate used in reorg.c is > may_trap_p so we would need to detect faulting instructions too. may_trap_p is the correct thing as it should detect this instruction as trapping: /* Memory ref can trap unless it's a static var or a stack slot. */ case MEM: if (MEM_NOTRAP_P (x)) return 0; return rtx_addr_can_trap_p (XEXP (x, 0)); -- Pinski
> may_trap_p is the correct thing as it should detect this instruction as > trapping: > > /* Memory ref can trap unless it's a static var or a stack slot. */ > case MEM: > if (MEM_NOTRAP_P (x)) > return 0; > return rtx_addr_can_trap_p (XEXP (x, 0)); No, it's the other way around. The MEM is a stack slot so may_trap_p returns 0.
Subject: Re: [4.0 regression] mem_fun* code fine with -O1, bus error with -O2 ebotcazou at gcc dot gnu dot org wrote: > ------- Comment #10 from ebotcazou at gcc dot gnu dot org 2005-10-11 13:59 ------- > >>may_trap_p is the correct thing as it should detect this instruction as >>trapping: >> >> /* Memory ref can trap unless it's a static var or a stack slot. */ >> case MEM: >> if (MEM_NOTRAP_P (x)) >> return 0; >> return rtx_addr_can_trap_p (XEXP (x, 0)); > > > No, it's the other way around. The MEM is a stack slot so may_trap_p returns > 0. This certainly is a bug in the back-end, not a bug in the default location of the v-bit. You shouldn't need to break the C++ ABI on SPARC to fix this bug. However, I'm not sure why you're seeing a 4-byte load from an unaligned address. One possibility is that in rtx_addr_can_trap_p: case PLUS: /* An address is assumed not to trap if it is an address that can't trap plus a constant integer or it is the pic register plus a constant. */ return ! ((! rtx_addr_can_trap_p (XEXP (x, 0)) && GET_CODE (XEXP (x, 1)) == CONST_INT) || (XEXP (x, 0) == pic_offset_table_rtx && CONSTANT_P (XEXP (x, 1)))); we should consider the address as unsafe if the CONST_INT is not a multiple of the size of the mode, on a STRICT_ALIGNMENT target.
> This certainly is a bug in the back-end, not a bug in the default > location of the v-bit. You shouldn't need to break the C++ ABI on SPARC > to fix this bug. Right, I was confused, I thought __pfn was dereferenced itself. > However, I'm not sure why you're seeing a 4-byte load from an unaligned > address. Because p is a pointer to Class and Class has alignment 1. I guess the first branch of the expression works when Class contains a pointer to the vtable, hence has alignment 4. > One possibility is that in rtx_addr_can_trap_p: > > case PLUS: > /* An address is assumed not to trap if it is an address that > can't > trap plus a constant integer or it is the pic register plus a > > constant. */ > return ! ((! rtx_addr_can_trap_p (XEXP (x, 0)) > && GET_CODE (XEXP (x, 1)) == CONST_INT) > || (XEXP (x, 0) == pic_offset_table_rtx > && CONSTANT_P (XEXP (x, 1)))); > > we should consider the address as unsafe if the CONST_INT is not a > multiple of the size of the mode, on a STRICT_ALIGNMENT target. Yes, but that's not enough, as MEM_NO_TRAP_P might come into play.
Subject: Re: [4.0 regression] mem_fun* code fine with -O1, bus error with -O2 ebotcazou at gcc dot gnu dot org wrote: > ------- Comment #12 from ebotcazou at gcc dot gnu dot org 2005-10-11 14:41 ------- > >>This certainly is a bug in the back-end, not a bug in the default >>location of the v-bit. You shouldn't need to break the C++ ABI on SPARC >>to fix this bug. > > > Right, I was confused, I thought __pfn was dereferenced itself. > > >>However, I'm not sure why you're seeing a 4-byte load from an unaligned >>address. > > > Because p is a pointer to Class and Class has alignment 1. I guess the first > branch of the expression works when Class contains a pointer to the vtable, > hence has alignment 4. Ah. I think that would best be fixed in the front-end, then. If the class doesn't have a virtual pointer, then there's no need to generate the conditional expression; avoiding that will not only fix this bug, but make the code generated by the front-end easier for the middle end to process. Please re-assign to me.
> Ah. I think that would best be fixed in the front-end, then. If the > class doesn't have a virtual pointer, then there's no need to generate > the conditional expression; avoiding that will not only fix this bug, > but make the code generated by the front-end easier for the middle end > to process. Clever. :-) > Please re-assign to me. Thanks.
Probably. But what if the problem with dereferencing p was that it is NULL, instead of a misalignment? Would that case be caught in reorg by something else?
Subject: Re: [4.0 regression] mem_fun* code fine with -O1, bus error with -O2 giovannibajo at libero dot it wrote: > ------- Comment #15 from giovannibajo at libero dot it 2005-10-11 17:16 ------- > Probably. But what if the problem with dereferencing p was that it is NULL, > instead of a misalignment? Would that case be caught in reorg by something > else? Well, then the code would have undefined behavior, and the bus error would be OK.
Eric -- Sorry -- my idea about fixing this in the front end is bogus. It's OK to dereference a pointer-to-member to a virtual function member even if the base class doesn't have any virtual functions; c.f., g++.old-deja/g++.martin/pmf1.C. So, I've unassigned myself; you'll have to look for a solution in the back end. I don't understand your Comment #12 regarding MEM_NO_TRAP_P; MEM_NO_TRAP_P calls rtx_addr_can_trap_p. I'd suspect that making rtx_addr_can_trap_p respect STRICT_ALIGNMENT (by checking the values of the CONST_INT added to the non-trapping address) would work.
> Sorry -- my idea about fixing this in the front end is bogus. It's OK to > dereference a pointer-to-member to a virtual function member even if the base > class doesn't have any virtual functions; Too bad. :-( > I don't understand your Comment #12 regarding MEM_NO_TRAP_P; MEM_NO_TRAP_P > calls rtx_addr_can_trap_p. Not quite, MEM_NOTRAP_P is a RTL flag that may be put on a MEM to assert that the memory reference doesn't trap; in particular, you're allowed to put it on any stack slot reference as far as I understand. > I'd suspect that making rtx_addr_can_trap_p respect STRICT_ALIGNMENT (by > checking the values of the CONST_INT added to the non-trapping address) would > work. Modulo the MEM_NOTRAP_P thing above, I agree. But I'm not sure enhancing the existing may_trap_p is the way to go, because as far as I can tell you want to optimize away this faulting memory ref if you can, and may_trap_p MEMs cannot be optimized away with -fnon-call-exceptions for example. So I'd lean towards adding a may_fault_p predicate that supercedes may_trap_p and using it in the reorg pass.
(In reply to comment #16) > > Probably. But what if the problem with dereferencing p was that it is NULL, > > instead of a misalignment? Would that case be caught in reorg by something > > else? > Well, then the code would have undefined behavior, and the bus error > would be OK. Uh? I'm speaking of a NULL dereference on the branch of COND_EXPR which will not get executed. The point of this PR (as far as I understand it) is that an instruction of a branch which should not be evaluated (and thus potentially dangerous) is hoisted into the delay slot of the condition. Fixing the frontend so to not emit the unaligned load (which should not get executed anyway) is just papering over the bug in reorg which causes it to be (partially) executed.
Mark, do you have an opinion on the following implementation detail? We don't want to duplicate the code of may_trap_p and rtx_addr_can_trap_p, so the new predicate will essentially piggyback on it, simply bypassing MEM_NOTRAP_P and adding the supplementary check on the alignment on STRICT_ALIGNMENT machines. Now this new predicate will only be invoked (for now) from reorg.c and only do something new on STRICT_ALIGNMENT machines, so I'm not really thrilled to propagate an additional argument all the way through the recursion in may_trap_p; that's why I was pondering adding a global flag instead, possibly forced to "const bool false" if not on a STRICT_ALIGNMENT machine. Thanks in advance.
Subject: Bug 23585 CVSROOT: /cvs/gcc Module name: gcc Changes by: ebotcazou@gcc.gnu.org 2005-10-20 12:14:30 Modified files: gcc : ChangeLog function.c reorg.c rtl.h rtlanal.c gcc/config/sparc: sparc.h gcc/testsuite : ChangeLog Added files: gcc/testsuite/g++.dg/opt: delay-slot-1.C Log message: PR rtl-optimization/23585 * rtlanal.c (rtx_addr_can_trap_p_1): New predicate extracted from... (rtx_addr_can_trap_p): ... here. Invoke rtx_addr_can_trap_p_1. (may_trap_p_1): New predicate extracted from... (may_trap_p): ... here. Invoke may_trap_p_1. (may_trap_or_fault_p): New predicate. * rtl.h (may_trap_or_fault_p): Declare it. * reorg.c (steal_delay_list_from_target): Use may_trap_or_fault_p instead of may_trap_p. (steal_delay_list_from_fallthrough): Likewise. (fill_simple_delay_slots): Likewise. (fill_slots_from_thread): Likewise. * function.c (pad_to_arg_alignment): Rework comment about SPARC_STACK_BOUNDARY_HACK. * config/sparc/sparc.h: Likewise. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.10191&r2=2.10192 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/function.c.diff?cvsroot=gcc&r1=1.646&r2=1.647 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/reorg.c.diff?cvsroot=gcc&r1=1.111&r2=1.112 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/rtl.h.diff?cvsroot=gcc&r1=1.560&r2=1.561 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/rtlanal.c.diff?cvsroot=gcc&r1=1.220&r2=1.221 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/sparc/sparc.h.diff?cvsroot=gcc&r1=1.290&r2=1.291 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.6216&r2=1.6217 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/opt/delay-slot-1.C.diff?cvsroot=gcc&r1=NONE&r2=1.1
Subject: Bug 23585 CVSROOT: /cvs/gcc Module name: gcc Branch: gcc-4_0-branch Changes by: ebotcazou@gcc.gnu.org 2005-10-20 12:18:05 Modified files: gcc : ChangeLog function.c reorg.c rtl.h rtlanal.c gcc/config/sparc: sparc.h gcc/testsuite : ChangeLog Added files: gcc/testsuite/g++.dg/opt: delay-slot-1.C Log message: PR rtl-optimization/23585 * rtlanal.c (rtx_addr_can_trap_p_1): New predicate extracted from... (rtx_addr_can_trap_p): ... here. Invoke rtx_addr_can_trap_p_1. (may_trap_p_1): New predicate extracted from... (may_trap_p): ... here. Invoke may_trap_p_1. (may_trap_or_fault_p): New predicate. * rtl.h (may_trap_or_fault_p): Declare it. * reorg.c (steal_delay_list_from_target): Use may_trap_or_fault_p instead of may_trap_p. (steal_delay_list_from_fallthrough): Likewise. (fill_simple_delay_slots): Likewise. (fill_slots_from_thread): Likewise. * function.c (pad_to_arg_alignment): Rework comment about SPARC_STACK_BOUNDARY_HACK. * config/sparc/sparc.h: Likewise. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.7592.2.473&r2=2.7592.2.474 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/function.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.604.8.1&r2=1.604.8.2 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/reorg.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.104.8.1&r2=1.104.8.2 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/rtl.h.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.536.4.1&r2=1.536.4.2 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/rtlanal.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.211.8.2&r2=1.211.8.3 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/sparc/sparc.h.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.273.8.3&r2=1.273.8.4 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.5084.2.474&r2=1.5084.2.475 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/opt/delay-slot-1.C.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.2.1
See http://gcc.gnu.org/ml/gcc-patches/2005-10/msg00883.html Thanks for the reduced testcase.
Subject: Bug 23585 Author: ebotcazou Date: Thu Nov 3 11:31:46 2005 New Revision: 106427 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=106427 Log: PR rtl-optimization/23585 * rtlanal.c (rtx_addr_can_trap_p_1) <PLUS>: Return 0 for an address that can't trap plus a constant integer, if the mode has zero size. Added: trunk/gcc/testsuite/gcc.dg/delay-slot-1.c Modified: trunk/gcc/ChangeLog trunk/gcc/rtlanal.c trunk/gcc/testsuite/ChangeLog
Subject: Bug 23585 Author: ebotcazou Date: Thu Nov 3 11:34:55 2005 New Revision: 106428 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=106428 Log: PR rtl-optimization/23585 * rtlanal.c (rtx_addr_can_trap_p_1) <PLUS>: Return 0 for an address that can't trap plus a constant integer, if the mode has zero size. Added: branches/gcc-4_0-branch/gcc/testsuite/gcc.dg/delay-slot-1.c Modified: branches/gcc-4_0-branch/gcc/ChangeLog branches/gcc-4_0-branch/gcc/rtlanal.c branches/gcc-4_0-branch/gcc/testsuite/ChangeLog
*** Bug 23954 has been marked as a duplicate of this bug. ***