NEED REVIEWER RFR(S): JDK-8038392 Generating prelink cache breaks JAVA 'jinfo' utility normal behavior
Staffan Larsen
staffan.larsen at oracle.com
Fri Jun 13 10:58:15 UTC 2014
Looks good!
One small nit: rectifing -> rectifying
Thanks,
/Staffan
On 13 jun 2014, at 12:47, Dmitry Samersoff <dmitry.samersoff at 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 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.
More information about the serviceability-dev
mailing list