RFR (trivial) 8197772: metaspace uses global operator new/delete for gtest testing

[Thomas Stüfe]

On Wed, Feb 14, 2018 at 5:13 PM, <coleen.phillimore at oracle.com> wrote:

>
>
> On 2/14/18 9:47 AM, Thomas Stüfe wrote:
>
> Hi Coleen,
>
> On Wed, Feb 14, 2018 at 3:21 PM, <coleen.phillimore at oracle.com> wrote:
>
>>
>> Thank you Thomas, I just pushed it.
>>
>> Now all three places where we call global operator new and delete are
>> fixed.   We'd like to remove ValueObj and VALUE_OBJ_CLASS_SPEC and add a
>> link time check.   See https://bugs.openjdk.java.net/browse/JDK-8173070
>> .  Do you object to this change?
>>
>> thanks,
>> Coleen
>>
>>
> No, I think it makes sense. Is the intent to get 100% coverage for c++
> allocated objects in NMT?
>
>
> Yes.  I think historically it was to encourage arena allocation.
>
>
> But would a global operator new() not replace the operator new() for any
> library in process space, including third party libraries? We played around
> with replacing the global operator new some years ago for memory tracking
> purposes but in the end refrained from doing it because of this. But my
> knowledge on this is several years out of date, I may be wrong.
>
>
> We haven't had that problem although there used to be a comment about
> this.  The current jvm replaces all the global operators with ones with
> asserts in src/share/memory/operator_new.cpp .
>

Oh I see. And in !PRODUCT only. So you are saying that this is already
established code and you have not seen problems with third party libraries
in your tests?

The more I think about this, the more confused I get. In
http://en.cppreference.com/w/cpp/memory/new/operator_new I read:

"The versions (1-4) are implicitly declared in each translation unit even
if the <new> header is not included. Versions (1-8) are replaceable: a
user-provided non-member function with the same signature defined anywhere
in the program, in any source file, replaces the default version. Its
declaration does not need to be visible."

Which to me means that we replace glob operator new for any C++ code
running anywhere in the process - depending on what the runtime linker does
- but you do not see any problems at all with your global operator new()
which asserts ? No one outside the libjvm ever uses new?

If this is really a process global resource, like signal handling, it feels
a bit rude. Especially if the libjvm gets embedded as "just another
library".

I might be completely off track here, I am certainly no expert, sorry.



>
> As for the linker tool mentioned in the issue description, we did just
> that, run nm on all object files to find offending news and raw mallocs.
>
>
> Oh, very good.  Did you add it to the build? Do you have a patch?
>
>
Unfortunately no. This was years ago, in our own commercial port. We have a
memory tracker similar to NMT (NMT did not exist yet) and then we wanted to
make sure we did catch all uninstrumented allocation sites. I think I just
used nm recursivly on all object files. I always did plan to add this to
our build and never got around it.

I also found that it is quite difficult to catch all allocations. It is not
only the usual suspects - malloc, calloc, realloc and friends - but also
things like strdup etc. And then there are quite a few APIs returning
C-Heap allocated memory, where calling free() afterwards is part of the
protocol (I always found that quite ugly). In the end, we managed to cover
~98% of the JDK with our memory tracker, but never got to 100%. We had to
change the design of our memory tracker to be able to cope with un-balanced
mallocs and frees. But all this is another topic...

Best Regards, Thomas

Thanks!
> Coleen
>
> Best Regards, Thomas
>
>
>>
>> On 2/14/18 3:24 AM, Thomas Stüfe wrote:
>>
>> Hi Coleen, thanks for fixing this.
>>
>> Kind Regards, Thomas
>>
>> On Wed, Feb 14, 2018 at 12:23 AM, <coleen.phillimore at oracle.com> wrote:
>>
>>> Summary: Inherit ChunkManagerReturnTestImpl from CHeapObj
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8197772.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8197772
>>>
>>> Tested with gtest that uses it.
>>>
>>> Thanks,
>>> Coleen
>>>
>>
>>
>>
>
>