RFR: SA: MacOS X: 8184042: several serviceability/sa tests timed out on MacOS X

[Poonam Parhar]

Hello Jini,

I don't know much about the mach exceptions either. I read through your 
wiki page and looked at the relevant gdb and lldb code to understand how 
it works.

A few points regarding the changes for this CR:

File: src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m

1. In the lldb code, a separate thread is being used to catch and handle 
the mach exceptions. I am assuming we don't need to do that since we use 
the mach exceptions mechanism only during the attach process, and after 
which we suspend all the threads in the target process.Is that correct?

2.

823 if (result != KERN_SUCCESS) {
824 print_error("attach: mach_port_allocate() failed: '%s' (%d)\n",
825 mach_error_string(result), result);

For all the mach calls in this function, would it make sense to print 
the exception_port as well while reporting errors to make is easier to 
debug the attach failures.

3.
843 result = task_get_exception_ports(gTask, 844 EXC_MASK_ALL, 845 
saved_masks, 846 &saved_exception_types_count, 847 saved_ports, 848 
saved_behaviors, 849 saved_flavors);

A comment for this call explaining why we are getting and saving the 
exceptions info here would be good. e.g. we need to restore the 
exceptions state for the process when we detach from it.

4. It would make the code look cleaner to have all these exception 
details that we need to save as the fields of a structure.

5. /tgt_exception_port/ and /saved_exception_types_count/ are saved on 
the Java side, but /saved_masks, //saved_ports, //saved_behaviors and 
//saved_flavors/ are defined as static on the native side. All of these 
values are process specific, and are needed when we detach from a 
process. Why are these being treated differently?

Thanks,
Poonam


On 9/14/2017 8:17 PM, Jini George wrote:
> Thanks much, Dan.
>
> -Jini.
>
> On 9/15/2017 4:18 AM, Daniel D. Daugherty wrote:
>> On 8/18/17 4:00 AM, Jini George wrote:
>>> Hi all,
>>>
>>> Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8184042
>>>
>>> Webrev: http://cr.openjdk.java.net/~jgeorge/8184042/webrev.00/
>>
>> hotspot/src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m
>>     L832:   result = mach_port_insert_right (mach_task_self(),
>>         Nit - extra blank before '('
>>
>>     L860-863 - nit - indents don't match the other func calls.
>>
>>     I read through all the changes and looked at logic flow, error
>>     returns, variable usage, etc. The problem is I'm not conversant
>>     in Mach exception programming so there could easily be things
>>     that I missed here.
>>
>>     You really need to have a MacOS X person look at these changes
>>     also.
>>
>> hotspot/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java
>>     No comments.
>>
>> hotspot/src/jdk.hotspot.agent/macosx/native/libsaproc/mach_exc.h
>> hotspot/src/jdk.hotspot.agent/macosx/native/libsaproc/mach_excServer.c
>> hotspot/src/jdk.hotspot.agent/macosx/native/libsaproc/mach_excUser.c
>>     These files are generated by a tool and will be replaced
>>     by build changes. I didn't review them.
>>
>> hotspot/test/serviceability/sa/TestAttachDetach.java
>>     L35: import java.util.ArrayList;
>>     L36: import java.util.List;
>>     L37: import java.util.Arrays;
>>     L38: import java.util.Optional;
>>     L40: import jdk.test.lib.Utils;
>>     L41: import jdk.test.lib.process.OutputAnalyzer;
>>     L42: import jdk.test.lib.process.ProcessTools;
>>         These imports do not seem to be needed.
>>
>>     L45: import jdk.test.lib.JDKToolLauncher;
>>         Duplicate import.
>>
>>     L98:                             "The String 'Attaching to 
>> process' appears " +
>>     L99:                              numberOfMatches + " number of 
>> times");
>>         Would be helpful to include the expected value also.
>>
>> jdk/test/ProblemList.txt
>>     Thanks for remembering to update the ProblemList.txt file!
>>
>> I don't have any comments other than nits. However, I don't
>> know mach exceptions so I can't really do a functional review.
>>
>> We need someone that knows MacOS X mach exceptions to chime
>> in on this thread. I'm adding Gerard Z to this review thread
>> in the hopes that he can help...
>>
>> Dan
>>
>>
>>>
>>> Problem gist: The deprecated ptrace() command, PT_ATTACH was changed 
>>> to PT_ATTACHEXC, which causes mach exceptions (and not UNIX signals) 
>>> to be delivered via mach messages.This caused SA to hang at 
>>> waitpid() waiting for a signal, which does not arrive.
>>>
>>> Solution in a nutshell: The solution is to make the required changes 
>>> to handle mach 'soft signal' exceptions in the form of mach messages 
>>> instead of signals, while attaching to and detaching from the target 
>>> process. The detailed steps are outlined in JBS.
>>>
>>> The changes appear huge due to the inclusion of pre-generated mach 
>>> exception handling files (mach_exc*). Since this is an integration 
>>> blocker, it would be great to get quick reviews on this.
>>>
>>> Thank you,
>>> Jini.
>>>
>>>
>>>
>>>
>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20170915/a1cbe715/attachment-0001.html>