RFR(S) 8205965: SIGSEGV on write to NativeCallStack::EMPTY_STACK

[Martin Buchholz]

Below is a possible untested attempt to /Construct On First Use Idiom/ that
doesn't (appear to) allocate, and gets rid of the double construction.

diff --git a/src/hotspot/share/services/mallocSiteTable.hpp
b/src/hotspot/share/services/mallocSiteTable.hpp
--- a/src/hotspot/share/services/mallocSiteTable.hpp
+++ b/src/hotspot/share/services/mallocSiteTable.hpp
@@ -42,7 +42,7 @@ class MallocSite : public AllocationSite

  public:
   MallocSite() :
-    AllocationSite<MemoryCounter>(NativeCallStack::EMPTY_STACK),
_flags(mtNone) {}
+    AllocationSite<MemoryCounter>(NativeCallStack::empty_stack()),
_flags(mtNone) {}

   MallocSite(const NativeCallStack& stack, MEMFLAGS flags) :
     AllocationSite<MemoryCounter>(stack), _flags(flags) {}
diff --git a/src/hotspot/share/services/memTracker.cpp
b/src/hotspot/share/services/memTracker.cpp
--- a/src/hotspot/share/services/memTracker.cpp
+++ b/src/hotspot/share/services/memTracker.cpp
@@ -68,10 +68,6 @@ NMT_TrackingLevel MemTracker::init_track
     os::unsetenv(buf);
   }

-  // Construct NativeCallStack::EMPTY_STACK. It may get constructed twice,
-  // but it is benign, the results are the same.
-  ::new ((void*)&NativeCallStack::EMPTY_STACK) NativeCallStack(0, false);
-
   if (!MallocTracker::initialize(level) ||
       !VirtualMemoryTracker::initialize(level)) {
     level = NMT_off;
diff --git a/src/hotspot/share/services/memTracker.hpp
b/src/hotspot/share/services/memTracker.hpp
--- a/src/hotspot/share/services/memTracker.hpp
+++ b/src/hotspot/share/services/memTracker.hpp
@@ -31,8 +31,8 @@

 #if !INCLUDE_NMT

-#define CURRENT_PC   NativeCallStack::EMPTY_STACK
-#define CALLER_PC    NativeCallStack::EMPTY_STACK
+#define CURRENT_PC   NativeCallStack::empty_stack()
+#define CALLER_PC    NativeCallStack::empty_stack()

 class Tracker : public StackObj {
  public:
@@ -86,9 +86,9 @@ class MemTracker : AllStatic {
 extern volatile bool NMT_stack_walkable;

 #define CURRENT_PC ((MemTracker::tracking_level() == NMT_detail &&
NMT_stack_walkable) ? \
-                    NativeCallStack(0, true) :
NativeCallStack::EMPTY_STACK)
+                    NativeCallStack(0, true) :
NativeCallStack::empty_stack())
 #define CALLER_PC  ((MemTracker::tracking_level() == NMT_detail &&
NMT_stack_walkable) ?  \
-                    NativeCallStack(1, true) :
NativeCallStack::EMPTY_STACK)
+                    NativeCallStack(1, true) :
NativeCallStack::empty_stack())

 class MemBaseline;
 class Mutex;
diff --git a/src/hotspot/share/services/virtualMemoryTracker.hpp
b/src/hotspot/share/services/virtualMemoryTracker.hpp
--- a/src/hotspot/share/services/virtualMemoryTracker.hpp
+++ b/src/hotspot/share/services/virtualMemoryTracker.hpp
@@ -302,7 +302,7 @@ class ReservedMemoryRegion : public Virt


   ReservedMemoryRegion(address base, size_t size) :
-    VirtualMemoryRegion(base, size), _stack(NativeCallStack::EMPTY_STACK),
_flag(mtNone) { }
+    VirtualMemoryRegion(base, size),
_stack(NativeCallStack::empty_stack()), _flag(mtNone) { }

   // Copy constructor
   ReservedMemoryRegion(const ReservedMemoryRegion& rr) :
diff --git a/src/hotspot/share/utilities/nativeCallStack.cpp
b/src/hotspot/share/utilities/nativeCallStack.cpp
--- a/src/hotspot/share/utilities/nativeCallStack.cpp
+++ b/src/hotspot/share/utilities/nativeCallStack.cpp
@@ -28,8 +28,6 @@
 #include "utilities/globalDefinitions.hpp"
 #include "utilities/nativeCallStack.hpp"

-const NativeCallStack NativeCallStack::EMPTY_STACK(0, false);
-
 NativeCallStack::NativeCallStack(int toSkip, bool fillStack) :
   _hash_value(0) {

diff --git a/src/hotspot/share/utilities/nativeCallStack.hpp
b/src/hotspot/share/utilities/nativeCallStack.hpp
--- a/src/hotspot/share/utilities/nativeCallStack.hpp
+++ b/src/hotspot/share/utilities/nativeCallStack.hpp
@@ -53,7 +53,10 @@
  */
 class NativeCallStack : public StackObj {
  public:
-  static const NativeCallStack EMPTY_STACK;
+  inline static const NativeCallStack& empty_stack() {
+    static const NativeCallStack EMPTY_STACK(0, false);
+    return EMPTY_STACK;
+  }

  private:
   address       _stack[NMT_TrackingStackDepth];


On Mon, Jul 2, 2018 at 12:41 PM, Zhengyu Gu <zgu at redhat.com> wrote:

> Hi Martin,
>
>
> On 07/02/2018 03:26 PM, Martin Buchholz wrote:
>
>> Zhengyu,
>>
>> Thanks for fixing this!
>>
>> I would have tried to use /Construct On First Use Idiom/
>> https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use
>> but nothing here is easy, unlike in Java with its static finals.
>> (and I'm not (yet) a hotspot engineer)
>>
>
> Thanks for the link.
>
> However, this pattern does not apply here, since we *cannot* allocate any
> objects at this point.
>
> Hotspot disables global new operator, and allocates any CHeapObj here will
> loop back to initialize NMT (when it is on). We have to workaround this
> problem in several places, see comments in services/mallocSiteTable.hpp,
> for an example.
>
> Thanks,
>
> -Zhengyu
>
>
>
>>
>> On Fri, Jun 29, 2018 at 6:04 AM, Zhengyu Gu <zgu at redhat.com <mailto:
>> zgu at redhat.com>> wrote:
>>
>>     Hi,
>>
>>     clang-6.0 and above, can deduce that NativeCallStack::EMPTY_STACK is
>>     all zeros, and since it is a static constant, it places the object
>>     in the read-only BSS data section.
>>
>>     To workaround static initialization ordering issue, NMT has to
>>     ensure EMPTY_STACK is initialized before turns itself on, which can
>>     happen in the middle of initialization of other static objects. In
>>     this case, it causes SIGSEGV while try to write to the read-only
>> memory.
>>
>>     The solution is to make EMPTY_STACk private and non-constant, but
>>     hands out constant version.
>>
>>     Bug: https://bugs.openjdk.java.net/browse/JDK-8205965
>>     <https://bugs.openjdk.java.net/browse/JDK-8205965>
>>     Webrev: http://cr.openjdk.java.net/~zgu/8205965/webrev.00/
>>     <http://cr.openjdk.java.net/~zgu/8205965/webrev.00/>
>>
>>     Test:
>>
>>        hotspot_nmt on Linux 64 (fastdebug and release)
>>
>>     Thanks,
>>
>>     -Zhengyu
>>
>>
>>