> On 11 dec. 2015, at 11:54, Dmitry Samersoff <dmitry.samers...@oracle.com> > wrote: > > Staffan, > >> This is all platform specific code - and all the platforms are >> different. Of course, it would be good if all platforms reported good >> error message always - and we can continue working on that. > > OK. > >> It should get filled in in the ptrace_attach() call. > > if ph->pid == thr->lwp_id at ll. 409 the function return NULL but > doesn't touch err_buf.
if ph->pid == thr->lwp_id the if-statement is skipped and we continue in the while-loop. > > -Dmitry > > On 2015-12-11 11:12, Staffan Larsen wrote: >> >>> On 10 dec. 2015, at 23:20, Dmitry Samersoff >>> <dmitry.samers...@oracle.com <mailto:dmitry.samers...@oracle.com>> wrote: >>> >>> Staffan, >>> >>> 1. >>> >>> strerror_r comes in two versions - returning string (GNU) and returning >>> int (XSI) >>> >>> To make developers live more interesting GNU version doesn't guarantee >>> to fill buf with appropriate string, so you can't just void return value. >>> >>> Default version tends to be XSI now days and I'm not sure that build >>> system add -D_GNU_SOURCE on all platforms. >>> >>> So you have to add something like >>> >>> #if _POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && ! _GNU_SOURCE >>> use XSI strerror_r >>> #else >>> use GNU strerror_r >>> #endif >> >> We are getting the _GNU_SOURCE version when building. If that changes in >> the makefiles somewhere so that we get the XSI version, then this code >> should not compile - it should issue a warning when assigning an int to >> char*. >> >>> >>> Or just don't care of threads and use plain strerror - it widely used in >>> hotspot and it doesn't crash on a race. >>> >>> 2. >>> >>> err_buff remains empty if the attach fails at ll.412 of ps_proc.c, >>> but see below. >> >> It should get filled in in the ptrace_attach() call. >> >>> >>> 3. (* just an opinion *) >>> >>> Extra parameters to ptrace_attach/PGrab linux makes in different from >>> other platforms. >> >> This is all platform specific code - and all the platforms are >> different. Of course, it would be good if all platforms reported good >> error message always - and we can continue working on that. >> >> Thanks, >> /Staffan >> >>> >>> So I would prefer to have it unchanged - you can either mimic Solaris >>> behavior and add Pgrab_error() function or just move all strerror staff >>> to LinuxDebuggerLocal_attach0 - errno from ptrace_attach/Pgrab is >>> available there. >>> >>> (except calloc case, but if we can't allocate couple of bytes for struct >>> ps_prochandle we most likely will not see the error anyway). >>> >>> >>> -Dmitry >>> >>> >>> On 2015-12-10 18:10, Staffan Larsen wrote: >>>> Thanks! >>>> >>>>> On 10 dec. 2015, at 15:56, Thomas Stüfe <thomas.stu...@gmail.com >>>>> <mailto:thomas.stu...@gmail.com> >>>>> <mailto:thomas.stu...@gmail.com>> wrote: >>>>> >>>>> Hi Staffan, >>>>> >>>>> this looks fine now, thanks! >>>>> >>>>> ..Thomas >>>>> >>>>> On Thu, Dec 10, 2015 at 3:42 PM, Staffan Larsen >>>>> <staffan.lar...@oracle.com >>>>> <mailto:staffan.lar...@oracle.com> <mailto:staffan.lar...@oracle.com>> >>>>> wrote: >>>>> >>>>> Hi Thomas, >>>>> >>>>>> On 10 dec. 2015, at 15:27, Thomas Stüfe <thomas.stu...@gmail.com >>>>>> <mailto:thomas.stu...@gmail.com> >>>>>> <mailto:thomas.stu...@gmail.com>> wrote: >>>>>> >>>>>> Hi Staffan, >>>>>> >>>>>> thanks! >>>>>> >>>>>> It also just occurred to me that strerror is not considered >>>>>> threadsafe. Maybe strerror_r() would a better option (depending >>>>>> on the version you use you would have to check the return value >>>>>> for NULL, though). >>>>> >>>>> Updated here: http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.03/ >>>>> >>>>> Thanks, >>>>> /Staffan >>>>> >>>>>> >>>>>> sorry for this piecemeal review. >>>>>> >>>>>> Kind Regards, Thomas >>>>>> >>>>>> >>>>>> >>>>>> On Thu, Dec 10, 2015 at 3:22 PM, Staffan Larsen >>>>>> <staffan.lar...@oracle.com >>>>>> <mailto:staffan.lar...@oracle.com> <mailto:staffan.lar...@oracle.com>> >>>>>> wrote: >>>>>> >>>>>> Hi Thommas, >>>>>> >>>>>>> On 10 dec. 2015, at 14:49, Thomas Stüfe >>>>>>> <thomas.stu...@gmail.com >>>>>>> <mailto:thomas.stu...@gmail.com> <mailto:thomas.stu...@gmail.com>> >>>>>>> wrote: >>>>>>> >>>>>>> Hi Stefan, >>>>>>> >>>>>>> Disclaimer: not a "R"eviewer. >>>>>>> >>>>>>> >>>>>>> http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.01/agent/src/os/linux/LinuxDebuggerLocal.c.udiff.html >>>>>>> >>>>>>> I am not sure why you pass sizeof(err_buf) - 1 instead of >>>>>>> sizeof(err_buf) to Pgrab and sizeof(msg) - 1 instead of >>>>>>> sizeof(msg) to snprintf? snprintf will always zero terminate >>>>>>> in case of truncation, at least on posix platforms. >>>>>> >>>>>> Good point. I was just being overly cautious without thinking. >>>>>> >>>>>> Updated >>>>>> webrev: http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.02/ >>>>>> >>>>>> Thanks, >>>>>> /Staffan >>>>>> >>>>>>> >>>>>>> Otherwise this looks good. >>>>>>> >>>>>>> Kind Regards, Thomas >>>>>>> >>>>>>> >>>>>>> On Thu, Dec 10, 2015 at 2:19 PM, Staffan Larsen >>>>>>> <staffan.lar...@oracle.com <mailto:staffan.lar...@oracle.com> >>>>>>> <mailto:staffan.lar...@oracle.com>> wrote: >>>>>>> >>>>>>> Please review this patch to add a better error message >>>>>>> to SA when it fails to attach to a process on linux. >>>>>>> Currently the error says "Can't attach to the process”. >>>>>>> After this change the message will look something like: >>>>>>> "Can't attach to the process: ptrace(PTRACE_ATTACH, ..) >>>>>>> failed for 28417: Operation not permitted” >>>>>>> >>>>>>> webrev: >>>>>>> http://cr.openjdk.java.net/~sla/JDK-8145099/webrev.01/ >>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8145099 >>>>>>> >>>>>>> Thanks, >>>>>>> /Staffan >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>> >>> >>> >>> -- >>> Dmitry Samersoff >>> Oracle Java development team, Saint Petersburg, Russia >>> * I would love to change the world, but they won't give me the sources. >> > > > -- > Dmitry Samersoff > Oracle Java development team, Saint Petersburg, Russia > * I would love to change the world, but they won't give me the sources.