Review comments

Author: zhengyu123

ddprof-lib/src/main/cpp/flightRecorder.cpp

@@ -493,11 +493,12 @@ MethodInfo *Lookup::resolveMethod(ASGCT_CallFrame &frame) {
     method_id = JVMSupport::resolve(frame.method);
   }
 
+/**
   // Maps JMETHODID_NOT_WAKEABLE back to nullptr
   if (method_id == JMETHODID_NOT_WALKABLE) {
     method_id = nullptr;
   }
-
+*/
   // BCI_VTABLE_RECEIVER: method holds a VMSymbol* (see vmEntry.h). Resolve
   // to a class_id via the per-dump cache once, then key MethodMap by the
   // resolved class_id so two distinct Symbol addresses for the same class

ddprof-lib/src/main/cpp/flightRecorder.h

@@ -90,8 +90,8 @@ class MethodInfo {
   std::shared_ptr<SharedLineNumberTable> _line_number_table;
   FrameTypeId _type;
 
-  jint getLineNumber(jint bci);
-  bool isHidden();
+  inline jint getLineNumber(jint bci);
+  inline bool isHidden();
 };
 
 // MethodMap's key can be derived from 3 sources:

ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp

@@ -171,14 +171,15 @@ static void fillFrameTypes(ASGCT_CallFrame *frames, int num_frames, VMNMethod *n
 }
 
 // Fill the frame with raw method pointer
-static inline void fillFrameRaw(ASGCT_CallFrame& frame, FrameTypeId type, int bci, const VMMethod* method) {
+static void fillFrameRaw(ASGCT_CallFrame& frame, FrameTypeId type, int bci, const VMMethod* method) {
     assert(method != nullptr);
     frame.bci = FrameType::encode(type, bci, true /*raw method pointer*/);
     frame.method = static_cast<const void*>(method);
 }
 
-static inline void fillFrame(ASGCT_CallFrame& frame, FrameTypeId type, int bci, jmethodID method_id, const VMMethod* method) {
-    if (method_id != nullptr) {
+static void fillFrame(ASGCT_CallFrame& frame, FrameTypeId type, int bci, jmethodID method_id, const VMMethod* method) {
+    // Pack JMETHODID_NOT_WALKABLE frame as raw pointer frame, so it can not resolved into nullptr to shared code.
+    if (method_id != nullptr && method_id != JMETHODID_NOT_WALKABLE) {
         fillFrame(frame, type, bci, method_id);
     } else {
         assert(method != nullptr);
@@ -1250,7 +1251,7 @@ static bool isLambdaClass(const char* signature) {
            strncmp(signature, FFM_PREFIX, strlen(FFM_PREFIX)) == 0;
 }
 
-static inline bool isSystemClassLoaders(JNIEnv* jni, jobject cl) {
+static bool isSystemClassLoader(JNIEnv* jni, jobject cl) {
     return cl == nullptr ||                         // bootstrap class loader
            jni->IsSameObject(cl, JAVA_PLATFORM_CLASSLOADER) ||       // platform class loader
            jni->IsSameObject(cl, JAVA_APPLICATION_CLASSLOADER);      // application class loader (system class loader)
@@ -1271,7 +1272,7 @@ bool HotspotSupport::loadMethodIDsImpl(jvmtiEnv *jvmti, JNIEnv *jni, jclass klas
     // we use Method instead.
     if (!isHiddenClass(jvmti, klass) &&
         jvmti->GetClassLoader(klass, &cl) == JVMTI_ERROR_NONE && 
-        isSystemClassLoaders(jni, cl)) {
+        isSystemClassLoader(jni, cl)) {
         char* signature_ptr = nullptr;
         if (jvmti->GetClassSignature(klass, &signature_ptr, nullptr) == JVMTI_ERROR_NONE) {
             // Lambda classes, even loaded by bootstrap class loader, can be unloaded,
@@ -1299,6 +1300,13 @@ bool HotspotSupport::loadMethodIDsImpl(jvmtiEnv *jvmti, JNIEnv *jni, jclass klas
 jmethodID HotspotSupport::resolve(const void* method) {
   assert(VM::isHotspot());
   assert(method != nullptr);
+  // We packed not walkable method as raw pointer,
+  // map it back to nullptr, as JMETHODID_NOT_WALKABLE is only
+  // known in hotspot.
+  if ((jmethodID)method == JMETHODID_NOT_WALKABLE) {
+    return nullptr;
+  }
+
   VMMethod* vm_method = VMMethod::cast_or_null(method);
   if (vm_method == nullptr) {
     return nullptr;

ddprof-lib/src/main/cpp/hotspot/vmStructs.h

@@ -20,6 +20,11 @@
 #include "threadState.h"
 #include "vmEntry.h"
 
+#define JMETHODID_NOT_WALKABLE  (jmethodID)((uintptr_t)-1)
+inline bool isValidJMethodID(jmethodID method_id) {
+  return method_id != JMETHODID_NOT_WALKABLE && method_id != nullptr;
+}
+
 class GCHeapSummary;
 class HeapUsage;
 class VMNMethod;
@@ -48,7 +53,7 @@ inline T* cast_to(const void* ptr) {
 }
 
 template <typename T>
-inline T* cast_or_null(const void* ptr) {
+T* cast_or_null(const void* ptr) {
     assert(VM::isHotspot()); // This should only be used in HotSpot-specific code
     assert(T::type_size() > 0); // Ensure type size has been initialized
     if(ptr == nullptr || SafeAccess::isReadableRange(ptr, T::type_size())) {
@@ -615,7 +620,6 @@ class VMNMethod;
 class VMMethod;
 
 DECLARE(VMSymbol)
-  public:
     unsigned short length() {
         assert(_symbol_length_offset >= 0);
         return *(unsigned short*) at(_symbol_length_offset);
@@ -654,7 +658,6 @@ DECLARE(VMClassLoaderData)
 DECLARE_END
 
 DECLARE(VMKlass)    
-  public:
     static VMKlass* fromJavaClass(JNIEnv* env, jclass cls) {
         if (sizeof(VMKlass*) == 8) {
             return VMKlass::cast((const void*)(intptr_t)env->GetLongField(cls, _klass));
@@ -754,7 +757,6 @@ DECLARE(VMJavaFrameAnchor)
 DECLARE_END
 
 DECLARE(VMContinuationEntry)
-  public:
     // Address of the enterSpecial frame's {saved_fp, return_addr} pair.
     // Layout above this address: [saved_fp][return_addr_to_carrier][carrier_sp...]
     // The ContinuationEntry struct is embedded on the carrier stack immediately
@@ -795,7 +797,6 @@ enum JVMJavaThreadState {
 
 DECLARE(VMThread)
   friend class JVMThread;
-  public:
     static void* initialize(jthread thread);
 
     static inline VMThread* current();
@@ -889,15 +890,13 @@ DECLARE(VMThread)
 DECLARE_END
 
 DECLARE(VMConstantPool)
-public:
     inline VMKlass* holder_or_null() const;
     inline VMSymbol* symbolAt(int index) const;
  private:
     inline intptr_t* base() const;
 DECLARE_END
 
 DECLARE(VMConstMethod)
-public:
     inline VMConstantPool* constants_or_null() const;
     inline VMSymbol* name() const;
     inline VMSymbol* signature() const;
@@ -948,7 +947,6 @@ static inline bool startsWith(const char* s, const char (&pattern)[N]) {
 }
 
 DECLARE(VMNMethod)
-  public:
     int size() {
         assert(_blob_size_offset >= 0);
         return *(int*) at(_blob_size_offset);

ddprof-lib/src/main/cpp/hotspot/vmStructs.inline.h

@@ -110,15 +110,17 @@ jmethodID VMMethod::id() {
 
 jmethodID VMMethod::validatedId() {
     jmethodID method_id = id();
-    if (isValidJMethodID(method_id)) {
-        if (!_can_dereference_jmethod_id ||
-            ((goodPtr(method_id) && SafeAccess::loadPtr((void**)method_id, nullptr) == this))) {
-            return method_id;
-        } else {
-            return JMETHODID_NOT_WALKABLE;
-        }
+    // We are sure about the value, return it
+    if (method_id == JMETHODID_NOT_WALKABLE || method_id == nullptr) {
+        return method_id;
+    }
+    // Check if the value make sense
+    if (!_can_dereference_jmethod_id ||
+        ((goodPtr(method_id) && SafeAccess::loadPtr((void**)method_id, nullptr) == this))) {
+        return method_id;
     }
-    return method_id;
+
+    return JMETHODID_NOT_WALKABLE;
 }
 
 VMKlass* VMConstantPool::holder_or_null() const {

ddprof-lib/src/main/cpp/vmEntry.h

@@ -20,14 +20,6 @@
 #define DLLEXPORT __attribute__((visibility("default"), externally_visible))
 #endif
 
-#ifndef JMETHODID_NOT_WALKABLE
-#define JMETHODID_NOT_WALKABLE  (jmethodID)((uintptr_t)-1)
-#endif // JMETHODID_NOT_WALKABLE
-
-inline bool isValidJMethodID(jmethodID method_id) {
-  return method_id != JMETHODID_NOT_WALKABLE && method_id != nullptr;
-}
-
 // Denotes ASGCT_CallFrame where method_id has special meaning (not jmethodID)
 enum ASGCT_CallFrameType {
   BCI_CPU = 0,                  // cpu time