RFR(S/M): 8205419: [testbug] TestJmapCore failing without SA: introduce @requires vm.hasSA

[Jini George]

Hi Goetz,

Your changes look good, though I feel that even for the tests that use 
ClhsdbLauncher and which need an attach, an "@requires 
vm.hasSAandCanAttach" could be used, and the Platform.shouldSAAttach() 
calls can be removed from ClhsdbLauncher.

Some nits:

1. In VMProps.java,

servicability ==> serviceability (an extra 'e')

2. In Platform.java

reqires ==> requires

Thanks,
Jini (Not a (R)eviewer).

On 6/22/2018 1:50 PM, Lindenmaier, Goetz wrote:
> Hi,
> 
> Ok, so I now added
>    vm.hasSA and
>    vm.hasSAandCanAttach
> I use the first in the JMapCore tests, and in the tests that don't check shouldSAAttach()
> right at the beginning.
> Wherever the test is immediately ended after checking shouldSAAttach(), I
> removed shouldSAAttach(), and added vm.hasSAandCanAttach.  The others
> depend on  ClhsdbLauncher. Just checking vm.hasSA is on the safe side
> if runOnCore() is changed.
> http://cr.openjdk.java.net/~goetz/wr18/8205419-requiresHasSA/02/
> 
> This also makes the implementation of VMProps and VMPlatform more clear
> as requested by Chris.
> 
> Best regards,
>   Goetz.
> 
> 
>> -----Original Message-----
>> From: Jini George <jini.george at oracle.com>
>> Sent: Thursday, June 21, 2018 12:18 PM
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'Chris Plummer'
>> <chris.plummer at oracle.com>; serviceability-dev (serviceability-
>> dev at openjdk.java.net) <serviceability-dev at openjdk.java.net>
>> Subject: Re: RFR(S/M): 8205419: [testbug] TestJmapCore failing without SA:
>> introduce @requires vm.hasSA
>>
>> Hi Goetz,
>>
>> Tests like TestJmapCore which involve only corefile debugging by SA (and
>> not attaching to a live process) would only need to check for the
>> presence of SA, and would not need to check for ptrace related attach
>> permissions like what is done for Linux and OSX -- since super user
>> privileges are not required to debug corefiles. So a single @requires
>> catering to both these might not be desirable.
>>
>> Having said that, I realized that runOnCore() of ClhsdbLauncher
>> incorrectly checks for the ptrace related attach permissions. (Will file
>> an issue to correct this).
>>
>> An @requires instead of a shouldSAAttach() might help in avoiding fake
>> passes like those on OSX when not run as "root".
>> (https://bugs.openjdk.java.net/browse/JDK-8199700)
>>
>> Thanks,
>> Jini.
>>
>>
>>
>>
>> On 6/21/2018 1:52 AM, Lindenmaier, Goetz wrote:
>>> Hi Chris,
>>>
>>> Thanks for looking at this change.
>>>
>>>> Can't VMProps.vmHasSA() just return Platform.shouldSAAttach()? It
>> seems
>>>> you are unnecessarily replicating Platform.shouldSAAttach() logic.
>>> I think there are two things to distinguish:
>>>    - platforms that don't implement SA
>>>    - systems/setups where SA does not work.
>>> If both can be recognized when VMProps is evaluated, shouldSAAttach()
>>> is the one that is superfluous in the long run?!  If not, shouldSAAttach
>>> should only return those where it is necessary to check in the test.
>>> But this is probably too much brains in this case ��
>>>
>>>> If you are adding "@requires vm.hasSAandCanAttach" to a test, shouldn't
>>>> you remove the test's Platform.shouldSAAttach() check?
>>> Well I asked in my mail whether I should do that. So I take this as a yes.
>>> But it depends on whether it must be evaluated in the test.
>>>
>>>> Is there a reason not to take the more direct and established approach
>>>> of just adding the Platform.shouldSAAttach() check to TestJmapCore? It
>>>> would be a lot less disruptive.  I know the vm.hasSAandCanAttach
>>>> approach has advantages in not even attempting to compile and run the
>>>> test, but I'm not so sure those advantages outweigh the simplicity of
>>>> just adding a Platform.shouldSAAttach() check to one test. I can go
>>>> either way here. Just wondering if there are additional reasons I might
>>>> not be seeing.
>>> I like the @requires much more. It tells right where the test is described
>>> that it does not run with SA. The other is quite hidden in the test, e.g., in
>>> helper class ClhsdbLauncher.java.
>>> Also, it's more easy to remember (The implementor of
>>> TestJmapCore forgot it...).  And, last but not least, it seems best practice
>>> nowadays. There are lots of excludes for mac, they are also done by
>> @requires
>>> and not as a check using Platform at runtime.
>>>
>>>> Sorry, I don't have an answer to your VMProps evaluation question. You
>>>> might need to ask a wider audience than just svc.
>>> Whom should I ask? Hotspot-dev?
>>>
>>> Best regards,
>>>     Goetz.
>>>
>>>
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> On 6/20/18 6:49 AM, Lindenmaier, Goetz wrote:
>>>>> Hi,
>>>>>
>>>>> TestJmapCore is failing on aix because there jhsdb is not implemented.
>>>>> So far, such tests check at runtime Platform.shouldSAAttach() which is
>>>> missing
>>>>> in TestJmapCore.
>>>>>
>>>>> Instead, I implement @requires vm.hasSAandCanAttach.
>>>>> This makes jtreg skip the tests without compiling and starting them.
>>>>>
>>>>> I added the new property to all tests that check shouldSAAttach in jdk
>> and
>>>> hostpot test
>>>>> directories.
>>>>> This is the implementation of the new property, the rest is just
>> repetitive
>>>> checking it:
>>>>> http://cr.openjdk.java.net/~goetz/wr18/8205419-
>>>> requiresHasSA/01/test/jtreg-ext/requires/VMProps.java.udiff.html
>>>>> webrev:
>>>>> http://cr.openjdk.java.net/~goetz/wr18/8205419-requiresHasSA/01/
>>>>>
>>>>> Is it correct to assume the VMProps is evaluated on the same machine
>> and
>>>> with the same
>>>>> user as used for executing the test?  canPtraceAttachLinux directly
>>>> accesses the system.
>>>>>
>>>>> (Should I remove the checks for shouldSAAttach() from the changed
>> tests?)
>>>>>
>>>>> Best regards,
>>>>>      Goetz.
>>>>
>>>>
>>>