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

[Dmitry Samersoff]

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.spitsyn at 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.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.