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.
