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.geo...@oracle.com> > Sent: Thursday, June 21, 2018 12:18 PM > To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; 'Chris Plummer' > <chris.plum...@oracle.com>; serviceability-dev (serviceability- > d...@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. > >> > >> > >
