Add safeguard for J9-specific jmethodid unloading behaviour (version-dependent)

[MattAlp]

What does this PR do?:
Adds a safeguard to prevent crashes on certain versions of J9 for which we don't have certain API compliance guarantees assumed to be present by async-profiler / java-profiler.

Motivation:
Incident 32141

Additional Notes:
See incident channel and private Slack for further details, (private) customer info and correspondence informed this change.

How to test the change?:

For Datadog employees:

• If this PR touches code that signs or publishes builds or packages, or handles
  credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
• This PR doesn't touch any of that.
• JIRA: PROF-11094

Unsure? Have a question? Request a review!

[github-actions]

🔧 Report generated by pr-comment-scanbuild

Scan-Build Report

User:	runner@fv-az1334-408
Working Directory:	/home/runner/work/java-profiler/java-profiler/ddprof-lib/src/test/make
Command Line:	make -j4 clean all
Clang Version:	Ubuntu clang version 18.1.3 (1ubuntu1)
Date:	Wed Jan 8 14:07:12 2025

Bug Summary

Bug Type	Quantity	Display?
All Bugs	5
Logic error
Dereference of null pointer	3
Suspicious operation
Bitwise shift	1
Unused code
Dead nested assignment	1

Reports

Bug Group	Bug Type ▾	File	Function/Method	Line	Path Length
Suspicious operation	Bitwise shift	vmStructs.cpp	find	872	16
Unused code	Dead nested assignment	vmStructs.cpp	checkNativeBinding	977	1
Logic error	Dereference of null pointer	safeAccess.h	load	33	18
Logic error	Dereference of null pointer	symbols_linux.h	ElfParser	129	28
Logic error	Dereference of null pointer	flightRecorder.cpp	flush	1511	8

[github-actions]

🔧 Report generated by pr-comment-cppcheck

CppCheck Report

Warnings (7)

• The class 'RecordingBuffer' defines member function with name 'flushIfNeeded' also defined in its parent class 'Buffer'.
• Obsolete function 'alloca' called.
• Member variable 'ScopeDesc::_stream' is not initialized in the constructor.
• Member variable 'ScopeDesc::_method_offset' is not initialized in the constructor.
• Member variable 'ScopeDesc::_bci' is not initialized in the constructor.
• Member variable 'ElfParser::_vaddr_diff' is not initialized in the constructor.
• Either the condition 'vm_thread' is redundant or there is possible null pointer dereference: vm_thread.

Style Violations (403)

• Class 'Arguments' has a constructor with 1 argument that is not explicit.
• struct member 'Multiplier::symbol' is never used.
• struct member 'Multiplier::multiplier' is never used.
• class member 'Error::OK' is never used.
• Class 'AsyncSampleMutex' has a constructor with 1 argument that is not explicit.
• The class 'NativeFunc' does not declare a constructor although it has private member variables which likely require initialization.
• Class 'SpinLock' has a constructor with 1 argument that is not explicit.
• Local variable 'count' shadows outer function
• Local variable 'jni' shadows outer function
• Local variable 'size' shadows outer function
• Local variable 'prev' shadows outer function
• Variable 'cb1' can be declared as pointer to const
• Variable 'cb2' can be declared as pointer to const
• Variable 'keys' can be declared as pointer to const
• Parameter 'frames' can be declared as pointer to const
• Parameter 'frames' can be declared as pointer to const
• Variable 'keys' can be declared as pointer to const
• struct member 'CallTrace::truncated' is never used.
• struct member 'CallTrace::num_frames' is never used.
• class member 'CallTraceStorage::_current_table' is never used.
• class member 'CallTraceStorage::_overflow' is never used.
• class member 'CallTraceStorage::_lock' is never used.
• Local variable 'i' shadows outer variable
• Parameter 'prefixes' can be declared as reference to const
• Variable 'fd1' can be declared as pointer to const
• Variable 'fd2' can be declared as pointer to const
• Parameter 'name' can be declared as pointer to const
• Consider using std::transform algorithm instead of a raw loop.
• struct member 'Tag::value' is never used.
• class member 'Context::spanId' is never used.
• class member 'Context::rootSpanId' is never used.
• class member 'Context::checksum' is never used.
• struct member 'ContextPage::capacity' is never used.
• struct member 'ContextPage::storage' is never used.
• class member 'Contexts::_max_pages' is never used.
• class member 'Contexts::_pages' is never used.
• The function 'check' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'start' overrides a function in a base class but is not marked with a 'override' specifier.
• class member 'Shims::_tid_setter_ref' is never used.
• Class 'Dictionary' has a constructor with 1 argument that is not explicit.
• struct member 'DictRow::keys' is never used.
• struct member 'DictRow::next' is never used.
• Operator '|' with one operand equal to zero is redundant.
• Operator '|' with one operand equal to zero is redundant.
• Local variable 'table' shadows outer function
• Local variable 'pc_off' shadows outer variable
• Variable 'table' can be declared as pointer to const
• struct member 'FrameDesc::cfa' is never used.
• struct member 'FrameDesc::fp_off' is never used.
• struct member 'FrameDesc::pc_off' is never used.
• struct member 'FrameDesc::empty_frame' is never used.
• struct member 'FrameDesc::default_frame' is never used.
• class member 'DwarfParser::_name' is never used.
• class member 'DwarfParser::_image_base' is never used.
• class member 'DwarfParser::_capacity' is never used.
• class member 'DwarfParser::_prev' is never used.
• class member 'DwarfParser::_code_align' is never used.
• class member 'DwarfParser::_data_align' is never used.
• Class 'WallClockEpochEvent' has a constructor with 1 argument that is not explicit.
• class member 'LockEvent::_start_time' is never used.
• class member 'LockEvent::_end_time' is never used.
• class member 'LockEvent::_address' is never used.
• class member 'LockEvent::_timeout' is never used.
• class member 'ObjectLivenessEvent::_alloc' is never used.
• class member 'ObjectLivenessEvent::_skipped' is never used.
• class member 'ObjectLivenessEvent::_start_time' is never used.
• class member 'ObjectLivenessEvent::_age' is never used.
• class member 'ObjectLivenessEvent::_ctx' is never used.
• struct member 'QueueTimeEvent::_start' is never used.
• struct member 'QueueTimeEvent::_end' is never used.
• struct member 'QueueTimeEvent::_task' is never used.
• struct member 'QueueTimeEvent::_scheduler' is never used.
• struct member 'QueueTimeEvent::_origin' is never used.
• The class 'VirtualSpaceSummary' does not declare a constructor although it has private member variables which likely require initialization.
• The class 'GCHeapSummary' does not declare a constructor although it has private member variables which likely require initialization.
• The class 'CollectedHeap' does not declare a constructor although it has private member variables which likely require initialization.
• Class 'Element' has a constructor with 1 argument that is not explicit.
• Class 'NoField' has a constructor with 1 argument that is not explicit.
• Class 'MutexLocker' has a constructor with 1 argument that is not explicit.
• Class 'MethodList' has a constructor with 1 argument that is not explicit.
• Class 'ScopeDesc' has a constructor with 1 argument that is not explicit.
• The function 'is_nofield' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'check' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'start' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'stop' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'interval' overrides a function in a base class but is not marked with a 'override' specifier.
• Condition 'enabled()' is always false
• Condition 'usage._maxSize==-1' is always false
• Condition 'counters' is always false
• Condition 'i<names.size()' is always false
• Condition '!TSC::initialized()' is always false
• The scope of the variable 'class_name_id' can be reduced.
• The scope of the variable 'method_name_id' can be reduced.
• The scope of the variable 'method_sig_id' can be reduced.
• Local variable 'start' shadows outer function
• Parameter 'frame' can be declared as reference to const
• Variable 'strings' can be declared as reference to const
• Parameter 'args' can be declared as reference to const
• Parameter 'constants' can be declared as reference to const
• Parameter 'ptr' can be declared as pointer to const. However it seems that 'load32' is a callback function, if 'ptr' is declared with const you might also need to cast function pointer(s).
• Variable 'methods' can be declared as pointer to const
• Parameter 'func' can be declared as pointer to const
• Variable 'counters' can be declared as pointer to const
• Parameter 'event' can be declared as pointer to const
• Parameter 'event' can be declared as pointer to const
• Parameter 'event' can be declared as pointer to const
• Parameter 'event' can be declared as pointer to const
• Parameter 'event' can be declared as pointer to const
• Parameter 'event' can be declared as pointer to const
• Parameter 'event' can be declared as pointer to const
• Parameter 'event' can be declared as pointer to const
• Variable 'class_name_id' is assigned a value that is never used.
• Variable 'method_name_id' is assigned a value that is never used.
• Variable 'method_sig_id' is assigned a value that is never used.
• Parameter 'ptr' can be declared as pointer to const
• struct member 'CpuTime::real' is never used.
• struct member 'CpuTime::user' is never used.
• struct member 'CpuTime::system' is never used.
• struct member 'CpuTimes::proc' is never used.
• struct member 'CpuTimes::total' is never used.
• class member 'Recording::_agent_properties' is never used.
• class member 'Recording::_jvm_args' is never used.
• class member 'Recording::_jvm_flags' is never used.
• class member 'Recording::_java_command' is never used.
• class member 'Recording::_fd' is never used.
• class member 'Recording::_start_time' is never used.
• class member 'Recording::_recording_start_time' is never used.
• class member 'Recording::_start_ticks' is never used.
• class member 'Recording::_recording_start_ticks' is never used.
• class member 'Recording::_stop_time' is never used.
• class member 'Recording::_stop_ticks' is never used.
• class member 'Recording::_base_id' is never used.
• class member 'Recording::_bytes_written' is never used.
• class member 'Recording::_tid' is never used.
• class member 'Recording::_available_processors' is never used.
• class member 'Recording::_recorded_lib_count' is never used.
• class member 'Recording::_cpu_monitor_enabled' is never used.
• class member 'Recording::_last_times' is never used.
• class member 'FlightRecorder::_filename' is never used.
• The function 'name' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'interval' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'check' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'start' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'stop' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'enableEvents' overrides a function in a base class but is not marked with a 'override' specifier.
• class member 'ITimer::_cstack' is never used.
• struct member 'jvmtiFrameInfoExtended::native_frame_address' is never used.
• struct member 'jvmtiStackInfoExtended::frame_buffer' is never used.
• class member 'J9Ext::InstrumentableObjectAlloc_id' is never used.
• The class 'J9WallClock' does not declare a constructor although it has private member variables which likely require initialization.
• The function 'name' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'interval' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'start' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'stop' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'enableEvents' overrides a function in a base class but is not marked with a 'override' specifier.
• class member 'J9WallClock::_max_stack_depth' is never used.
• class member 'J9WallClock::_running' is never used.
• Class 'ReservoirSampler' has a constructor with 1 argument that is not explicit.
• The function 'name' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'interval' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'enableEvents' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'start' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'stop' overrides a function in a base class but is not marked with a 'override' specifier.
• Variable 'value' can be declared as pointer to const
• class member 'JfrMetadata::_initialized' is never used.
• class member 'JVM::_is_readable_pointer_resolved' is never used.
• class member 'JVM::_is_readable_pointer' is never used.
• class member 'VirtualSpaceSummary::_committed_end' is never used.
• class member 'GCHeapSummary::vptr' is never used.
• class member 'CollectedHeap::vptr' is never used.
• class member 'CollectedHeap::_gc_heap_log' is never used.
• class member 'CollectedHeap::_capacity_at_last_gc' is never used.
• class member 'CollectedHeap::_used_at_last_gc' is never used.
• struct member 'Chunk::prev' is never used.
• struct member 'Chunk::offs' is never used.
• struct member 'Chunk::_padding' is never used.
• class member 'LinearAllocator::_chunk_size' is never used.
• class member 'LinearAllocator::_tail' is never used.
• class member 'LinearAllocator::_reserve' is never used.
• Local variable 'start' shadows outer function
• Local variable 'start' shadows outer function
• Parameter 'args' can be declared as reference to const
• Parameter 'event' can be declared as reference to const
• Parameter 'frames' can be declared as pointer to const
• Unused variable: kept_classes
• struct member 'TrackingEntry::alloc' is never used.
• struct member 'TrackingEntry::skipped' is never used.
• struct member 'TrackingEntry::frames' is never used.
• struct member 'TrackingEntry::ctx' is never used.
• class member 'LivenessTracker::MAX_TRACKING_TABLE_SIZE' is never used.
• class member 'LivenessTracker::MIN_SAMPLING_INTERVAL' is never used.
• class member 'LivenessTracker::_table_lock' is never used.
• Condition 'level>=_level' is always true
• Parameter 'args' can be declared as reference to const
• class member 'Log::_file' is never used.
• class member 'Log::LEVEL_NAME' is never used.
• class member 'ObjectSampler::CONFIG_UPDATE_CHECK_PERIOD_SECS' is never used.
• class member 'JitWriteProtection::_prev' is never used.
• class member 'JitWriteProtection::_restore' is never used.
• class member 'OS::page_size' is never used.
• class member 'OS::page_mask' is never used.
• The function 'rewind' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'next' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'size' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'check' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'start' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'enableEvents' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'registerThread' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'unregisterThread' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'interval' overrides a function in a base class but is not marked with a 'override' specifier.
• The function 'enableEvents' overrides a function in a base class but is identical to the overridden function
• The function 'registerThread' overrides a function in a base class but is identical to the overridden function
• The function 'unregisterThread' overrides a function in a base class but is identical to the overridden function
• Condition 'i<PROBE_SP_LIMIT' is always false
• Condition 'event_name==NULL' is always true
• Variable 'sp' can be declared as reference to const
• Variable 'pc' can be declared as reference to const
• Parameter 'args' can be declared as reference to const
• Parameter 'java_ctx' can be declared as pointer to const
• Variable 'error' is assigned a value that is never used.
• Variable 'error' is assigned a value that is never used.
• Variable 'error' is assigned a value that is never used.
• Variable 'error' is assigned a value that is never used.
• Condition 'length>0' is always false
• class member 'Profiler::_signals_initialized' is never used.
• class member 'Profiler::_class_map_lock' is never used.
• class member 'Profiler::_locks' is never used.
• Condition 'strcmp(name,"forward_copy_longs")==0' is always false
• Condition 'strcmp(name,"backward_copy_longs")==0' is always false
• Variable 'ip' can be declared as pointer to const
• Parameter 'ip' can be declared as pointer to const
• Variable 'ip' can be declared as pointer to const
• Parameter 'pc' can be declared as pointer to const
• Unused private function: 'StackFrame::withinCurrentStack'
• Variable 'ip' can be declared as pointer to const
• Parameter 'entry' can be declared as pointer to const
• Variable 'ip' can be declared as pointer to const
• Parameter 'pc' can be declared as pointer to const
• Variable 'ip' can be declared as pointer to const
• Variable 'ip' can be declared as pointer to const
• Variable 'entry' can be declared as pointer to const
• Parameter 'pc' can be declared as pointer to const
• Parameter 'sp' can be declared as reference to const
• Variable 'inst' can be declared as pointer to const
• Variable 'inst' can be declared as pointer to const
• Parameter 'pc' can be declared as pointer to const
• Variable 'ip' can be declared as pointer to const
• Parameter 'pc' can be declared as pointer to const
• Local variable 'pc' shadows outer function
• Return value 'ptr>(const void*)-DEAD_ZONE' is always true
• Condition '!inDeadZone(method)' is always false
• Condition '!inDeadZone(method_id)' is always false
• Condition 'inDeadZone(pc)' is always true
• Condition 'inDeadZone(pc)' is always true
• Condition 'is_plausible_interpreter_frame' is always false
• Condition 'inDeadZone(pc)' is always true
• The comparison 'EMPTY_FRAME_SIZE > 0' is always false.
• The comparison 'EMPTY_FRAME_SIZE > 0' is always false.
• The scope of the variable 'prev_sp' can be reduced.
• Local variable 'frame' shadows outer variable
• The comparison 'EMPTY_FRAME_SIZE > 0' is always false.
• The comparison 'EMPTY_FRAME_SIZE > 0' is always false.
• Class 'SymbolDesc' has a constructor with 1 argument that is not explicit.
• Class 'MemoryMapDesc' has a constructor with 1 argument that is not explicit.
• Local variable 'section' shadows outer function
• Local variable 'section' shadows outer function
• Local variable 'section' shadows outer function
• Variable 'ident' can be declared as pointer to const
• Parameter 'section' can be declared as pointer to const
• Variable 'checked' can be declared as pointer to const
• Variable 'pheader' can be declared as pointer to const
• Variable 'sym' can be declared as pointer to const
• Variable 'sym' can be declared as pointer to const
• Variable 'chain' can be declared as pointer to const
• Variable 'plt' can be declared as pointer to const
• Variable 'note' can be declared as pointer to const
• Variable 'sym' can be declared as pointer to const
• Variable 'sym' can be declared as pointer to const
• Local variable 'tid' shadows outer function
• Local variable 'tid' shadows outer function
• class member 'ProfiledThread::_buffer_size' is never used.
• class member 'ProfiledThread::_buffer' is never used.
• Local variable 'word' shadows outer function
• class member 'ThreadFilter::_max_bitmaps' is never used.
• class member 'ThreadInfo::_thread_names' is never used.
• class member 'ThreadInfo::_thread_ids' is never used.
• class member 'Trap::_page_start' is never used.
• Local variable 'frequency' shadows outer function
• Local variable 'flag_addr' shadows outer variable
• Variable 'idx' can be declared as pointer to const
• Parameter 'prop_value' can be declared as pointer to const
• Variable 'flag_addr' can be declared as pointer to const
• Parameter 'options' can be declared as pointer to const
• struct member 'ASGCT_CallTrace::env' is never used.
• struct member 'ASGCT_CallTrace::frames' is never used.
• struct member 'VMManagement::unused' is never used.
• struct member 'JVMTIFunctions::unused1' is never used.
• struct member 'JVMTIFunctions::unused2' is never used.
• struct member 'JavaFullVersion::major' is never used.
• struct member 'JavaFullVersion::update' is never used.
• class member 'VM::_can_intercept_binding' is never used.
• class member 'VM::_libjvm' is never used.
• class member 'VM::_libjava' is never used.
• class member 'VM::_asyncGetCallTrace' is never used.
• The scope of the variable 'delayedCounter' can be reduced.
• Local variable 'type' shadows outer function
• Parameter 'ptr' can be declared as pointer to const. However it seems that 'load32' is a callback function, if 'ptr' is declared with const you might also need to cast function pointer(s).
• Variable 'code_heap_array' can be declared as pointer to const
• Variable 'blob' can be declared as pointer to const
• Parameter 'vm_thread' can be declared as pointer to const
• Variable 'segmap' can be declared as pointer to const
• Variable 'type_name' can be declared as pointer to const
• Variable 'pcd_end' can be declared as pointer to const
• Consider using std::any_of, std::all_of, std::none_of, or std::accumulate algorithm instead of a raw loop.
• Variable 'error' is assigned a value that is never used.
• class member 'VMStructs::_has_native_thread_id' is never used.
• class member 'VMStructs::_class_loader_data_next_offset' is never used.
• class member 'VMStructs::_scopes_pcs_offset' is never used.
• class member 'VMStructs::_constmethod_constants_offset' is never used.
• class member 'VMStructs::_constmethod_idnum_offset' is never used.
• class member 'VMStructs::_pool_holder_offset' is never used.
• class member 'VMStructs::_array_data_offset' is never used.
• class member 'VMStructs::_code_heap_segmap_offset' is never used.
• class member 'VMStructs::_code_heap_segment_shift' is never used.
• class member 'VMStructs::_heap_block_used_offset' is never used.
• class member 'VMStructs::_vs_low_bound_offset' is never used.
• class member 'VMStructs::_vs_high_bound_offset' is never used.
• class member 'VMStructs::_flag_type_offset' is never used.
• class member 'VMStructs::_flags_addr' is never used.
• class member 'VMStructs::_flag_count' is never used.
• class member 'VMStructs::_flag_size' is never used.
• class member 'VMStructs::_code_heap_low_addr' is never used.
• class member 'VMStructs::_code_heap_high_addr' is never used.
• class member 'VMStructs::_klass_offset_addr' is never used.
• class member 'VMStructs::_narrow_klass_base_addr' is never used.
• class member 'VMStructs::_narrow_klass_base' is never used.
• class member 'VMStructs::_narrow_klass_shift_addr' is never used.
• class member 'VMStructs::_narrow_klass_shift' is never used.
• class member 'VMStructs::_collected_heap' is never used.
• class member 'VMStructs::_collected_heap_reserved_offset' is never used.
• class member 'VMStructs::_region_start_offset' is never used.
• class member 'VMStructs::_region_size_offset' is never used.
• class member 'VMStructs::_markword_klass_shift' is never used.
• class member 'VMStructs::_markword_monitor_value' is never used.
• class member 'VMStructs::_unsigned5_base' is never used.
• class member 'VMStructs::_call_stub_return_addr' is never used.
• class member 'VMStructs::_find_flag_func' is never used.
• class member 'VMStructs::_is_valid_method_func' is never used.
• class member 'HeapUsage::is_jmx_attempted' is never used.
• class member 'PcDesc::_pc' is never used.
• class member 'PcDesc::_scope_offset' is never used.
• class member 'PcDesc::_obj_offset' is never used.
• class member 'PcDesc::_flags' is never used.
• Local variable 'interval' shadows outer function
• Variable 'context' can be declared as reference to const
• struct member 'ThreadEntry::native' is never used.
• The function 'patchImport' is never used.
• The function 'getCounter' is never used.
• The function 'supported' is never used.
• The function 'updateCounter' is never used.
• The function 'deallocateLineNumberTable' is never used.
• The function 'copyTo' is never used.
• The function 'cpuMonitorCycle' is never used.
• The function 'appendRecording' is never used.
• The function 'writeListSetting' is never used.
• The function 'units' is never used.
• The function 'can_use_ASGCT' is never used.
• The function 'is_jmethodid_safe' is never used.
• The function 'GetJ9vmThread' is never used.
• The function 'is_readable_pointer' is never used.
• The function '_min' is never used.
• The function 'trace' is never used.
• The function 'waitUntil' is never used.
• The function 'notify' is never used.
• The function 'cputime' is never used.
• The function 'ntoh64' is never used.
• The function 'schedPolicy' is never used.
• The function 'fileSize' is never used.
• The function 'getJavaTraceJvmti' is never used.
• The function 'getJavaTraceInternal' is never used.
• The function 'flushJfr' is never used.
• The function 'restart' is never used.
• The function 'arg0' is never used.
• The function 'arg2' is never used.
• The function 'jarg0' is never used.
• The function 'ret' is never used.
• The function 'clearParsingCaches' is never used.
• The function 'isDeepCrashHandler' is never used.
• The function 'getThreadId' is never used.
• The function 'assign' is never used.
• The function 'covers' is never used.
• The function 'install' is never used.
• The function 'uninstall' is never used.
• The function 'loaded' is never used.
• The function 'hasClassNames' is never used.
• The function 'hasCompilerStructs' is never used.
• The function 'body' is never used.
• The function 'fromHandle' is never used.
• The function 'fromMethodID' is never used.
• The function 'isEntryFrame' is never used.
• The function 'inJava' is never used.
• The function 'lastJavaFP' is never used.
• The function 'compiledMethod' is never used.

[r1viollet]

Thanks for looking into this.
Is it possible to add a test for this behaviour ?

[jbachorik]

Is it possible to add a test for this behaviour ?

This would probably be almost impossible. We are getting the wrong method ID from a JVMTI call. Hence, we would need to stub the JNI call with some complex patching just for the duration of the test case (patching via LD_PRELOAD, if at all possible, would be too wide and we might fail way before we actually get to the code we want to test).

One thing that is fishy is that jvmti->GetMethodDeclaringClass(method, &method_class) is returning -1 in method_class but not returning an error code. By the spec if this function is not able to properly handle the provided methodid it should return [JVMTI_ERROR_INVALID_METHODID](https://docs.oracle.com/javase/7/docs/platform/jvmti/jvmti.html#JVMTI_ERROR_INVALID_METHODID)

[MattAlp]

My understanding from the internally shared docs is that this error return code is guaranteed only for more recent patches of J9, and can return OK with a reference to -1. This lines up with the incident reporting that the JVMTI calls succeed but then on the crash stack, we see

Registers

rip 0x7fb9e9766ef6 <jvmtiGetClassSignature+198>
rbp 0xffffffffffffffff (-1)

Ops

0x00007fb9e9766e3c <+12>: mov %rsi,%rbp <-- save the second parameter jclazz to rbp
0x00007fb9e9766ed5 <+165>: test %rbp,%rbp <-- check if jclazz is null
0x00007fb9e9766ede <+174>: mov 0x0(%rbp),%rbp <-- dereference jclazz and save to rbp
0x00007fb9e9766ee2 <+178>: test %rbp,%rbp <-- check if rbp is null
=> 0x00007fb9e9766ef6 <+198>: mov 0x4(%rbp,%rax,1),%r10 <-- crash here because rbp is not valid.

[MattAlp]

This would probably be almost impossible. We are getting the wrong method ID from a JVMTI call. Hence, we would need to stub the JNI call with some complex patching just for the duration of the test case (patching via LD_PRELOAD, if at all possible, would be too wide and we might fail way before we actually get to the code we want to test).

Agreed. The surface area of stubbing and testing something like this would quickly require finicky setup and/or stubbing out a lot of JVMTI and/or core JVM functionality.

[jbachorik]

Just one thing - could you add a comment about the J9 using -1 for unloaded methods so it is visible that this is actually a bug in J9 JVMTI implementation where they fail to return error code for unloaded method but rather segfault?

[r1viollet]

minor / style: we could have declared the -1 value as a constexpr value (k_invalid_j9_method_class ?)
Is checking the -1 value not enough (to avoid the isOpenJ9 call) ?

[jbachorik]

Hm, now when rereading the comment, this does not make much sense.
The info from J9 engineers is about invalid jmethodid becoming -1 - but that's not what we were seeing, right? It was the jclass returned from GetMethodDeclaringClass on such invalid jmethodid that was -1.
I mean, the workaround would work but the root cause seems to be J9 not handling the invalid jmethodids consistently in the JVMTI calls itself (eg. GetMethodDeclaringClass when called on an invalid jmethodid should return an error code instead of putting -1 to a jclass variable 🤷

[MattAlp]

the root cause seems to be J9 not handling the invalid jmethodids consistently in the JVMTI calls itself

I agree with this. I believe that this fixed in later versions of J9, but is considered acceptable for others (hence version-dependent in the PR title). It would be more accurate to do a specific J9 version check here rather than simply !VM::isOpenJ9(), but that opens up the possibility of us missing some versions in our known-to-be-buggy list.

[MattAlp]

@r1viollet

1. I'll constexpr, it doesn't hurt
2. I checked isOpenJ9() earlier - it returns a boolean based on a member that is set once at profiler startup, so I think the call is negligible. I added the check prior to adding the comments for the sake of self-documenting code, but also to remove the comparison for non-J9 VMs. WDYT - remove it?

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

Line 169 in 82a3272

static bool isOpenJ9() { return _openj9; }

[r1viollet]

1. As a general guidance we would want to avoid reinterpret casts when possible. The constexpr part is just a good habit to take (though sometimes hard to apply if we use old C++ versions)
2. The j9 call is more a question of taste and obviously very minor.

[r1viollet]

This would probably be almost impossible. We are getting the wrong method ID from a JVMTI call. Hence, we would need to stub the JNI call with some complex patching just for the duration of the test case (patching via LD_PRELOAD, if at all possible, would be too wide and we might fail way before we actually get to the code we want to test).

Agreed. The surface area of stubbing and testing something like this would quickly require finicky setup and/or stubbing out a lot of JVMTI and/or core JVM functionality.

Capturing JVM invariants per distributions seems like something very interesting.
I was not particularly precise on what type of test I was referring to. Do you think a unit test mocking the interactions would be difficult to write ?

[jbachorik]

Do you think a unit test mocking the interactions would be difficult to write ?

The problem is that it is the JVMTI function itself which behaves not according to the spec. I mean, theoretically we could create tests for spec conformance of the JVMTI implementation per JDK distribution but that would be no small task (not even considering the cost of running and maintaining the suite)