RFR: (S): JDK-8200613: SA: jstack throws UnmappedAddressException with a CDS core file

[Jini George]

I have the revised webrev here:

http://cr.openjdk.java.net/~jgeorge/8200613/webrev.02/index.html

The extra changes here are to:

* Introduce a new option DumpPrivateMappingsInCore to control the 
dumping of the file backed private regions into the corefile.

* Close the modules file before dumping core in os::abort(). Currently, 
there is a small bug (https://bugs.openjdk.java.net/browse/JDK-8215026) 
which prevents the closure of the image file in unmapping the entire file.

I plan to take up the unmapping of NIO MapMode.PRIVATE files as a 
separate task (https://bugs.openjdk.java.net/browse/JDK-8215027) since 
this seems a bit involved.

Thanks a bunch,
Jini.

On 11/12/2018 10:26 AM, Jini George wrote:
> Thank you very much, Chris, Kevin and Ioi for your comments!
> 
> I will send another webrev with this change enabled under an opt-out 
> flag, as you suggest, and would look at unmapping the JDK modules file 
> and if possible, the NIO mapped files too in the signal handler.
> 
> Thanks a bunch,
> Jini.
> 
> On 11/9/2018 11:53 PM, Ioi Lam wrote:
>> Hi Jini,
>>
>> Thanks for investigating the size expansion issue.
>>
>> I agree that the size increase is worth it. Even when not using SA, if 
>> we open the core file inside GDB, we cannot read certain sections in 
>> the CDS archive (such as the RO section and strings sections). That 
>> would make debugging difficult. So I am in favor of this change.
>>
>> For the JDK modules file, maybe we can unmap it in the signal handler, 
>> before going ahead with the core dump? I think it's hardly needed for 
>> debugging purposes. (Perhaps we can also do the same for the NIO 
>> mapped files?)
>>
>> A opt-flag as suggested by Kevin is a good idea.
>>
>> Thanks
>>
>> - Ioi
>>
>> On 11/9/18 3:29 AM, Kevin Walls wrote:
>>> Hi Jini,
>>>
>>> Looks good to me.  It might be a significant increase in size of 
>>> _some_ core files, but so many core files we see are much larger, in 
>>> gigabytes++ of course, so the CDS data size should not be such a 
>>> significant increase on (I think) most files.
>>>
>>> The flexibiity of always having the CDS data there is very 
>>> significant.  A core file should ideally be usable, without 
>>> additionally requiring the CDS archive from the machine.  That 
>>> additional human round-trip upload request on every transmitted core 
>>> that needs investigating, seems like a less efficient route...).
>>>
>>> Is there an opt-out?  It's conditional on UseSharedSpaces but could 
>>> there be a flag to disable, in case we see crashes with gigabytes of 
>>> private mappings that we really don't want to retain (the user would 
>>> have to know to set a flag, to disable the new coredump filter ahead 
>>> of time).
>>>
>>> Thanks!
>>> Kevin
>>>
>>>
>>> On 29/10/2018 06:02, Jini George wrote:
>>>> Thank you very much, Ioi, for looking into this, and the 
>>>> clarification offline. My bad, I had missed the earlier mail from 
>>>> you. :-( My responses below.
>>>>
>>>> Yes, I had tested this on MacOS. The issue does not exist on MacOS 
>>>> since the file backed private mmap()-ed regions get dumped into the 
>>>> MacOS corefiles by default.
>>>>
>>>> The corefile sizes on Linux do increase due to this change. And the 
>>>> increase would also include any file mapped using NIO with 
>>>> MapMode.PRIVATE. The typical corefile size increase with this change 
>>>> would include the following components at a high level:
>>>>
>>>> * Any NIO file mapping with MapMode.PRIVATE.
>>>> * Any file mmap()-ed by any native library with MAP_PRIVATE.
>>>> * The read only CDS regions (ro and od): Of the order of a few MB.
>>>> * The shared strings CDS region. (typically less than 1 MB).
>>>> * 2 MB per native shared library (regions with ---p permissions 
>>>> mapped by the dynamic linker for better alignment and for keeping 
>>>> libraries efficiently shareable).
>>>> * The JDK 'modules' file. (About 140 MB).
>>>>
>>>> So, without including any NIO mapping, I typically see around 
>>>> 250-300 MB increase in the corefile sizes. I agree that the size 
>>>> increase could be a cause for concern, but for FWIW, these privately 
>>>> mapped files get dumped into the corefile for MacOS too. And the 
>>>> corefile sizes for the same program on MacOS are way larger (of the 
>>>> order of a few GB as against about 300 MB on Linux (without the 
>>>> change)).
>>>>
>>>> The advantage of fixing this by modifying the coredump_filter v/s 
>>>> doing it in SA (by reading in more sections of the shared archive 
>>>> file) is that this would benefit other debuggers like gdb also. (And 
>>>> reduces the dependence on having the shared archive file being 
>>>> available at the time of debugging). If folks still think this is a 
>>>> cause for concern, I could make modifications to fix this by reading 
>>>> in the regions from the shared archive file in the SA code. I also 
>>>> wonder if it is worth taking a relook at the mapping types of the 
>>>> various CDS regions also.
>>>>
>>>> Thank you,
>>>> Jini.
>>>>
>>>> On 10/22/2018 10:27 AM, Ioi Lam wrote:
>>>>> Hi Jini,
>>>>>
>>>>> Did you see my earlier reply? I might have sent it out during the 
>>>>> mail server outage days :-(
>>>>>
>>>>> http://openjdk.5641.n7.nabble.com/RFR-S-JDK-8200613-SA-jstack-throws-UnmappedAddressException-with-a-CDS-core-file-td352681.html#a353026 
>>>>>
>>>>>
>>>>> Here was my reply again:
>>>>>
>>>>>> Hi Jini,
>>>>>>
>>>>>> The changes looks good to me.
>>>>>>
>>>>>> Have you tested this on MacOS? CDS heap support is also enabled on
>>>>>> MacOS. See macros.hpp:
>>>>>>
>>>>>>      #if INCLUDE_CDS && INCLUDE_G1GC && defined(_LP64) && 
>>>>>> !defined(_WINDOWS)
>>>>>>      #define INCLUDE_CDS_JAVA_HEAP 1
>>>>>>
>>>>>> Also, besides CDS, do we know how often other files will be mmaped 
>>>>>> with
>>>>>> MAP_PRIVATE? Will users get huge core files because CDS is 
>>>>>> enabled? In
>>>>>> JDK 12, CDS will be enabled by default (see JDK-8202951), so all 
>>>>>> users
>>>>>> will be affected by the following line:
>>>>>>
>>>>>>    if (UseSharedSpaces) {
>>>>>>      set_coredump_filter(FILE_BACKED_PVT_BIT);
>>>>>>    }
>>>>>>
>>>>>> Maybe you can run an big app such as Eclipse, trigger a core dump, 
>>>>>> and
>>>>>> compare the size of the core file before/after this change?
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> - Ioi 
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>> - Ioi
>>>>>
>>>>>
>>>>> On 10/21/18 8:58 PM, Jini George wrote:
>>>>>> Gentle reminder!
>>>>>>
>>>>>> Thanks,
>>>>>> - Jini
>>>>>>
>>>>>> On 10/9/2018 11:31 AM, Jini George wrote:
>>>>>>> Hello!
>>>>>>>
>>>>>>> [Including runtime-dev since the changes are in runtime code]
>>>>>>>
>>>>>>> Requesting reviews for:
>>>>>>>
>>>>>>> Webrev: http://cr.openjdk.java.net/~jgeorge/8200613/webrev.01/
>>>>>>> BugID: https://bugs.openjdk.java.net/browse/JDK-8200613
>>>>>>>
>>>>>>> Issue: jhsdb jstack would throw an UnmappedAddressException with 
>>>>>>> a core file generated from a CDS enabled java process. This is 
>>>>>>> seen only with Linux and with G1GC, while trying to read in data 
>>>>>>> from the shared strings region (the closed archive heap space). 
>>>>>>> This region (which is a file backed private memory region) is not 
>>>>>>> dumped into the corefile for Linux. This, being a heap region 
>>>>>>> (and therefore being a read-write region) is also not read in 
>>>>>>> from the classes.jsa file in SA since only the read only regions 
>>>>>>> are read in while processing the core file. (The expectation 
>>>>>>> being that the read write regions are in the core file).
>>>>>>>
>>>>>>> Proposed solution: The proposed solution is to have the 
>>>>>>> coredump_filter value corresponding to the CDS process to include 
>>>>>>> bit 2 (file-backed private memory), so that the file-backed 
>>>>>>> private memory region also gets dumped into the corefile. The 
>>>>>>> proposed fix is in src/hotspot/os/linux/os_linux.cpp.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jini.
>>>>>>>
>>>>>
>>>
>>