GCC Bugzilla has been upgraded from version 4.4.9 to 5.0rc3. If you see any problem, please report it to bug 64968.
Bug 50594 - Option -fwhole-program discards replaced new operator for std::string
Summary: Option -fwhole-program discards replaced new operator for std::string
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.6.1
: P3 normal
Target Milestone: 4.7.0
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-02 22:53 UTC by Kerrek SB
Modified: 2012-03-12 16:49 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-10-04 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kerrek SB 2011-10-02 22:53:42 UTC
The following program fails to output the messages in the replaced allocation operators, but *only* during the string allocation *and* when the option `-fwhole-program` is present. (Tested on 4.6.1 and 4.4.3.) That is, the program behaves differently when compiles with the following two commands:

  g++ -std=c++0x -o prog prog.cpp
  g++ -std=c++0x -o prog prog.cpp -fwhole-program

Note that the allocation used by the "map" is always done correctly.

Program code:

#include <new>
#include <string>
#include <iostream>
#include <cstdlib>
#include <map>

void * operator new(std::size_t n) throw(std::bad_alloc)
{
  void * const p = std::malloc(n);

  if (p == NULL) throw std::bad_alloc();

  std::cerr << "new() requests " << n << " bytes, allocated at " << p << ".\n";

  return p;
}

void operator delete(void * p) throw()
{
  std::cerr << "delete() at " << p << ".\n";
  std::free(p);
}

int main()
{
  std::string s = "Hello World.";

  std::map<int, int> m { { 0, 1 } };

  return s.length();
}
Comment 1 Andrew Pinski 2011-10-02 23:03:17 UTC
I think you need to mark operator new and delete as being exported.
Comment 2 Paolo Carlini 2011-10-02 23:08:12 UTC
Note that basic_string<char>, at variance with any std::map instantiation, is exported by the *.so, in particular the functions managing memory.
Comment 3 Kerrek SB 2011-10-02 23:32:47 UTC
Thank you for the replies. Is this behaviour standard-conforming? Also, could you tell me how I "mark the operators as exported", or just anything else that'll make the program behave as expected?
Comment 4 Paolo Carlini 2011-10-02 23:41:39 UTC
I don't see anything non-standard conforming in the library, for sure.

Better asking Richard, I think, but I'm afraid you can't really do what you would like to do as soon as you start linking in code from *any* library (not just the standard C++ library and its basic_string instantiations) managing memory.
Comment 5 Daniel Krügler 2011-10-03 13:17:11 UTC
I would have expected that the shown program works as expected. I'm quoting
ISO/IEC 14882:2003(E) (but N3290 seems to say the same):

1) [lib.replacement.functions] p2+3:

"2 - A C++ program may provide the definition for any of eight dynamic memory allocation function signatures declared in header <new> (3.7.3, clause 18):[..]"

"3 - The program’s definitions are used instead of the default versions supplied by the implementation (18.4). Such replacement occurs prior to program startup (3.2, 3.6)."

[C++11 adds here: "The program’s definitions shall not be specified as inline." But we have no inline problem here]

2) [lib.new.delete.single] p2 or p11:

"Replaceable: a C++ program may define a function with this function signature that displaces the default version defined by the C++ Standard library."

Especially note that (2) says "this function signature", which does IMO not allow to require the addition of export directives or anything else.
Comment 6 Paolo Carlini 2011-10-03 13:22:23 UTC
I have no idea how this can be made to work with -fwhole-program, but other people know better.
Comment 7 Jonathan Wakely 2011-10-03 13:41:50 UTC
(In reply to comment #3)
> Thank you for the replies. Is this behaviour standard-conforming?

The documentation for -fwhole-program says that all functions become static, which I suspect makes your new and delete operators invalid for replacement functions.  Andrew's suggestion works, just add these declarations:

void * operator new(std::size_t n) throw(std::bad_alloc) __attribute__((externally_visible));
void operator delete(void * p) throw() __attribute__((externally_visible));

That prevents -fwhole-program from making them static.
Comment 8 Jonathan Wakely 2011-10-03 13:43:21 UTC
[basic.stc.dynamic.allocation] p1

"a program is ill-formed if an allocation function is declared in a namespace scope other than global scope or declared static in global scope."

So using -fwhole-program and not making replacement functions externally-visible produces an ill-formed program (which G++ should probably reject)
Comment 9 Paolo Carlini 2011-10-03 13:45:03 UTC
Oh, thanks Jon for testing that. Indeed, as far as I'm concerned, the issue is resolved.
Comment 10 Daniel Krügler 2011-10-03 13:49:01 UTC
(In reply to comment #8)

I agree that given the "make static" contract of -fwhole-program (which I was not aware about) the compiler behaves accordingly. I wonder whether it would be OK to consider the magic functions as special even for -fwhole-program, because they do have a "magic-kind-of" specification in the standard.
Comment 11 Jonathan Wakely 2011-10-03 15:02:37 UTC
we could add __attribute__((externally_visible)) on the declarations in <new>

I don't know what other side effects that would have
Comment 12 Paolo Carlini 2011-10-03 15:12:53 UTC
Yes, yesterday, a bit sleepy, I also wondered that, but I'm not knowledgeable enough of these mechanisms to say whether it would be otherwise safe.
Comment 13 Kerrek SB 2011-10-03 16:12:28 UTC
Very interesting. I understand that making the function static makes the program ill-formed, but it's still somewhat surprising that a compiler flag should turn a perfectly valid program into an invalid one.

Perhaps adding those visibility specifiers to the <new> header would be a good idea, since it'd be a pure-library solution.

Thanks in any case for clarifying this!
Comment 14 Richard Biener 2011-10-04 09:26:57 UTC
I think that if the user may override certain builtin functions the frontend
should be responsible to inherit the proper visibility of those replacements
so it works even with -fwhole-program (or -flto).

Confirmed as a C++ frontend issue.  To the middle-end those functions are
not in any way special.
Comment 15 Paolo Carlini 2011-10-04 09:36:45 UTC
Interesting, let's add Jason in CC.
Comment 16 Jonathan Wakely 2011-10-04 09:54:57 UTC
I don't think they're special to the front end either, it transforms "new T" into a call to the relevant library allocation function and then invokes a constructor.  The allocation functions are just ordinary functions implemented in libsupc++ but as weak symbols so they can be overridden.  If we just declare them externally_visible in libsupc++'s <new> won't that work?
Comment 17 rguenther@suse.de 2011-10-04 10:03:36 UTC
On Tue, 4 Oct 2011, redi at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50594
> 
> --- Comment #16 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-10-04 09:54:57 UTC ---
> I don't think they're special to the front end either, it transforms "new T"
> into a call to the relevant library allocation function and then invokes a
> constructor.  The allocation functions are just ordinary functions implemented
> in libsupc++ but as weak symbols so they can be overridden.  If we just declare
> them externally_visible in libsupc++'s <new> won't that work?

If it's required to include <new> to override them yes, but I doubt that
from looking at our testsuite ;)

Richard.
Comment 18 Jonathan Wakely 2011-10-04 10:12:25 UTC
Ah yes, my mistake, <new> is not required for all of them, [basic.stc.dynamic] says

The following allocation and deallocation functions (18.6) are implicitly declared in global scope in each translation unit of a program.
  void* operator new(std::size_t);
  void* operator new[](std::size_t);
  void operator delete(void*);
  void operator delete[](void*);
Comment 19 Kerrek SB 2011-10-04 11:59:39 UTC
> The following allocation and deallocation functions (18.6) are implicitly
declared in global scope in each translation unit of a program.

If those functions are declared implicitly, could you just amend that implicit declaration to include the visibility attribute?
Comment 20 Jason Merrill 2011-10-04 18:37:09 UTC
(In reply to comment #19)
> could you just amend that implicit declaration to include the visibility
> attribute?

Certainly.  They're declared in cxx_init_decl_processing:

      push_cp_library_fn (NEW_EXPR, newtype);
      push_cp_library_fn (VEC_NEW_EXPR, newtype);
Comment 21 Paolo Carlini 2011-10-11 23:30:07 UTC
I'm working on this. Indeed, the library bits at this point are more or less trivial (minor nit: __externally_visible__, not externally_visible).

However, I guess we need a simple testcase for the 4 implicitly defined without <new> included: can anybody figure out one?
Comment 22 Paolo Carlini 2011-10-11 23:39:47 UTC
... because otherwise I'm not confident I'm changing cxx_init_decl_processing in the right way: I have a patchlet which fiddles with newattrs and newtype, I *think* adding the attribute, it doesn't ICE ;), but I don't know if the attribute is really alive.
Comment 23 paolo@gcc.gnu.org 2011-10-12 18:41:03 UTC
Author: paolo
Date: Wed Oct 12 18:40:58 2011
New Revision: 179863

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=179863
Log:
2011-10-12  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/50594
	* decl.c (cxx_init_decl_processing): Add
	__attribute__((externally_visible)) to operator new and
	operator delete library fn.

2011-10-12  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/50594
	* libsupc++/new (operator new, operator delete): Decorate with
	__attribute__((__externally_visible__)).
	* include/bits/c++config: Add _GLIBCXX_THROW.
	* libsupc++/del_op.cc: Adjust.
	* libsupc++/del_opv.cc: Likewise.
	* libsupc++/del_opnt.cc: Likewise.
	* libsupc++/del_opvnt.cc: Likewise.
	* libsupc++/new_op.cc: Likewise.
	* libsupc++/new_opv.cc: Likewise.
	* libsupc++/new_opnt.cc: Likewise.
	* libsupc++/new_opvnt.cc: Likewise.
	* testsuite/18_support/50594.cc: New.
	* testsuite/ext/profile/mutex_extensions_neg.cc: Adjust dg-error
	line number.

Added:
    trunk/libstdc++-v3/testsuite/18_support/50594.cc
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/decl.c
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/c++config
    trunk/libstdc++-v3/libsupc++/del_op.cc
    trunk/libstdc++-v3/libsupc++/del_opnt.cc
    trunk/libstdc++-v3/libsupc++/del_opv.cc
    trunk/libstdc++-v3/libsupc++/del_opvnt.cc
    trunk/libstdc++-v3/libsupc++/new
    trunk/libstdc++-v3/libsupc++/new_op.cc
    trunk/libstdc++-v3/libsupc++/new_opnt.cc
    trunk/libstdc++-v3/libsupc++/new_opv.cc
    trunk/libstdc++-v3/libsupc++/new_opvnt.cc
    trunk/libstdc++-v3/testsuite/ext/profile/mutex_extensions_neg.cc
Comment 24 Paolo Carlini 2011-10-12 18:42:23 UTC
Fixed for 4.7.0.
Comment 25 David Fang 2012-03-12 16:31:03 UTC
Seeing this failing on powerpc-darwin8.
http://gcc.gnu.org/ml/gcc-testresults/2012-03/msg01296.html

Log shows failed assertion:
Executing on host: /Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/./gcc/g++ -shared-libgcc -B/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/./gcc -nostdinc++ -L/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/powerpc-apple-darwin8.11.0/libstdc++-v3/src -L/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/powerpc-apple-darwin8.11.0/libstdc++-v3/src/.libs -B/sw/lib/gcc4.7/powerpc-apple-darwin8.11.0/bin/ -B/sw/lib/gcc4.7/powerpc-apple-darwin8.11.0/lib/ -isystem /sw/lib/gcc4.7/powerpc-apple-darwin8.11.0/include -isystem /sw/lib/gcc4.7/powerpc-apple-darwin8.11.0/sys-include -B/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/powerpc-apple-darwin8.11.0/./libstdc++-v3/src/.libs -g -O2 -D_GLIBCXX_ASSERT -fmessage-length=0 -ffunction-sections -fdata-sections -g -O2 -g -O2 -DLOCALEDIR="." -nostdinc++ -I/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/powerpc-apple-darwin8.11.0/libstdc++-v3/include/powerpc-apple-darwin8.11.0 -I/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/powerpc-apple-darwin8.11.0/libstdc++-v3/include -I/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/gcc-4.7.0-RC-20120302/libstdc++-v3/libsupc++ -I/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/gcc-4.7.0-RC-20120302/libstdc++-v3/include/backward -I/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/gcc-4.7.0-RC-20120302/libstdc++-v3/testsuite/util /Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/gcc-4.7.0-RC-20120302/libstdc++-v3/testsuite/18_support/50594.cc   -fwhole-program ./libtestc++.a -L/sw/lib -liconv  -lm   -o ./50594.exe    (timeout = 600)
PASS: 18_support/50594.cc (test for excess errors)
Setting LD_LIBRARY_PATH to :/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/gcc:/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/powerpc-apple-darwin8.11.0/./libstdc++-v3/../libgomp/.libs:/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/powerpc-apple-darwin8.11.0/./libstdc++-v3/src/.libs::/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/gcc:/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/powerpc-apple-darwin8.11.0/./libstdc++-v3/../libgomp/.libs:/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/darwin_objdir/powerpc-apple-darwin8.11.0/./libstdc++-v3/src/.libs
/Volumes/Isolde/fink.build/gcc47-4.7.0-0.rc1.1/gcc-4.7.0-RC-20120302/libstdc++-v3/testsuite/18_support/50594.cc:64: failed assertion `user_new_called'
FAIL: 18_support/50594.cc execution test