NEED REVIEWER RFR(S): JDK-8038392 Generating prelink cache breaks JAVA 'jinfo' utility normal behavior

[Dmitry Samersoff]

Serguei,

Thank you for the review, please see below.

On 2014-06-12 14:49, serguei.spitsyn at 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.spitsyn at 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.