[Bug c++/46941] New: [trans-mem] new/delete operator are unsafe

[Bug c++/46941] New: [trans-mem] new/delete operator are unsafe

[patrick.marlier at gmail dot com]

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

           Summary: [trans-mem] new/delete operator are unsafe
           Product: gcc
           Version: trans-mem
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: patrick.marlier@gmail.com
                CC: rth@gcc.gnu.org, aldyh@gcc.gnu.org


Created attachment 22754
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=22754
testcase for new delete

The new/delete are not considered safe even if GCC managed and replaced to
transaction_safe ones.

g++ -fgnu-tm -Wall -o newdelete newdelete.cpp
newdelete.cpp:38:13: error: unsafe function call ‘void operator delete
[](void*)’ within ‘transaction_safe’ function
newdelete.cpp:32:19: error: unsafe function call ‘void* operator new [](long
unsigned int)’ within ‘transaction_safe’ function
newdelete.cpp:26:11: error: unsafe function call ‘void operator delete(void*)’
within ‘transaction_safe’ function
newdelete.cpp:20:14: error: unsafe function call ‘void* operator new(long
unsigned int)’ within ‘transaction_safe’ function

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[patrick.marlier at gmail dot com]

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

--- Comment #1 from Patrick Marlier <patrick.marlier at gmail dot com> 2010-12-22 10:11:54 UTC ---
Created attachment 22839
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=22839
proposed patch to add safe attribute to new delete operators

gcc/cp/decl.c: build_library_fn_1()
add transaction_safe attribute to new/delete/new []/delete [] declarations if
transaction mode is enabled.

Patrick Marlier.

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[aldyh at gcc dot gnu.org]

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

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2010.12.22 13:51:33
     Ever Confirmed|0                           |1

--- Comment #2 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2010-12-22 13:51:33 UTC ---
Patrick.

I haven't analyzed your patch yet, but it causes the following regressions:

FAIL: g++.dg/tm/pr45940-3.C (test for excess errors)
FAIL: g++.dg/tm/pr45940-4.C (test for excess errors)
FAIL: g++.dg/tm/pr46269.C (internal compiler error)
FAIL: g++.dg/tm/pr46269.C (test for excess errors)

Try to run the regression suite to test your patch first.  It's a good first
round of testing.  You can run it with "make check" from the toplevel build
directory.  You can compare results with and without your patch.

To run just the TM compiler tests you can do:

   make check-gcc RUNTESTFLAGS=tm.exp

I have a similar patch to yours that I'm playing with, that unfortunately also
causes the pr46269.C regression, though not the pr45940-* failures.

I am debugging and will report shortly.

Thanks.

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[patrick.marlier at gmail dot com]

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

--- Comment #3 from Patrick Marlier <patrick.marlier at gmail dot com> 2010-12-23 15:45:15 UTC ---
Actually, I was guessing that the patch was not intrusive. Wrong guess, play
again... I should really spend more time on hacking gcc ;)

Anyway, Thank you for your advices! (I will follow them next time!)

About the bug itself, I will not have time to look at it these days.

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[aldyh at gcc dot gnu.org]

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

--- Comment #4 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2010-12-23 16:19:12 UTC ---
Patrick.

You will find out that when hacking on GCC, everything is intrusive in
non-obvious ways :).

What I had in mind was something simple like this, since push_cp_library_fn()
only gets called when creating delete/new operators.

Index: cp/decl.c
===================================================================
--- cp/decl.c   (revision 168123)
+++ cp/decl.c   (working copy)
@@ -3775,6 +3775,8 @@ push_cp_library_fn (enum tree_code opera
                                 operator_code,
                                 type);
   pushdecl (fn);
+  if (flag_tm)
+    apply_tm_attr (fn, get_identifier ("transaction_pure"));
   return fn;
 }

However, this is exhibiting an unrelated bug with inlining that I am
investigating.

Thanks for all your bugreports!  They've made my life easier.

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[patrick.marlier at gmail dot com]

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

--- Comment #5 from Patrick Marlier <patrick.marlier at gmail dot com> 2010-12-23 16:27:47 UTC ---
Aldy.

I think you should declare it 'transaction_safe' and not 'transaction_pure'
since symbols in the libitm are binded to safe:
        _ZGTtnwm;
        _ZGTtnam;
        _ZGTtdlPv;
        _ZGTtdaPv;
        _ZGTtnwmRKSt9nothrow_t;
        _ZGTtnamRKSt9nothrow_t;
        _ZGTtdlPvRKSt9nothrow_t;
        _ZGTtdaPvRKSt9nothrow_t;

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[aldyh at gcc dot gnu.org]

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

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

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

--- Comment #6 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2011-01-12 19:03:53 UTC ---
Fixed
http://gcc.gnu.org/ml/gcc-patches/2011-01/msg00776.html

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[patrick.marlier at gmail dot com]

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

Patrick Marlier <patrick.marlier at gmail dot com> changed:

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

--- Comment #7 from Patrick Marlier <patrick.marlier at gmail dot com> 2011-02-02 14:17:23 UTC ---
(In reply to comment #6)
> Fixed
> http://gcc.gnu.org/ml/gcc-patches/2011-01/msg00776.html

Please considered this...
http://gcc.gnu.org/ml/gcc-patches/2011-01/msg01206.html

It must be **transaction_safe** not transaction_pure.

Moreover I think we should also add ECF_MALLOC (malloc attribute) because
otherwise the allocated memory will not be considered as local.

Extract from trans-mem.c: thread_private_new_memory()
  if (stmt && is_gimple_call (stmt) && gimple_call_flags (stmt) & ECF_MALLOC)
    /* Thread-local or transaction-local.  */
    ;
  else
    retval = mem_non_local;

Thanks.

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[aldyh at gcc dot gnu.org]

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

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |WAITING

--- Comment #8 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2011-02-03 16:24:29 UTC ---
Awaiting approval:
http://gcc.gnu.org/ml/gcc-patches/2011-02/msg00143.html

I will tackle the ECF_MALLOC comment separately.

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[aldyh at gcc dot gnu.org]

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

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

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

--- Comment #9 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2011-02-03 16:41:14 UTC ---
Resolved and committed.  Richard will take care of another unrelated 32-bit
problem which probably doesn't affect y'all.  He'll take care of that
separately.  Closing PR.

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[patrick.marlier at gmail dot com]

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

--- Comment #10 from Patrick Marlier <patrick.marlier at gmail dot com> 2011-02-04 13:54:30 UTC ---
(In reply to comment #8)
> I will tackle the ECF_MALLOC comment separately.

Should I open up a new bug report for this? or is it already on your TODO list?

Patrick Marlier.

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[aldyh at gcc dot gnu.org]

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

--- Comment #11 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2011-02-04 15:31:14 UTC ---
Patrick.  It's already on my TODO.  I'm working on it.

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[aldyh at gcc dot gnu.org]

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

--- Comment #12 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2011-02-04 18:40:09 UTC ---
Patrick, the reason memory allocated by the C++ new operator does not get
optimized by the TM-memopt pass is not because of a missing ECF_MALLOC
attribute, but because the alias oracle in GCC considers the chunk of memory as
possibly able to escape from the current function.

That is, the following hunk of code returns true in the TM-memopt pass:

      if (ptr_deref_may_alias_global_p (x))

Apparently, since new/delete operators may be overridden by class specific
hooks, the memory must be considered escaped.

So, this is not a TM bug, but a "feature" of the alias oracle in GCC.

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[patrick.marlier at gmail dot com]

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

--- Comment #13 from Patrick Marlier <patrick.marlier at gmail dot com> 2011-02-04 20:13:04 UTC ---
Hi Aldy,

On Fri, Feb 4, 2011 at 7:40 PM, aldyh at gcc dot gnu.org <
gcc-bugzilla@gcc.gnu.org> wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46941
>
> --- Comment #12 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2011-02-04
> 18:40:09 UTC ---
> Patrick, the reason memory allocated by the C++ new operator does not get
> optimized by the TM-memopt pass is not because of a missing ECF_MALLOC
> attribute, but because the alias oracle in GCC considers the chunk of
> memory as
> possibly able to escape from the current function.
>
>
I have added the "malloc" attribute to the declaration of the new operator
and it worked so I though this was it. After this I found a discussion about
adding malloc attribute to new operator...


> That is, the following hunk of code returns true in the TM-memopt pass:
>
>      if (ptr_deref_may_alias_global_p (x))
>
> Apparently, since new/delete operators may be overridden by class specific
> hooks, the memory must be considered escaped.
>

Hum I see. But if the new/delete operators are not overridden then the
returned memory could be marked as transaction local, right?
Otherwise at least a comment somewhere is welcome to be aware of this
behavior.


> So, this is not a TM bug, but a "feature" of the alias oracle in GCC.


Actually, this was not a bug but an improvement (why using costly write
barriers on transaction local memory).

As usual, thanks ;)

Patrick.

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[aldyh at gcc dot gnu.org]

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

--- Comment #14 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2011-02-04 20:16:07 UTC ---
Yes, this is an improvement, because once has to figure out why the unadultered
new operator is not handled specially by the alias oracle.  You are welcome to
file a PR as an enhancement request, but honestly I won't get to it anytime
soon, because of the other critical problems with the toolchain.

[Bug c++/46941] [trans-mem] new/delete operator are unsafe

[patrick.marlier at gmail dot com]

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

--- Comment #15 from Patrick Marlier <patrick.marlier at gmail dot com> 2011-02-04 20:30:15 UTC ---
Filled a enhancement
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47611

<http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47611>Yeah don't lose time of
this!


On Fri, Feb 4, 2011 at 9:16 PM, aldyh at gcc dot gnu.org <
gcc-bugzilla@gcc.gnu.org> wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46941
>
> --- Comment #14 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2011-02-04
> 20:16:07 UTC ---
> Yes, this is an improvement, because once has to figure out why the
> unadultered
> new operator is not handled specially by the alias oracle.  You are welcome
> to
> file a PR as an enhancement request, but honestly I won't get to it anytime
> soon, because of the other critical problems with the toolchain.
>
> --
> Configure bugmail: http://gcc.gnu.org/bugzilla/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You reported the bug.
>