[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: RE: RFR(S/M): 8205419: [testbug] TestJmapCore failing without SA: introduce @requires vm.hasSA
From: "Lindenmaier, Goetz" <goetz.lindenmaier () sap ! com>
Date: 2018-06-25 21:04:14
Message-ID: 6af2ac5b2a9d4d179934eb2c10ef7cbf () sap ! com
[Download RAW message or body]
Hi,
Thanks for reviewing!
Yes, what Jini is saying is also what I found in the comments.
Best regards,
Goetz.
> -----Original Message-----
> From: Chris Plummer <chris.plummer@oracle.com>
> Sent: Monday, June 25, 2018 7:02 PM
> To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; 'Jini George'
> <jini.george@oracle.com>; serviceability-dev (serviceability-
> dev@openjdk.java.net) <serviceability-dev@openjdk.java.net>
> Subject: Re: RFR(S/M): 8205419: [testbug] TestJmapCore failing without SA:
> introduce @requires vm.hasSA
>
> 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@oracle.com>
> >> Sent: Thursday, June 21, 2018 12:18 PM
> >> To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; 'Chris Plummer'
> >> <chris.plummer@oracle.com>; serviceability-dev (serviceability-
> >> dev@openjdk.java.net) <serviceability-dev@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.
> >>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic