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.
