| Summary: | [6/7 Regression] wrong code on arm-linux-gnueabi and arm-linux-gnueabihf | ||
|---|---|---|---|
| Product: | gcc | Reporter: | Matthias Klose <doko> |
| Component: | libstdc++ | Assignee: | Not yet assigned to anyone <unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | jwakely.gcc, ramana, rguenth, webrown.cpp |
| Priority: | P3 | Keywords: | wrong-code |
| Version: | 6.2.1 | ||
| Target Milestone: | 6.3 | ||
| Host: | Target: | arm-linux-gnueabihf, arm-linux-gnueabi | |
| Build: | Known to work: | 5.4.1 | |
| Known to fail: | 6.2.1, 7.0 | Last reconfirmed: | 2016-09-22 00:00:00 |
| Attachments: | Preprocessed source | ||
|
Description
Matthias Klose
2016-09-22 09:06:26 UTC
Confirmed on 6 and current trunk (and that it's working on 5) A slightly adjusted testcase is:
#include <functional>
#include <iostream>
struct C
{
void
doCb ()
{
size_t dummy_a = 1;
std::cout << "";
auto tmp = this;
std::function<void()> f;
f = [this, &dummy_a]() {};
f = [this, tmp]() {
if (this != tmp)
__builtin_abort ();
};
f ();
}
};
int
main ()
{
C c;
c.doCb ();
}
This aborts on arm at -O2 but not at -O1
The differences between r234749 and the one before it start in 032t.esra: --- good/wrong.cpp.032t.esra 2016-09-22 17:41:24.914598329 +0100 +++ bad/wrong.cpp.032t.esra 2016-09-22 17:40:33.134513348 +0100 @@ -1357,8 +1357,8 @@ Updating SSA information for statement MEM[(struct &)&D.43856] ={v} {CLOBBER}; Updating SSA information for statement MEM[(struct &)&D.43856] ={v} {CLOBBER}; Updating SSA information for statement MEM[(struct _Function_base *)&D.43856]._M_manager = 0B; -Updating SSA information for statement MEM[(struct function *)&D.43856] = __f$__this; -Updating SSA information for statement MEM[(struct function *)&D.43856 + 4B] = __f$__dummy_a; +Updating SSA information for statement MEM[(struct __lambda0 *)&D.43856] = __f$__this; +Updating SSA information for statement MEM[(struct __lambda0 *)&D.43856 + 4B] = __f$__dummy_a; Updating SSA information for statement D.43856._M_invoker = _M_invoke; Updating SSA information for statement D.43856.D.42018._M_manager = _M_manager; Updating SSA information for statement __tmp = MEM[(union _Any_data &)&D.43856]; @@ -1449,9 +1449,9 @@ # .MEM_23 = VDEF <.MEM_22> MEM[(struct _Function_baseD.30840 *)&D.43856]._M_managerD.30941 = 0B; # .MEM_4 = VDEF <.MEM_23> - MEM[(struct functionD.41835 *)&D.43856] = __f$__this_17; + MEM[(struct __lambda0D.42048 *)&D.43856] = __f$__this_17; # .MEM_16 = VDEF <.MEM_4> - MEM[(struct functionD.41835 *)&D.43856 + 4B] = __f$__dummy_a_19; + MEM[(struct __lambda0D.42048 *)&D.43856 + 4B] = __f$__dummy_a_19; # .MEM_25 = VDEF <.MEM_16> D.43856._M_invokerD.41987 = _ZNSt17_Function_handlerIFvvEZN1C4doCbEvEUlvE_E9_M_invokeERKSt9_Any_dataD.44149; # .MEM_26 = VDEF <.MEM_25> @@ -1791,8 +1791,8 @@ Updating SSA information for statement MEM[(struct &)&D.43888] ={v} {CLOBBER}; Updating SSA information for statement MEM[(struct &)&D.43888] ={v} {CLOBBER}; Updating SSA information for statement MEM[(struct _Function_base *)&D.43888]._M_manager = 0B; -Updating SSA information for statement MEM[(struct function *)&D.43888] = __f$__this; -Updating SSA information for statement MEM[(struct function *)&D.43888 + 4B] = __f$__tmp; +Updating SSA information for statement MEM[(struct __lambda1 *)&D.43888] = __f$__this; +Updating SSA information for statement MEM[(struct __lambda1 *)&D.43888 + 4B] = __f$__tmp; Updating SSA information for statement D.43888._M_invoker = _M_invoke; Updating SSA information for statement D.43888.D.42018._M_manager = _M_manager; Updating SSA information for statement __tmp = MEM[(union _Any_data &)&D.43888]; @@ -1883,9 +1883,9 @@ # .MEM_23 = VDEF <.MEM_22> MEM[(struct _Function_baseD.30840 *)&D.43888]._M_managerD.30941 = 0B; # .MEM_4 = VDEF <.MEM_23> - MEM[(struct functionD.41835 *)&D.43888] = __f$__this_17; + MEM[(struct __lambda1D.42234 *)&D.43888] = __f$__this_17; # .MEM_16 = VDEF <.MEM_4> - MEM[(struct functionD.41835 *)&D.43888 + 4B] = __f$__tmp_19; + MEM[(struct __lambda1D.42234 *)&D.43888 + 4B] = __f$__tmp_19; # .MEM_25 = VDEF <.MEM_16> D.43888._M_invokerD.41987 = _ZNSt17_Function_handlerIFvvEZN1C4doCbEvEUlvE0_E9_M_invokeERKSt9_Any_dataD.45281; # .MEM_26 = VDEF <.MEM_25> This can only uncover another pre-existing issue as it is a 100% obvious fix. I do not have a good way to run anything on arm so please help me spot the wrong-code in the assembler output. TBAA issues are also often exposed by scheduling so please try -fno-schedule-insns[2] or -fstrict-aliasing with -O1. -Updating SSA information for statement MEM[(struct function *)&D.43856 + 4B] = __f$__dummy_a; +Updating SSA information for statement MEM[(struct __lambda0 *)&D.43856] = this might also very well be a C++ FE issue. (In reply to Richard Biener from comment #5) > This can only uncover another pre-existing issue as it is a 100% obvious > fix. I do not have a good way to run anything on arm so please help me spot > the wrong-code in the assembler output. > > TBAA issues are also often exposed by scheduling so please try > -fno-schedule-insns[2] or -fstrict-aliasing with -O1. > You're right. -fno-schedule-insns2 makes the testcase pass at -O2 > -Updating SSA information for statement MEM[(struct function *)&D.43856 + > 4B] = __f$__dummy_a; > +Updating SSA information for statement MEM[(struct __lambda0 *)&D.43856] = > > this might also very well be a C++ FE issue. As snippet of the assembly output without scheduling (-fno-schedule-insns2):
.LEHE0:
add r3, sp, #32
str r5, [sp, #32] <-------- I1
add ip, sp, #8
str r5, [sp, #36] <-------- I2
add r4, sp, #16
ldm r3, {r0, r1} <---------I3
add lr, sp, #4
str r5, [sp, #16]
mov r2, #3
str lr, [sp, #20]
movw r5, #:lower16:<sym>
stm ip, {r0, r1}
movt r5, #:upper16:<sym>
ldm r4, {r0, r1}
movw lr, #:lower16:<sym>
str r5, [sp, #40]
<snip>
The same region with scheduling:
.LEHE0:
add r3, sp, #32
add ip, sp, #8
add r4, sp, #16
ldm r3, {r0, r1} <---------- I3
add lr, sp, #4
str r5, [sp, #16]
mov r2, #3
str lr, [sp, #20]
movw lr, #:lower16:<sym>
stm ip, {r0, r1}
movt lr, #:upper16:<sym>
ldm r4, {r0, r1}
str r5, [sp, #32] <---------- I1
str r5, [sp, #36] <---------- I2
The stores I1 and I2 were moved past the load I3 that loads from SP + 32
If I manually move those two stores back before I3 the program doesn't abort
(In reply to ktkachov from comment #7) > As snippet of the assembly output without scheduling (-fno-schedule-insns2): > .LEHE0: > add r3, sp, #32 > str r5, [sp, #32] <-------- I1 > add ip, sp, #8 > str r5, [sp, #36] <-------- I2 > add r4, sp, #16 > ldm r3, {r0, r1} <---------I3 > add lr, sp, #4 > str r5, [sp, #16] > mov r2, #3 > str lr, [sp, #20] > movw r5, #:lower16:<sym> > stm ip, {r0, r1} > movt r5, #:upper16:<sym> > ldm r4, {r0, r1} > movw lr, #:lower16:<sym> > str r5, [sp, #40] > <snip> > > The same region with scheduling: > .LEHE0: > add r3, sp, #32 > add ip, sp, #8 > add r4, sp, #16 > ldm r3, {r0, r1} <---------- I3 > add lr, sp, #4 > str r5, [sp, #16] > mov r2, #3 > str lr, [sp, #20] > movw lr, #:lower16:<sym> > stm ip, {r0, r1} > movt lr, #:upper16:<sym> > ldm r4, {r0, r1} > str r5, [sp, #32] <---------- I1 > str r5, [sp, #36] <---------- I2 > > The stores I1 and I2 were moved past the load I3 that loads from SP + 32 > If I manually move those two stores back before I3 the program doesn't abort which function is the above in? Also can you please attach preprocessed source? Created attachment 39688 [details]
Preprocessed source
Attaching the preprocessed source.
The function in question is:
_ZN1C4doCbEv
This is compiled with -std=gnu++14 -O2 -march=armv7-a -mfpu=vfpv3-d16 -mfloat-abi=hard
So it's
@(insn:TI 21 24 15 (parallel [
@ (set (reg:SI 0 r0)
@ (mem/c:SI (reg/f:SI 3 r3 [120]) [23 MEM[(union _Any_data &)&D.50945]+0 S4 A64]))
@ (set (reg:SI 1 r1)
@ (mem/c:SI (plus:SI (reg/f:SI 3 r3 [120])
@ (const_int 4 [0x4])) [23 MEM[(union _Any_data &)&D.50945]+4 S4 A32]))
@ ]) t.ii:1926 383 {*ldm2_}
@ (nil))
ldm r3, {r0, r1} @ 21 *ldm2_ [length = 4]
vs.
@(insn:TI 17 25 18 (set (mem/f/c:SI (plus:SI (reg/f:SI 13 sp)
@ (const_int 32 [0x20])) [30 MEM[(struct __lambda1 *)&D.50945]+0 S4 A64])
@ (reg/f:SI 5 r5 [orig:114 this ] [114])) 630 {*arm_movsi_vfp}
@ (nil))
str r5, [sp, #32] @ 17 *arm_movsi_vfp/6 [length = 4]
@(insn:TI 18 17 118 (set (mem/f/c:SI (plus:SI (reg/f:SI 13 sp)
@ (const_int 36 [0x24])) [30 MEM[(struct __lambda1 *)&D.50945 + 4B]+0 S4 A32])
@ (reg/f:SI 5 r5 [orig:114 this ] [114])) 630 {*arm_movsi_vfp}
@ (nil))
str r5, [sp, #36] @ 18 *arm_movsi_vfp/6 [length = 4]
and obviously r3 == sp + 32 and thus this is a must-alias. But the alias sets
are 30 vs. 23 here.
At RTL expansion time:
;; MEM[(struct __lambda1 *)&D.50945] = this_4(D);
(insn 17 16 0 (set (mem/f/c:SI (plus:SI (reg/f:SI 105 virtual-stack-vars)
(const_int -16 [0xfffffffffffffff0])) [30 MEM[(struct __lambda1 *)&D.50945]+0 S4 A64])
(reg/f:SI 114 [ this ])) -1
(nil))
;; MEM[(struct __lambda1 *)&D.50945 + 4B] = this_4(D);
(insn 18 17 0 (set (mem/f/c:SI (plus:SI (reg/f:SI 105 virtual-stack-vars)
(const_int -12 [0xfffffffffffffff4])) [30 MEM[(struct __lambda1 *)&D.50945 + 4B]+0 S4 A32])
(reg/f:SI 114 [ this ])) -1
(nil))
...
;; __tmp = MEM[(union _Any_data &)&D.50945];
(insn 19 18 20 (set (reg:SI 119)
(plus:SI (reg/f:SI 105 virtual-stack-vars)
(const_int -40 [0xffffffffffffffd8]))) t.ii:1926 -1
(nil))
(insn 20 19 21 (set (reg:SI 120)
(plus:SI (reg/f:SI 105 virtual-stack-vars)
(const_int -16 [0xfffffffffffffff0]))) t.ii:1926 -1
(nil))
(insn 21 20 22 (parallel [
(set (reg:SI 0 r0)
(mem/c:SI (reg:SI 120) [23 MEM[(union _Any_data &)&D.50945]+0 S4 A64]))
(set (reg:SI 1 r1)
(mem/c:SI (plus:SI (reg:SI 120)
(const_int 4 [0x4])) [23 MEM[(union _Any_data &)&D.50945]+4 S4 A32]))
]) t.ii:1926 -1
(nil))
(insn 22 21 0 (parallel [
(set (mem/c:SI (reg:SI 119) [23 __tmp+0 S4 A64])
(reg:SI 0 r0))
(set (mem/c:SI (plus:SI (reg:SI 119)
(const_int 4 [0x4])) [23 __tmp+4 S4 A32])
(reg:SI 1 r1))
]) t.ii:1926 -1
(nil))
From GIMPLE with more context:
;; basic block 2, loop depth 0
;; pred: ENTRY
dummy_a = 1;
std::__ostream_insert<char, std::char_traits<char> > (&cout, "", 0);
MEM[(struct &)&f] ={v} {CLOBBER};
MEM[(struct &)&f] ={v} {CLOBBER};
MEM[(union _Any_data *)&f] = this_4(D);
MEM[(union _Any_data *)&f + 4B] = &dummy_a;
MEM[(struct &)&D.50945] ={v} {CLOBBER};
MEM[(struct &)&D.50945] ={v} {CLOBBER};
MEM[(struct __lambda1 *)&D.50945] = this_4(D); <---
MEM[(struct __lambda1 *)&D.50945 + 4B] = this_4(D); <---
__tmp = MEM[(union _Any_data &)&D.50945]; <---
MEM[(union _Any_data *)&D.50945] = MEM[(union _Any_data &)&f];
MEM[(union _Any_data *)&f] = __tmp;
__tmp ={v} {CLOBBER};
...
not very well optimized either, the copy to __tmp could be elided.
What SRA does is obviously correct now (and incorrect before):
- MEM[(struct __lambda0 *)&D.47785] = __f;
+ MEM[(struct __lambda0 *)&D.47785] = __f$__this_17;
+ MEM[(struct __lambda0 *)&D.47785 + 4B] = __f$__dummy_a_19;
...
- MEM[(struct __lambda1 *)&D.47815] = __f;
+ MEM[(struct __lambda1 *)&D.47815] = __f$__this_17;
+ MEM[(struct __lambda1 *)&D.47815 + 4B] = __f$__tmp_19;
so you see it now preserves the alias sets for the stores.
That _Any_data identifier makes me suspicious of the testcase invoking undefined behavior:
union _Any_data
{
void* _M_access() { return &_M_pod_data[0]; }
const void* _M_access() const { return &_M_pod_data[0]; }
template<typename _Tp>
_Tp&
_M_access()
{ return *static_cast<_Tp*>(_M_access()); }
template<typename _Tp>
const _Tp&
_M_access() const
{ return *static_cast<const _Tp*>(_M_access()); }
_Nocopy_types _M_unused;
char _M_pod_data[sizeof(_Nocopy_types)];
};
it seems to fall foul of the common misconception that you can do aggregate
copies of type _Any_data and that this will properly transfer objects
constructed into _Any_data::_M_pod_data. That is obviously not the case.
-> latent libstd++ issue unless proved otherwise.
;; Function void std::function<_Res(_ArgTypes ...)>::swap(std::function<_Res(_ArgTypes ...)>&) [with _Res = void; _ArgTypes = {}] (null)
;; enabled by -tree-original
<<< Unknown tree: must_not_throw_expr
<<cleanup_point <<< Unknown tree: expr_stmt
std::swap<std::_Any_data> ((union _Any_data &) &((struct function *) this)->D.45189._M_functor, (union _Any_data &) &((struct function *) __x)->D.45189._M_functor) >>>>>;
^^^^
doing std::swap on _Any_data. That's of course bogus.
/**
* @brief Swap the targets of two %function objects.
* @param __x A %function with identical call signature.
*
* Swap the targets of @c this function object and @a __f. This
* function will not throw an %exception.
*/
void swap(function& __x) noexcept
{
std::swap(_M_functor, __x._M_functor);
std::swap(_M_manager, __x._M_manager);
std::swap(_M_invoker, __x._M_invoker);
}
and swap() seems to be used in multiple places throughout functional.
Index: libstdc++-v3/include/std/functional
===================================================================
--- libstdc++-v3/include/std/functional (revision 240521)
+++ libstdc++-v3/include/std/functional (working copy)
@@ -1418,7 +1418,7 @@ _GLIBCXX_MEM_FN_TRAITS(&&, false_type, t
_Nocopy_types _M_unused;
char _M_pod_data[sizeof(_Nocopy_types)];
- };
+ } __attribute__((may_alias));
enum _Manager_operation
{
fixes this testcase. As std::swap takes reference args I suppose it should
reliably work to prevent the issue.
(In reply to Richard Biener from comment #12) > Index: libstdc++-v3/include/std/functional > =================================================================== > --- libstdc++-v3/include/std/functional (revision 240521) > +++ libstdc++-v3/include/std/functional (working copy) > @@ -1418,7 +1418,7 @@ _GLIBCXX_MEM_FN_TRAITS(&&, false_type, t > > _Nocopy_types _M_unused; > char _M_pod_data[sizeof(_Nocopy_types)]; > - }; > + } __attribute__((may_alias)); > > enum _Manager_operation > { > > fixes this testcase. As std::swap takes reference args I suppose it should > reliably work to prevent the issue. I think this is the right fix. IIUC this is a valid use of may_alias, as the whole point of _Any_data is that it can alias an unbounded set of types. Just to explain a little bit. Consider
void foo (void)
{
union { float f; char c[4]; } u, v;
*(int *)&u = 0;
v = u;
return *(int *)&v;
}
then C11 6.5/6,7 make it clear that it is not valid to use 'u' (of union type)
to access the object which now has effective type 'int' (in fact that store
alone, given strict reading of C11 6.5/6 is invalid as u has a declared type).
The union does not have 'int' amongst its members. Putting 'char' or an
array of 'char' into the union does not make the access fall under 6.5/7
as 'a character type' only follows 'an aggregate or union type that includes one
of the _AFOREMENTIONED_ types...' (emphasis mine).
GCC allows even objects with a declared type to take any effective type by
means of storing to it with that type (to support existing practice and
multiple languages).
Btw, what I fail to find is sth in the (C) standard that specifies the semantics of an aggregate assignment of union type. Thus I interpret it to use the effective type of the object (determined by the last access) and thus only transfer the active object (so a union aggregate copy is _not_ a bytewise copy of size sizeof (union type)). For C++ 12.8 [class.copy] says: -16- The implicitly-defined copy/move constructor for a union X copies the object representation (3.9) of X. and -29- The implicitly-defined copy assignment operator for a union X copies the object representation (3.9) of X. so it is a bytewise copy for PODs. (In reply to Richard Biener from comment #14) > Just to explain a little bit. Consider > > void foo (void) > { > union { float f; char c[4]; } u, v; > *(int *)&u = 0; > v = u; > return *(int *)&v; > } > > then C11 6.5/6,7 make it clear that it is not valid to use 'u' (of union > type) > to access the object which now has effective type 'int' (in fact that store > alone, given strict reading of C11 6.5/6 is invalid as u has a declared > type). > The union does not have 'int' amongst its members. Putting 'char' or an > array of 'char' into the union does not make the access fall under 6.5/7 > as 'a character type' only follows 'an aggregate or union type that includes > one > of the _AFOREMENTIONED_ types...' (emphasis mine). But would *(int*)u.c be OK, because that is a character type? Given that C++ does define what the assignment means for the union, we just need to make sure we do *(int*)u.c not *(int*)&u or is that still not enough? If not, what does the last bullet of 6.5/7 allow? On Tue, 27 Sep 2016, redi at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77686 > > --- Comment #17 from Jonathan Wakely <redi at gcc dot gnu.org> --- > (In reply to Richard Biener from comment #14) > > Just to explain a little bit. Consider > > > > void foo (void) > > { > > union { float f; char c[4]; } u, v; > > *(int *)&u = 0; > > v = u; > > return *(int *)&v; > > } > > > > then C11 6.5/6,7 make it clear that it is not valid to use 'u' (of union > > type) > > to access the object which now has effective type 'int' (in fact that store > > alone, given strict reading of C11 6.5/6 is invalid as u has a declared > > type). > > The union does not have 'int' amongst its members. Putting 'char' or an > > array of 'char' into the union does not make the access fall under 6.5/7 > > as 'a character type' only follows 'an aggregate or union type that includes > > one > > of the _AFOREMENTIONED_ types...' (emphasis mine). > > But would *(int*)u.c be OK, because that is a character type? Given that C++ > does define what the assignment means for the union, we just need to make sure > we do *(int*)u.c not *(int*)&u or is that still not enough? No, *(int *)u.c is not OK, because that's still 'int', not a character type (because of the cast). > If not, what does the last bullet of 6.5/7 allow? It allows *(char *)&u basically it allows a memcpy implementation to exist. Author: redi Date: Wed Sep 28 10:57:46 2016 New Revision: 240567 URL: https://gcc.gnu.org/viewcvs?rev=240567&root=gcc&view=rev Log: libstdc++/77686 use may_alias for std::function storage PR libstdc++/77686 * include/std/functional (_Any_data): Add may_alias attribute. Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/std/functional Author: redi Date: Wed Sep 28 12:03:43 2016 New Revision: 240571 URL: https://gcc.gnu.org/viewcvs?rev=240571&root=gcc&view=rev Log: libstdc++/77686 use may_alias for std::function storage PR libstdc++/77686 * include/std/functional (_Any_data): Add may_alias attribute. Modified: branches/gcc-6-branch/libstdc++-v3/ChangeLog branches/gcc-6-branch/libstdc++-v3/include/std/functional Fixed. On Wed, 28 Sep 2016, redi at gcc dot gnu.org wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77686
>
> Jonathan Wakely <redi at gcc dot gnu.org> changed:
>
> What |Removed |Added
> ----------------------------------------------------------------------------
> Status|NEW |RESOLVED
> Resolution|--- |FIXED
>
> --- Comment #21 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> Fixed.
The issue is latent on the GCC 5 branch as well I guess.
Author: redi Date: Wed Sep 28 12:47:24 2016 New Revision: 240574 URL: https://gcc.gnu.org/viewcvs?rev=240574&root=gcc&view=rev Log: libstdc++/77686 use may_alias for std::function storage PR libstdc++/77686 * include/std/functional (_Any_data): Add may_alias attribute. Modified: branches/gcc-5-branch/libstdc++-v3/ChangeLog branches/gcc-5-branch/libstdc++-v3/include/std/functional |