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

[Chris Plummer]

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.
>>>>