URG! Need a second reviewer! Re: RR(S): JDK-8025812 tmtools/jmap/heap_config tests fail on Linux-ia32 because it 'Can't attach to the core file'

[David Holmes]

On 17/10/2013 9:01 PM, Staffan Larsen wrote:
> Thanks - good to go!

+1

Thanks,
David

> On 17 okt 2013, at 12:54, Dmitry Samersoff <Dmitry.Samersoff at oracle.com> wrote:
>
>> Staffan,
>>
>> On 2013-10-17 14:46, Staffan Larsen wrote:
>>> This version looks good. A minor nit is a missing space after the comma in calls to ROUNDUP.
>>
>> Thank you (nit fixed and webrev reloaded)
>>
>> -Dmitry
>>
>>>
>>> /Staffan
>>>
>>>
>>> On 17 okt 2013, at 12:10, Dmitry Samersoff <dmitry.samersoff at oracle.com> wrote:
>>>
>>>> David,
>>>>
>>>> OK. I'm guilty overcomplicating simple things. I step back and
>>>> followed Staffan advice to just use ROUNDUP macro directly.
>>>>
>>>> Webrev changed in-place.
>>>>
>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8025812/webrev.01/
>>>>
>>>> -Dmitry
>>>>
>>>> On 2013-10-17 06:46, David Holmes wrote:
>>>>> Hi Dmitry,
>>>>>
>>>>> In
>>>>>
>>>>> 716 static inline size_t round_page(int x){
>>>>> 717   static int page_size;id,
>>>>> 718
>>>>> 719   page_size = sysconf(_SC_PAGE_SIZE);
>>>>> 720   return ROUNDUP(x,page_size);
>>>>> 721 }
>>>>>
>>>>> Does page_size being static mean that line 719 will only be executed
>>>>> once? Otherwise it serves no purpose.
>>>>>
>>>>> Typo: exisiting
>>>>>
>>>>> Otherwise seems okay.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>> On 17/10/2013 7:11 AM, Dmitry Samersoff wrote:
>>>>>> On 2013-10-14 11:49, Staffan Larsen wrote:
>>>>>>> The fix looks good, but I have a problem with the ROUNDUP_PAGE macro.
>>>>>>> First, I don't like having macros defined in the middle of a method.
>>>>>>> Second, the definition of the macro includes the value of a local
>>>>>>> variable which is a bit hairy. Can't you just ROUNDUP directly in the
>>>>>>> four places it's needed? I think it would make for more readable code.
>>>>>>>
>>>>>>> nit on line 743: filed -> field
>>>>>>>
>>>>>>> Thanks,
>>>>>>> /Staffan
>>>>>>>
>>>>>>> On 12 okt 2013, at 13:25, Dmitry Samersoff
>>>>>>> <Dmitry.Samersoff at oracle.com> wrote:
>>>>>>>
>>>>>>>> Hi Everybody,
>>>>>>>>
>>>>>>>> Please review the fix
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8025812/webrev.01/
>>>>>>>>
>>>>>>>> The value of p_memsz filed of elf header of LOAD section inside
>>>>>>>> coredump
>>>>>>>> is rounded up to page size. So round up corresponding value read from
>>>>>>>> the header of library it self.
>>>>>>>>
>>>>>>>> --
>>>>>>>> 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.
>>>
>>
>>
>> --
>> Dmitry Samersoff
>> Oracle Java development team, Saint Petersburg, Russia
>> * I would love to change the world, but they won't give me the sources.
>