RFR 8222379: JFR TestClassLoadEvent.java failed due to EXCEPTION_ACCESS_VIOLATION

[coleen.phillimore at oracle.com]

Thanks Jiangli!
Coleen

On 4/17/19 2:55 PM, Jiangli Zhou wrote:
> Looks good!
>
> Best,
> Jiangli
>
> On Wed, Apr 17, 2019 at 11:54 AM <coleen.phillimore at oracle.com 
> <mailto:coleen.phillimore at oracle.com>> wrote:
>
>
>
>     On 4/17/19 1:50 PM, Jiangli Zhou wrote:
>>     Hi Coleen,
>>
>>     Looks reasonable to me.
>>
>>     - src/hotspot/os/windows/os_windows.cpp
>>     5005   // There is a very small theoretical window between the unmap_memory()
>>     5006   // call above and the map_memory() call below where a thread in native
>>     5007   // code may be able to access an address that is no longer mapped.
>>     The comment refers to the 'unmap_memory() call above and the map_memory() call below'. Since the calls are removed, could you please update the comment as well.
>
>     I can update the comments.  How about:
>
>     // Remap a block of memory.
>     char* os::pd_remap_memory(int fd, const char* file_name, size_t
>     file_offset,
>                               char *addr, size_t bytes, bool read_only,
>                               bool allow_exec) {
>       // This OS does not allow existing memory maps to be remapped so we
>       // would have to unmap the memory before we remap it.
>
>       // Because there is a small window between unmapping memory and
>     mapping
>       // it in again with different protections, CDS archives are
>     mapped RW
>       // on windows, so this function isn't called.
>       ShouldNotReachHere();
>       return NULL;
>     }
>
>
>>     - src/hotspot/share/memory/filemap.cpp
>>     853 // If a tool agent is in use (debugging enabled), or JFR, we
>>     must map the address space RW
>>     854 if (JvmtiExport::can_modify_any_class() ||
>>     JvmtiExport::can_walk_any_space() ||
>>     855 Arguments::has_jfr_option()) {
>>       856     si->_read_only = false;
>>       857   }
>>     858 #ifdef _WINDOWS
>>     859 // Windows cannot remap read-only shared memory to read-write
>>     when required for
>>     860 // RedefineClasses, which is also used by JFR. Always map
>>     windows regions as RW.
>>     861 si->_read_only = false;
>>     862 #endif
>>     To avoid setting si->_read_only twice on Windows, maybe putting the code at line #853-#857 under #else after #ifdef _WINDOWS?
>
>     Okay, that's an improvement too.
>

>     Thanks Jiangli!
>     Coleen
>
>>     Best regards,
>>     Jiangli
>>
>>     On Wed, Apr 17, 2019 at 5:25 AM <coleen.phillimore at oracle.com
>>     <mailto:coleen.phillimore at oracle.com>> wrote:
>>
>>         Summary: Give fatal error if CDS loses archive mapping; but
>>         map Windows
>>         RW because remapping is dangerous.
>>
>>         Ioi and I discussed this change and thought it is best. 
>>         Windows only
>>         maps the CDS archive around 50% time because of ASLR and this
>>         retains
>>         the startup performance improvements for CDS on windows.
>>
>>         Tested with mach5 tier1-3.
>>
>>         open webrev at
>>         http://cr.openjdk.java.net/~coleenp/2019/8222379.01/webrev
>>         bug link https://bugs.openjdk.java.net/browse/JDK-8222379
>>
>>         Thanks,
>>         Coleen
>>
>