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

[Jini George]

Thanks very much, Poonam, for this.

My comments inline.

On 9/16/2017 1:18 AM, Poonam Parhar wrote:
> 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?

Yes.

>
> 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.

Will do.

>
> 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.

Makes sense. Will do.


> 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.

Will do.

>
> 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?

"tgt_exception_port" and "saved_exception_types_count" can also be moved 
to the native side -- will do this. Looks like this would apply to 
"symbolicator" and "task" too.

Thanks,
Jini.



>
> 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.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>