Re: gcc 7.3: Replacing global operator new/delete in shared libraries

[Paul Smith]

On Wed, 2018-02-07 at 19:26 -0500, Paul Smith wrote:
> Thanks for the conversation.  I'm moving forward with a real global
> operator new/delete and working out the magic needed to ensure those
> symbols are not global in our shared library.

I remember one annoying thing I ran into: through compiler "magic" the
-fvisibility=hidden compile-time attribute is ignored when it comes to
global operator new/delete.  Similarly, it's a compiler error to force
visibility("hidden") in the declaration via __attribute__.

The only way to get operator new/delete to be hidden inside a shared
library is to really force it using a linker script that declares them
"local:" specifically.

Re: gcc 7.3: Replacing global operator new/delete in shared libraries

[Paul Smith]

On Thu, 2018-02-08 at 01:17 +0100, Marc Glisse wrote:
> On Wed, 7 Feb 2018, Paul Smith wrote:
> > > > My question is, what do I need to do to ensure this behavior
> > > > persists if I create a global operator new/delete?
> > > > 
> > > > Is it sufficient to ensure that the symbol for our shared
> > > > library global new/delete symbols are hidden and not global,
> > > > using a linker map or -fvisibility=hidden?
> > > 
> > > I think so (hidden implies not-interposable, so locally bound),
> > > but I don't have much experience there.
> > 
> > OK I'll pursue this for now.
> 
> I answered too fast. It isn't just new/delete that need to be hidden.
> It is also anything that uses them and might be used in both
> contexts.  For instance, std::allocator::allocate is an inline
> function that calls operator new. You get one version that calls
> new1, and one version that calls new2. If you don't do anything
> special, the linker keeps only one (more or less arbitrarily). So I
> believe you need -fvisibility=hidden to hide everything but a few
> carefully chosen interfaces.

At the moment I'm compiling all my code with -fvisibility=hidden, and
marking out specific symbols to be public in the source code, with
__attribute__((visibility("default"))).

This is nice andeasy, but I've grown frustrated with it.  In order to
run our unit tests we must statically link them with the code; we can't
create a shared library because all the internal symbols that the unit
tests want to refer to are hidden.  Since we have about 150 individual
unit test programs and each one statically links all the code it uses
(although we're using GTest framework and I call these "unit tests",
they're not really unit tests in the classical sense that they mock out
a single class for test), that uses a lot of disk space and takes a lot
of link time during the builds... it's already aggravating and we're
only adding more unit tests over time.

So in the near future I intend to reset this and compile all the code
without -fvisibility=hidden, then when I create our shared library I'll
generate a linker map to make only specific symbols visible and hide
everything else.

What would be ideal is if I could continue to use the visibility
attributes in the source code to mark out symbols I wanted to publish. 
If that information were preserved in the ELF output in a way I could
extract with a script running objdump or readelf on the object files,
for example, then I could automate the generation of the proper linker
map for my shared library.

However the last time I checked this, visibility("default") didn't
leave a trace in the object file unless -fvisibility=hidden was used to
make the default visibility hidden.

Too bad.

Re: gcc 7.3: Replacing global operator new/delete in shared libraries

[Paul Smith]

On Wed, 2018-02-07 at 16:38 -0700, Martin Sebor wrote:
> I'm not sure I see how defining operator new inline can work
> unless you recompile the world (i.e., all the DSOs used by
> the program, including libstdc++). As Marc already hinted,
> if libstdc++ dynamically allocates an object using the default
> operator new and returns that object to the program to destroy,
> it will end up causing a mismatch.  The same can happen in
> the opposite direction.  To avoid such mismatches the entire
> program needs to use a single operator new (each of
> the required forms), and the only safe way to do that
> is to define each out-of-line.

I'm aware of these issues, and I know it's a dangerous game.  As I
mentioned I wasn't surprised, really, that eventually it caught us out.

However, it did work.  We don't use huge amounts of the STL but we use
basics like vector, string, a bit of unordered_map, etc., and those
worked fine.  All that's required for it to work is that either both
the new and delete were both performed inside libstdc++, or both the
new and delete were performed in STL header file implementations.  In
the former case that memory is coming from the system alloc, not
jemalloc, but the amount is small enough that it's not worrisome.  In
the latter case it will use jemalloc via the inline.

The problem comes in where you do the new in an STL header and the
delete in the compiled libstdc++, or vice versa... that's what I ran
into in GCC 7.3.

On GNU/Linux you can just replace malloc/free using non-global symbols
in the shared library, to ensure even libstdc++.a implementations use
jemalloc.  Unfortunately this is not possible in Windows.


On Wed, 7 Feb 2018 Jonathan Wakely  writes:
> The code to read a string from an istream is instantiated in the
> library, so the string gets resized (allocating using malloc) by code
> inside libstdc++.so.6 and then the destructor is run (deallocating
> using your inline operator delete) in main.

We don't use C++ iostream in our code.  Although, I'm sure GTest does
use it in THEIR code.  Of course, for unit tests we don't really care
which allocator is used as long as there's no mismatch.  I haven't
investigated exactly how it all worked in 6.2, I can only tell you that
for our use, it did :).

Thanks for the conversation.  I'm moving forward with a real global
operator new/delete and working out the magic needed to ensure those
symbols are not global in our shared library.

Re: gcc 7.3: Replacing global operator new/delete in shared libraries

[Marc Glisse]

On Wed, 7 Feb 2018, Paul Smith wrote:


My question is, what do I need to do to ensure this behavior
persists if I create a global operator new/delete?

Is it sufficient to ensure that the symbol for our shared library
global new/delete symbols are hidden and not global, using a linker
map or -fvisibility=hidden?


I think so (hidden implies not-interposable, so locally bound), but
I don't have much experience there.


OK I'll pursue this for now.


I answered too fast. It isn't just new/delete that need to be hidden. It 
is also anything that uses them and might be used in both contexts. For 
instance, std::allocator::allocate is an inline function that calls 
operator new. You get one version that calls new1, and one version that 
calls new2. If you don't do anything special, the linker keeps only one 
(more or less arbitrarily). So I believe you need -fvisibility=hidden to 
hide everything but a few carefully chosen interfaces.


--
Marc Glisse

Re: gcc 7.3: Replacing global operator new/delete in shared libraries

[Jonathan Wakely]

On 7 February 2018 at 23:38, Martin Sebor wrote:
> On 02/06/2018 03:56 PM, Paul Smith wrote:
>>
>> Hi all.
>>
>> Hopefully this isn't too annoying a question :).
>>
>> My environment has been using GCC 6.2 (locally compiled) on GNU/Linux
>> systems.  We use a separate heap management library (jemalloc) rather
>> than the libc allocator.  The way we did this in the past was to
>> declare operator new/delete (all forms) as inline functions in a header
>> and ensure that this header was always the very first thing in every
>> source file, before even any standard header files.  I know that inline
>> operator new/delete isn't OK in the C++ standard, but in fact it has
>> worked for us on the systems we care about.
>
>
> I don't know if something has changed that would expose this
> problem but...
>
> I'm not sure I see how defining operator new inline can work
> unless you recompile the world (i.e., all the DSOs used by
> the program, including libstdc++). As Marc already hinted,
> if libstdc++ dynamically allocates an object using the default
> operator new and returns that object to the program to destroy,
> it will end up causing a mismatch.

And without actually checking the code, I'm pretty sure that's what
happens in this case:

#include 
int main()
{
  std::string s;
  std::cin >> s;
}

The code to read a string from an istream is instantiated in the
library, so the string gets resized (allocating using malloc) by code
inside libstdc++.so.6 and then the destructor is run (deallocating
using your inline operator delete) in main.

That would be my first guess at explaining the stack trace you showed.

Re: gcc 7.3: Replacing global operator new/delete in shared libraries

[Martin Sebor]

On 02/06/2018 03:56 PM, Paul Smith wrote:

Hi all.

Hopefully this isn't too annoying a question :).

My environment has been using GCC 6.2 (locally compiled) on GNU/Linux
systems.  We use a separate heap management library (jemalloc) rather
than the libc allocator.  The way we did this in the past was to
declare operator new/delete (all forms) as inline functions in a header
and ensure that this header was always the very first thing in every
source file, before even any standard header files.  I know that inline
operator new/delete isn't OK in the C++ standard, but in fact it has
worked for us on the systems we care about.


I don't know if something has changed that would expose this
problem but...

I'm not sure I see how defining operator new inline can work
unless you recompile the world (i.e., all the DSOs used by
the program, including libstdc++). As Marc already hinted,
if libstdc++ dynamically allocates an object using the default
operator new and returns that object to the program to destroy,
it will end up causing a mismatch.  The same can happen in
the opposite direction.  To avoid such mismatches the entire
program needs to use a single operator new (each of
the required forms), and the only safe way to do that
is to define each out-of-line.

Martin

PS This question was the subject of C++ CWG issue 412:
http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_closed.html#412




I'm attempting a toolchain upgrade which is switching to GCC 7.3 /
binutils 2.30 (along with many other updates).

Now when I run our code, I get a core on exit.  It appears an STL
container delete is invoking libc free() with a pointer to memory
allocated by jemalloc.

I suspect that between 6.2 and 7.3 something in the STL has been
modified to call new in a header file, so it's using our inline
operator new, but call the matching delete from inside libstdc++.a (we
link with static libstdc++ for portability), so it doesn't use our
inline operator delete.

While it's unfortunate for us, obviously that's a perfectly legal
implementation choice.  I don't know whether this is something the GCC
folks care about.  If so I can do more to track down the specifics.

If I create a real global operator new/delete, even keeping the inlined
versions, then the problem goes away (lending more weight to my guess
above).

I should point out that we don't use much STL memory so having some
compiled (not header-based) STL use the libc allocator is not a big
deal to us... it's just the mismatch which is a problem.

This leads to my question:

One of the things we provide is a shared library including much of our
code, and also jemalloc.  Users link this shared library with their
code and we do not want them to use our allocator.  By having all our
operator new/delete inlined we are sure that all our requests go to our
allocator and their requests do not.  It's a bit grungy, perhaps, but
it's worked well until now.

My question is, what do I need to do to ensure this behavior persists
if I create a global operator new/delete?

Is it sufficient to ensure that the symbol for our shared library
global new/delete symbols are hidden and not global, using a linker map
or -fvisibility=hidden?

Re: gcc 7.3: Replacing global operator new/delete in shared libraries

[Paul Smith]

On Wed, 2018-02-07 at 11:32 +0100, Marc Glisse wrote:
> On Tue, 6 Feb 2018, Paul Smith wrote:
> 
> > My environment has been using GCC 6.2 (locally compiled) on
> > GNU/Linux systems.  We use a separate heap management library
> > (jemalloc) rather than the libc allocator.  The way we did this in
> > the past was to declare operator new/delete (all forms) as inline
> > functions in a header
> 
> Are you sure you still have all forms? The aligned versions were
> added in gcc-7 IIRC.

Hm.  I didn't realize there were new forms in C++17; I had the throw
and no-throw versions and also sized delete.  No, I didn't create the
new ones.  I'll do that.

> > and ensure that this header was always the very first thing in
> > every source file, before even any standard header files.  I know
> > that inline operator new/delete isn't OK in the C++ standard, but
> > in fact it has worked for us on the systems we care about.
> 
> Inline usually works, but violating the ODR is harder... I would at
> least  use the always_inline attribute to improve chances (I assume
> that static  (or anonymous namespace) versions wouldn't work)

I definitely do that already; for example:

  inline __attribute__((__always_inline__)) void* operator new(size_t size)  ...

> > Now when I run our code, I get a core on exit.  It appears an STL
> > container delete is invoking libc free() with a pointer to memory
> > allocated by jemalloc.
> 
> An example would help the discussion.

Good point.  Here's an example (from a unit test crash, in GTest):

*** Error in `EncodedStreamTests.gtest': free(): invalid pointer:
0x7481d0a0 ***

Program received signal SIGABRT, Aborted.
0x74c62c37 in raise () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) bt
#0  0x74c62c37 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x74c66028 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x74c9f2a4 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#3  0x74cab82e in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#4  0x00539b89 in __gnu_cxx::new_allocator::deallocate
(this=, __p=) at /work/src/build/x86_64-
linux/cc/generic/x86_64-generic-linux-
gnu/include/c++/7.3.0/ext/new_allocator.h:125
#5  std::allocator_traits::deallocate (__a=...,
__n=, __p=) at /work/src/build/x86_64-
linux/cc/generic/x86_64-generic-linux-
gnu/include/c++/7.3.0/bits/alloc_traits.h:462
#6  std::__cxx11::basic_string::_M_destroy (__size=,
this=0x7fffe4e0) at /work/src/build/x86_64-linux/cc/generic/x86_64-
generic-linux-gnu/include/c++/7.3.0/bits/basic_string.h:226
#7  std::__cxx11::basic_string::_M_dispose (this=0x7fffe4e0) at
/work/src/build/x86_64-linux/cc/generic/x86_64-generic-linux-
gnu/include/c++/7.3.0/bits/basic_string.h:221
#8  std::__cxx11::basic_string::~basic_string (this=0x7fffe4e0,
__in_chrg=) at /work/src/build/x86_64-
linux/cc/generic/x86_64-generic-linux-
gnu/include/c++/7.3.0/bits/basic_string.h:647
#9  testing::internal::CodeLocation::~CodeLocation
(this=0x7fffe4e0, __in_chrg=) at
Tests/GTest/include/gtest/internal/gtest-internal.h:504
#10 testing::internal::MakeAndRegisterTestInfo (test_case_name=test_cas
e_name@entry=0x5d8ed7 "ESTests", name=name@entry=0x5d8ec9
"bytesForCount", type_param=type_param@entry=0x0, value_param=value_par
am@entry=0x0, code_location=..., fixture_class_id=fixture_class_id@entr
y=0x65dd5c ,
set_up_tc=0x48e010 ,
tear_down_tc=0x48e020 ,
factory=0x7481c000) at Tests/GTest/src/gtest.cc:2580
#11 0x00475286 in __static_initialization_and_destruction_0
(__priority=65535, __initialize_p=1) at EncodedStreamTests.cpp:351
#12 0x005d8dcd in __libc_csu_init ()
#13 0x74c4ded5 in __libc_start_main () from /lib/x86_64-linux-
gnu/libc.so.6
#14 0x00477375 in _start ()

> > My question is, what do I need to do to ensure this behavior
> > persists if I create a global operator new/delete?
> > 
> > Is it sufficient to ensure that the symbol for our shared library
> > global new/delete symbols are hidden and not global, using a linker
> > map or -fvisibility=hidden?
> 
> I think so (hidden implies not-interposable, so locally bound), but
> I don't have much experience there.

OK I'll pursue this for now.

Thanks!

Re: gcc 7.3: Replacing global operator new/delete in shared libraries

[Marc Glisse]

On Tue, 6 Feb 2018, Paul Smith wrote:


My environment has been using GCC 6.2 (locally compiled) on GNU/Linux
systems.  We use a separate heap management library (jemalloc) rather
than the libc allocator.  The way we did this in the past was to
declare operator new/delete (all forms) as inline functions in a header


Are you sure you still have all forms? The aligned versions were added in 
gcc-7 IIRC.



and ensure that this header was always the very first thing in every
source file, before even any standard header files.  I know that inline
operator new/delete isn't OK in the C++ standard, but in fact it has
worked for us on the systems we care about.


Inline usually works, but violating the ODR is harder... I would at least 
use the always_inline attribute to improve chances (I assume that static 
(or anonymous namespace) versions wouldn't work), since the optimizer may 
decide not to inline otherwise. Something based on visibility should be 
somewhat safer. But it still seems dangerous, some global libstdc++ object 
might be initialized using one allocator then used with another one...



I'm attempting a toolchain upgrade which is switching to GCC 7.3 /
binutils 2.30 (along with many other updates).

Now when I run our code, I get a core on exit.  It appears an STL
container delete is invoking libc free() with a pointer to memory
allocated by jemalloc.


An example would help the discussion.


My question is, what do I need to do to ensure this behavior persists
if I create a global operator new/delete?

Is it sufficient to ensure that the symbol for our shared library
global new/delete symbols are hidden and not global, using a linker map
or -fvisibility=hidden?


I think so (hidden implies not-interposable, so locally bound), but I 
don't have much experience there.


--
Marc Glisse