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.
