RFR 8222379: JFR TestClassLoadEvent.java failed due to EXCEPTION_ACCESS_VIOLATION

[Jiangli Zhou]

Looks good!

Best,
Jiangli

On Wed, Apr 17, 2019 at 11:54 AM <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> 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
>>
>>
>