Thanks! On 2014-06-13 14:58, Staffan Larsen wrote: > Looks good! > > One small nit: rectifing -> rectifying > > Thanks, > /Staffan > > On 13 jun 2014, at 12:47, Dmitry Samersoff <dmitry.samers...@oracle.com> > wrote: > >> Serguei, >> >> Thank you for the review. >> >> Updated webrev is here: >> >> http://cr.openjdk.java.net/~dsamersoff/JDK-8038392/webrev.03/ >> >> -Dmitry >> >> On 2014-06-13 12:14, serguei.spit...@oracle.com wrote: >>> Dmitry, >>> >>> Thank you for the explanations! >>> The fix looks good pending the simplification below. >>> I'm Ok with your testing approach. >>> >>> Thanks, >>> Serguei >>> >>> On 6/12/14 4:23 AM, Dmitry Samersoff wrote: >>>> Serguei, >>>> >>>> Thank you for the review, please see below. >>>> >>>> On 2014-06-12 14:49, serguei.spit...@oracle.com wrote: >>>>> Dmitry, >>>>> >>>>> Thank you for the details! >>>>> >>>>> I have a couple of comments so far. >>>>> >>>>> + if (word[5][0] == '[') { >>>>> + // not a shared library entry. ignore. >>>>> + if (strncmp(word[5],"[stack",6) == 0) { >>>>> + continue; >>>>> + } >>>>> + if (strncmp(word[5],"[heap]",6) == 0) { >>>>> + continue; >>>>> + } >>>>> + >>>>> + // SA don't handle VDSO >>>>> + if (strncmp(word[5],"[vdso]",6) == 0) { >>>>> + continue; >>>>> + } >>>>> + if (strncmp(word[5],"[vsyscall]",6) == 0) { >>>>> + continue; >>>>> + } >>>>> + } >>>>> >>>>> I'd suggest to simplify the fragment above to something like thus: >>>>> >>>>> + // SA does not handle the lines with patterns: >>>>> + // "[stack]", "[heap]", "[vdso]", "[vsyscall]", etc. >>>>> + if (word[5][0] == '[') { >>>>> + continue; // not a shared library entry, ignore >>>>> + } >>>> I'll change it. >>>> >>>>> This fragment does not look correct: >>>>> >>>>> + if (nwords > 6) { >>>>> + // prelink altered mapfile when the program is running. >>>>> + // Entries like one below have to be skipped >>>>> + // /lib64/libc-2.15.so (deleted) >>>>> + // SO name in entries like one below have to be stripped. >>>>> + // /lib64/libpthread-2.15.so.#prelink#.EECVts >>>>> + char *s = strstr(word[5],".#prelink#"); >>>>> + if (s == NULL) { >>>>> + // No prelink keyword. skip deleted library >>>>> + print_debug("skip shared object %s deleted by prelink\n", >>>>> word[5]); >>>>> + continue; >>>>> + } >>>>> + >>>>> + // Fall through >>>>> + print_debug("rectifing shared object name %s changed by >>>>> prelink\n", word[5]); >>>>> + *s = 0; >>>>> + } >>>> >>>> We always have word "(deleted)" at the end (see fragment of map >>>> below). >>>> >>>> But if the word "(deleted)" relates to original DSO we should skip the >>>> entry but if it relates to tmp file created by prelink we should accept >>>> this mapping - it's a new place for original library. >>>> >>>> >>>> Actually map entry looks like: >>>> >>>> 7f490b190000-7f490b1a8000 r-xp 00000000 08:01 350 >>>> /lib64/libpthread-2.15.so.#prelink#.EECVts (deleted) >>>> >>>> 7f490b1a8000-7f490b3a7000 ---p 00018000 08:01 350 >>>> /lib64/libpthread-2.15.so.#prelink#.EECVts (deleted) >>>> >>>> 7f490b3a7000-7f490b3a8000 r--p 00017000 08:01 350 >>>> /lib64/libpthread-2.15.so.#prelink#.EECVts (deleted) >>>> >>>> 7f490b3a8000-7f490b3a9000 rw-p 00018000 08:01 >>>> 350 /lib64/libpthread-2.15.so.#prelink#.EECVts >>>> (deleted) >>>> >>>> 7f490b3a9000-7f490b3ad000 rw-p 00000000 00:00 0 >>>> 7f490b3ad000-7f490b3cf000 r-xp 00000000 08:01 >>>> >>>> 48013 /lib64/ld-2.15.so (deleted) >>>> >>>> >>>>> The line with the pattern "(deleted)" has 7 words, but the line like: >>>>> <addr> /lib64/libpthread-2.15.so.#prelink#.EECVts >>>>> >>>>> has only 6 words, and so, your code will not process it properly. >>>>> >>>>> I'd expect something like this: >>>>> >>>>> + if (nwords > 6) { >>>>> + // prelink altered mapfile when the program is running. >>>>> + // Entries like one below have to be skipped: >>>>> + // /lib64/libc-2.15.so (deleted) >>>>> + print_debug("skip shared object %s deleted by prelink\n", >>>>> word[5]); >>>>> + continue; >>>>> + } else { >>>>> + char *s = strstr(word[5],".#prelink#"); >>>>> + if (s != NULL) { >>>>> + // There is the prelink keyword >>>>> + print_debug("rectifying shared object name %s changed by >>>>> prelink\n", word[5]); >>>>> + *s = 0; >>>>> + } >>>>> + } >>>>> >>>>> >>>>> Is it hard to add a unit test for this issue? >>>> It's not possible to test prelink during nightly as it affects entire OS >>>> (actually it's a bad idea to run prelink while java program is running). >>>> >>>> >>>> -Dmitry >>>> >>>> >>>> >>>>> Thanks, >>>>> Serguei >>>>> >>>>> On 6/12/14 3:10 AM, Dmitry Samersoff wrote: >>>>>> Serguei, >>>>>> >>>>>> *The problem:* >>>>>> >>>>>> If someone run prelink utility while Java program is running DSO's >>>>>> mappings in it's /proc/<pid>/maps become messy. >>>>>> >>>>>> After prelink it contains entry like >>>>>> >>>>>> <addr> /lib64/libc-2.15.so (deleted) >>>>>> <addr> /lib64/libpthread-2.15.so.#prelink#.EECVts >>>>>> >>>>>> instead of normal >>>>>> >>>>>> <addr> /lib64/libc-2.15.so >>>>>> >>>>>> Here is the letter from Carlos, describing the problem in details: >>>>>> https://bugs.openjdk.java.net/browse/JDK-8038392 >>>>>> >>>>>> *The fix:* >>>>>> >>>>>> The fix allows SA to handle situation above. >>>>>> >>>>>> Also I fix longstanding issue - maps file parser in SA doesn't skip >>>>>> [stack*, [heap] etc entry and attempts to open it as a DSO >>>>>> >>>>>> *Testing:* >>>>>> >>>>>> I tested it manually with a different of prelink options (no prelink, >>>>>> prelink all, prelink some libraries only) >>>>>> >>>>>> -Dmitry >>>>>> >>>>>> >>>>>> On 2014-06-12 13:50, serguei.spit...@oracle.com wrote: >>>>>>> Hi Dmitry, >>>>>>> >>>>>>> There is no description of the fix. >>>>>>> Could you, please, provide one? >>>>>>> What did you basically wanted to achieve? >>>>>>> Also, how did the fix been tested? >>>>>>> It seems, a unit test would be nice to have. >>>>>>> >>>>>>> Thanks, >>>>>>> Serguei >>>>>>> >>>>>>> On 6/5/14 12:08 AM, Dmitry Samersoff wrote: >>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8038392/webrev.02/ >>>>>>>> >>>>>>>> On 2014-05-20 17:47, Dmitry Samersoff wrote: >>>>>>>>> On 2014-04-07 16:56, Dmitry Samersoff wrote: >>>>>>>>>> Hi Everybody, >>>>>>>>>> >>>>>>>>>> Please review the patch >>>>>>>>>> >>>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8038392/webrev.02/ >>>>>>>>>> >>>>>>>>>> it's based on the patch contributed by Carlos Santos (see CR) >>>>>>>>>> and all >>>>>>>>>> credentials belong to him. >>>>>>>>>> >>>>>>>>>> -Dmitry >>>>>>>>>> >>>> >>> >> >> >> -- >> 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.
