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'

[Staffan Larsen]

Thanks - good to go!

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.