RFR: 8224221: add memprotect calls to event log

[David Holmes]

On 23/05/2019 12:54 am, Baesken, Matthias wrote:
> Hi David ,
> I added the  missing bsd-related  Events::log  calls .

Thanks.

> Additionally I added calls  to
>      bool os::guard_memory(char* addr, size_t bytes)
> and
>      bool os::unguard_memory(char* addr, size_t bytes)
> 
> on Windows .

This is starting to look a bit inconsistent. If you want to log 
VirtualProtect calls then there are more of them than just in 
guard_memory and unguard_memory. So I'm unclear on the intent here - is 
it to log all "memory protection" actions or only to log those that 
pertain to specific semantic actions? On linux we have a split between 
use of mprotect and use of mmap and this change only logs mprotect on 
Linux; but those same semantics actions e.g. pd_commit_memory, on 
Windows use VirtualProtect.

> Regarding the  event log initialization -   I think it is better to keep  the init where it is for now ,   moving  it in front of other inits just  leads to asserts and crashes
>    so I think it is out of scope of the CR  to redo the whole init process .

I'm not asking you to do that. I'm asking you to determine what init 
dependencies your change actually has. If you keep the init where it is 
you may miss logging some early mprotect usages (of course there's no 
guarantee you can log them all if the init dependencies can't be resolved)

David
-----

> New webrev :
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8224221.3/
> 
> 
> Best regards, Matthias
> 
> 
> 
>> Hi Matthias,
>>
>> On 21/05/2019 9:24 pm, Baesken, Matthias wrote:
>>> Hello David,  thanks for your comments.
>>>
>>>    I see no  "flooding"  of the  event log, of course a few more  events  occur
>> but not just mprotect events are seen .
>>
>> That's good to know.
>>
>>> Regarding the  event   init -  routines :
>>> I had a bit of trouble with the order of inits + usages  , so I changed back  to
>> the  original  coding ;
>>>     then I had to add  checks to events.hpp / cpp   so that  I do not log  to a
>> yet uninitialized  event infrastructure in early startup but I  think the added
>> checks are good to have .
>>
>> It should be easy enough to determine where the first mprotect call
>> happens and so determine where in the init sequence event_init() needs
>> to go. Now it still may be that it can't go there because of other
>> initialization dependencies, and so your initialization guards are still
>> needed, but I'd rather we understood the initialization dependencies fully.
>>
>>> I adjusted  the "small nit" regarding output - good point .
>>
>> Thanks for fixing.
>>
>>> Regarding the missing logs in os_bsd.cpp  you mentioned  :
>>> I think they are bsd only and I have no machine here to test  - should I still
>> add them and hope that it works ?
>>
>> Yes please. To me it's better to find out later that there is a problem
>> than to omit it, as there is no way to recognize that it has in fact
>> been omitted.
>>
>