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

[Jini George]

Some of the tests were skipped on OSX due to stackwalking issues on OSX. 
(Some were pstack related, and some for jstack on corefiles).

Thanks,
Jini.

On 6/25/2018 10:31 PM, Chris Plummer wrote:
> Hi Goetz,
> 
> The changes look good. Just one question though. In places where you have:
> 
>    42  * @requires vm.hasSAandCanAttach & os.family != "mac"
> 
> Do you know why these tests don't run on osx? I just wonder if it 
> related to attach not working unless root, and that is now covered by 
> hasSAandCanAttach(). If that is the case, the "mac" check can be removed.
> 
> thanks,
> 
> Chris
> 
> On 6/22/18 1:20 AM, 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.
>>>>>
> 
>