[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: NEED REVIEWER RFR(S): JDK-8038392 Generating prelink cache breaks JAVA 'jinfo' utility normal be
From: Dmitry Samersoff <dmitry.samersoff () oracle ! com>
Date: 2014-06-13 11:12:30
Message-ID: 539ADC9E.1090902 () oracle ! com
[Download RAW message or body]
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.samersoff@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.spitsyn@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@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@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.
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic