Thank you for review. Filed https://bugs.openjdk.java.net/browse/JDK-8242009 
<https://bugs.openjdk.java.net/browse/JDK-8242009> for issue with passing 
arguments into debugger/tools processes.

Leonid


> On Apr 1, 2020, at 11:21 AM, Chris Plummer <chris.plum...@oracle.com> wrote:
> 
> Hi Leonid,
> 
> Looks good. Thanks for cleaning this up!
> 
> Chris
> 
> On 3/31/20 4:12 PM, Leonid Mesnik wrote:
>> Here is new webrev:
>> http://cr.openjdk.java.net/~lmesnik/8240698/webrev.03/ 
>> <http://cr.openjdk.java.net/~lmesnik/8240698/webrev.03/>
>> 
>> The only difference is updated startApp() method and it's comments:
>> http://cr.openjdk.java.net/~lmesnik/8240698/webrev.03/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
>>  
>> <http://cr.openjdk.java.net/~lmesnik/8240698/webrev.03/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html>
>> 
>> Leonid
>> 
>>> On Mar 31, 2020, at 1:32 PM, Chris Plummer <chris.plum...@oracle.com 
>>> <mailto:chris.plum...@oracle.com>> wrote:
>>> 
>>> On 3/31/20 12:09 PM, Leonid Mesnik wrote:
>>>> Hi
>>>> 
>>>> On 3/30/20 9:43 PM, Chris Plummer wrote:
>>>>> Hi Leonid,
>>>>> 
>>>>> On 3/30/20 5:42 PM, Leonid Mesnik wrote:
>>>>>> Hi
>>>>>> 
>>>>>> See my comments inline. I will update webrev after go through all your 
>>>>>> comments.
>>>>>> 
>>>>>> 
>>>>>> On 3/30/20 11:39 AM, Chris Plummer wrote:
>>>>>>> Hi Leonid,
>>>>>>> 
>>>>>>> I haven't gone through all the tests yet.  I've accumulated enough 
>>>>>>> questions that I'd like to see them answered or addressed before I 
>>>>>>> continue on.
>>>>>>> 
>>>>>>> This isn't directly related to your changes, but I noticed that users 
>>>>>>> of JDKToolLauncher do nothing to make sure that default test options 
>>>>>>> are used. This means we are never running these tools with the test 
>>>>>>> options being specified with the jtreg run. Is that a bug or 
>>>>>>> intentional?
>>>>>> 
>>>>>> Which "default test options" do you mean? We have 2 properties to set 
>>>>>> JVM options. The idea is to pass test.vm.opts to ALL java processes and 
>>>>>> test.java.opts  to only tested processes if applicable. Usually, for 
>>>>>> example we don't want to run jcmd with -Xcomp. test.vm.opts was used (a 
>>>>>> long time ago) for options like '-d32/-d64' on Solaris where JVM don't 
>>>>>> start without choosing correct version. Also, it is used to reduce 
>>>>>> maximum heap for all JVM instances when tests are running concurrently.
>>>>>> 
>>>>>> So, probably test.vm.opts (or test.vm.tools.opts) should be added by 
>>>>>> JDKToolLauncher but not test.java.opts. It is separate topic, there are 
>>>>>> a lot of launchers which ignore test.vm.opts now.
>>>>> I always get confused about which set of options these properties 
>>>>> represent, but basically I'm suggesting that if for example we are doing 
>>>>> a -Xcomp run in mach5, JDKToolLauncher (at least in some cases) should be 
>>>>> launched with this option. I think this is what you get from 
>>>>> Utils.getTestJavaOpts(),.
>>>>> 
>>>>> For example the SA tests use JDKToolLauncher.createUsingTestJDK("jhsdb"). 
>>>>> jhsdb is what is really being tested here, and it should be launched with 
>>>>> the test vm options. Currently we launch the target process with these 
>>>>> options, which is probably also a good idea.  Also we aren't too 
>>>>> concerned with the options that the test itself is run with, although I'm 
>>>>> guessing they also get run with the test java opts. So we have 3 
>>>>> processes here:
>>>>>  - jhsdb, which should be getting test java opts but is not
>>>>>  - the target process, which should be getting test java opts and 
>>>>> currently is
>>>>>  - the test itself, where options don't really matter, but is getting 
>>>>> passed test java opts
>>>>> 
>>>>> However, you could argue that tests like jinfo, jstack, and jcmd, all of 
>>>>> which use the Attach API and the bulk of the work is done on the target 
>>>>> process, are not that concerned with the options passed to the command, 
>>>>> but do want the options passed to the target process.
>>>> 
>>>> Well, it is a good question if we want to run jhsdb tool itself with 
>>>> additional slow options like Xcomp. Does it help us to improve coverage? 
>>>> IIRC the original idea of adding test.java/vm.opts was to don't waste time 
>>>> executing javac and debuggers in slow mode on SPARC.
>>>> 
>>>> Anyway, it is a separate question which is out of scope of this change. We 
>>>> might want to review all debugger/debugee tests to find better way to deal 
>>>> with this.
>>> Might be good to get an RFE filed for this.
>>>> 
>>>>>> 
>>>>>>> 
>>>>>>> In the problem lists, is it necessary to list the test multiple times 
>>>>>>> with #id0, #id1, etc, or could you list it just once and leave that 
>>>>>>> part off. It seems very error prone. Also, changing tests like 
>>>>>>> ClhsdbFindPC, ClhsdbJstack, and ClhsdbScanOops to split out the testing 
>>>>>>> in this manner seems completely unrelated to this CR, especially when 
>>>>>>> the tests do not even contain any changes related to the CR.
>>>>>> 
>>>>>> I think, that these chages are related. The startApp(...) was updated so 
>>>>>> some test combinations become invalid or redundant.
>>>>>> 
>>>>>> ClhsdbFindPC and ClhsdbJstack were always run twice. Now, when test 
>>>>>> options passed in test it is not needed to run it twice when Xcomp is 
>>>>>> already set by user.
>>>>>> 
>>>>> Ok. I see now that the second test run, which is the non -Xcomp run, adds 
>>>>> '@requires vm.compMode != "Xcomp"'. But this also is strange. The first 
>>>>> test run, which does not have the @requires and is the one that makes 
>>>>> LingeredApp launch with -Xcomp, will always run whether or not it is an 
>>>>> -Xcomp test run. So it will run as part of the a regular test run and as 
>>>>> part of a -Xcomp test run. The only difference between the two is the 
>>>>> -Xcomp run will also run the test with -Xcomp, but that's not really 
>>>>> needed (I think it will also end up passing -Xcomp to the target processs 
>>>>> twice). Perhaps '@requires vm.compMode == "Xcomp"' should be used for the 
>>>>> first test run, but that means it no longer gets run until later tiers 
>>>>> when we use -Xcomp. Why not revert it back to a single test, but also add 
>>>>> '@requires vm.compMode != "Xcomp"'. Then it gets run both ways in an 
>>>>> early tier and not run during the -Xcomp run, which isn't really needed.
>>>> 
>>>> There several flag which are executed with Xcomp only: 
>>>> "-XX:-DoEscapeAnalysis",  "-XX:-UseBiasedLocking", "-XX:+DeoptimizeALot" 
>>>> where this test is going to be skipped. So we never run test with these 
>>>> options.
>>>> 
>>>> The original idea is to run test with given options and with added Xcomp.  
>>>> I left logic the same and only skip run with "Xcomp" when it is set 
>>>> already by user. I agree that we have some duplication here and it could 
>>>> be improved, but it could be done separately. If you are ok with this let 
>>>> me file separate RFE for this.
>>> Ok.
>>>> 
>>>>> 
>>>>>> ClhsdbScanOops is fixed to don't allow to run incompatible GC 
>>>>>> combination.
>>>>> Ok
>>>>>> 
>>>>>> So I should update these tests by splitting them or change them to  
>>>>>> startAppExactJvmOpts() if we wan't continue to ignore user-given test 
>>>>>> options.
>>>>> I don't think I was suggesting removing user-given test options. I don't 
>>>>> see why you would.
>>>> 
>>>> I just wanted to say that these tests are affected by my changes and 
>>>> should be fixed anyway.
>>> Ok.
>>> 
>>> So I think the one change you agreed to make is have the default be to 
>>> append test vm opts rather than prepend them. Let me know when you have a 
>>> new webrev.
>>> 
>>> thanks,
>>> 
>>> Chris
>>>> 
>>>> Leonid
>>>> 
>>>>>> 
>>>>>> It seems that #idN are required by jtreg now, otherwise it just run test.
>>>>> Ok.
>>>>>> 
>>>>>>> 
>>>>>>>  426     public static LingeredApp startApp(String... 
>>>>>>> additionalJvmOpts) throws IOException {
>>>>>>> 
>>>>>>> The default test opts are appended to additionalJvmOpts, and if you 
>>>>>>> want prepended you need to call Utils.prependTestJavaOpts(). I would 
>>>>>>> have thought the opposite would be more desirable and expected default 
>>>>>>> behavior. Why did you choose this way? I also find it somewhat 
>>>>>>> confusing that there is even a default mode for where the 
>>>>>>> additionalJvmOpts go. Maybe it would be best to have 
>>>>>>> startAppAppendJvmArgs() and startAppPrependJvmArgs() just to make it 
>>>>>>> explicit. This would also be in line with the existing 
>>>>>>> startAppExactJvmOpts().
>>>>>>> 
>>>>>> I've chosen the most popular usage, which was Utils.appendTestJavaOpts. 
>>>>>> But I agree, that it would be better to change it to prepend. Thanks for 
>>>>>> pointing to this.
>>>>>> 
>>>>>> I don't want to add startAppAppendJvmArgs()/startAppPrependJvmArgs() to 
>>>>>> don't complicate all things. I think that startApp() should be used in 
>>>>>> the cases when test vm options really shouldn't interfere with 
>>>>>> user-provided options or overwrite them. So basically the behavior is 
>>>>>> the same as for ProcessTools.createJavaProcessBuilder(true, ...) and 
>>>>>> jtreg itself.
>>>>>> 
>>>>> Ok.
>>>>>> 
>>>>>>> Is ClhsdbFindPC correct. It used to use just use -Xcomp or -Xint, 
>>>>>>> ignoring any default test opts. You've fixed it to include the default 
>>>>>>> test opts, but the are appended, possibly overriding the -Xcomp or 
>>>>>>> -Xint. Don't we want the default test opts prepended? Same for 
>>>>>>> ClhsdbJstack.
>>>>>> 
>>>>>> The idea is to don't mix Xcomp and Xmixed/Xint using requires filter. 
>>>>>> However ClhsdbFindPC might override Xint with Xmixed if it is set 
>>>>>> explicitly. Switching to prepending will fix it.
>>>>> Yes, that's what I was thinking and one reason I thought that should be 
>>>>> default behavior.
>>>>> 
>>>>> thanks,
>>>>> 
>>>>> Chris
>>>>>> 
>>>>>> Leonid
>>>>>> 
>>>>>>> 
>>>>>>> thanks,
>>>>>>> 
>>>>>>> Chris
>>>>>>> 
>>>>>>> On 3/25/20 2:31 PM, Leonid Mesnik wrote:
>>>>>>>> 
>>>>>>>> Igor, Stefan, Ioi
>>>>>>>> 
>>>>>>>> Thank you for your feedback.
>>>>>>>> 
>>>>>>>> Filed https://bugs.openjdk.java.net/browse/JDK-8241624 
>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8241624> To change @run 
>>>>>>>> main... to @run driver.
>>>>>>>> 
>>>>>>>> Test ClhsdbJstack.java is updated.
>>>>>>>> 
>>>>>>>> Still waiting for review from SVC team.
>>>>>>>> 
>>>>>>>> webrev: http://cr.openjdk.java.net/~lmesnik/8240698/webrev.02/ 
>>>>>>>> <http://cr.openjdk.java.net/~lmesnik/8240698/webrev.02/>
>>>>>>>> 
>>>>>>>> Leonid
>>>>>>>> 
>>>>>>>> On 3/25/20 12:46 PM, Igor Ignatyev wrote:
>>>>>>>>> Hi Leonid,
>>>>>>>>> 
>>>>>>>>> not related related to your patch (but yet somewhat made more obvious 
>>>>>>>>> by it), it seems all (or at least almost all) the tests which 
>>>>>>>>> use�LingeredApp should be run in "driver" mode as they just 
>>>>>>>>> orchestrate execution of other JVMs, so running them w/ main (let 
>>>>>>>>> alone main/othervm) just wastes time, 
>>>>>>>>> test/hotspot/jtreg/serviceability/sa/ClhsdbJstack.java#id1, for 
>>>>>>>>> example, will now executed w/ Xcomp which will make it very slow for 
>>>>>>>>> no reasons. since you already got your hands dirty w/ these tests, 
>>>>>>>>> could you please file an RFE to sort this out and list all the 
>>>>>>>>> affected tests there?
>>>>>>>>> 
>>>>>>>>> re: the patch, could you please update ClhsdbJstack.java test not to 
>>>>>>>>> be run w/ Xcomp and follow the same pattern you used in other tests 
>>>>>>>>> (e.g.�ClhsdbScanOops) ? other than that it looks fine to me, I 
>>>>>>>>> however wouldn't be able to tell if all svc tests continue to do that 
>>>>>>>>> they were supposed to, so I'd prefer for someone from svc team 
>>>>>>>>> to�chime in.
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> -- Igor
>>>>>>>>> 
>>>>>>>>>> On Mar 25, 2020, at 12:01 PM, Leonid Mesnik 
>>>>>>>>>> <leonid.mes...@oracle.com <mailto:leonid.mes...@oracle.com> 
>>>>>>>>>> <mailto:leonid.mes...@oracle.com <mailto:leonid.mes...@oracle.com>>> 
>>>>>>>>>> wrote:
>>>>>>>>>> 
>>>>>>>>>> Added Ioi, who also proposed new version of startAppVmOpts.
>>>>>>>>>> 
>>>>>>>>>> Please find new webrev: 
>>>>>>>>>> http://cr.openjdk.java.net/~lmesnik/8240698/webrev.01/ 
>>>>>>>>>> <http://cr.openjdk.java.net/~lmesnik/8240698/webrev.01/>
>>>>>>>>>> 
>>>>>>>>>> Renamed startAppVmOpts/runAppVmOpts to 
>>>>>>>>>> "startAppExactJvmOpts/runAppExactJvmOpts" is used. It should make 
>>>>>>>>>> very clear that this method doesn't use any of test.java.opts, 
>>>>>>>>>> test.vm.opts.
>>>>>>>>>> 
>>>>>>>>>> Also, I fixed test/hotspot/jtreg/serviceability/sa/ClhsdbFlags.java 
>>>>>>>>>> metnioned by Igor, and removed null pointer check as Ioi suggested 
>>>>>>>>>> in startApp method.
>>>>>>>>>> 
>>>>>>>>>> + public static void startApp(LingeredApp theApp, String... 
>>>>>>>>>> additionalJvmOpts) throws IOException {
>>>>>>>>>> + startAppExactJvmOpts(theApp, 
>>>>>>>>>> Utils.appendTestJavaOpts(additionalJvmOpts));
>>>>>>>>>> + }
>>>>>>>>>> 
>>>>>>>>>> Leonid
>>>>>>>>>> 
>>>>>>>>>> On 3/25/20 10:14 AM, Stefan Karlsson wrote:
>>>>>>>>>>> On 2020-03-25 17:40, Igor Ignatyev wrote:
>>>>>>>>>>>> Hi Leonid,
>>>>>>>>>>>> 
>>>>>>>>>>>> I have briefly looked at the patch, a few comments so far:
>>>>>>>>>>>> 
>>>>>>>>>>>> test/hotspot/jtreg/serviceability/sa/ClhsdbFlags.java:
>>>>>>>>>>>> � - at L#114, could you please call static method using class name 
>>>>>>>>>>>> (as the opposite of using instance)? or was it meant to be 
>>>>>>>>>>>> theApp.runAppVmOpts(vmArgs) ?
>>>>>>>>>>>> 
>>>>>>>>>>>> test/lib/jdk/test/lib/apps/LingeredApp.java:
>>>>>>>>>>>> - it seems that code indent of startApp(LingeredApp, String[]) 
>>>>>>>>>>>> isn't correct
>>>>>>>>>>>> - I don't like startAppVmOpts name, but unfortunately don't have a 
>>>>>>>>>>>> better suggestion (yet)
>>>>>>>>>>> 
>>>>>>>>>>> I was going to say the same. Jtreg has the concept of "java 
>>>>>>>>>>> options" and "vm options". We have had a fair share of bugs and 
>>>>>>>>>>> wasted time when tests have been using the "vm options" part 
>>>>>>>>>>> (VM_OPTIONS, test.vm.options, etc), and we've been moving away from 
>>>>>>>>>>> using that way to pass options. I recently cleaned up some of this 
>>>>>>>>>>> with:
>>>>>>>>>>> 
>>>>>>>>>>> 8237111: LingeredApp should be started with getTestJavaOpts
>>>>>>>>>>> 
>>>>>>>>>>> Because of this, I would prefer if we used a name that doesn't 
>>>>>>>>>>> include "VmOpts", because it's too alike the other concept. Some 
>>>>>>>>>>> suggestions:
>>>>>>>>>>> �startAppJavaOptions
>>>>>>>>>>> �startAppUsingJavaOptions
>>>>>>>>>>> �startAppWithJavaOptions
>>>>>>>>>>> �startAppExactJavaOptions
>>>>>>>>>>> �startAppJvmOptions
>>>>>>>>>>> 
>>>>>>>>>>> Thanks,
>>>>>>>>>>> StefanK
>>>>>>>>>>> 
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> -- Igor
>>>>>>>>>>>> 
>>>>>>>>>>>>> On Mar 25, 2020, at 8:55 AM, Leonid Mesnik 
>>>>>>>>>>>>> <leonid.mes...@oracle.com <mailto:leonid.mes...@oracle.com>> 
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Hi
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Could you please review following fix which change LingeredApp to 
>>>>>>>>>>>>> prepend vm options to java/vm.test.opts when startApp is used and 
>>>>>>>>>>>>> provide startAppVmOpts to override options completely.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> The intention is to avoid issue like in this bug where test/jtreg 
>>>>>>>>>>>>> options were ignored by tests. Also I fixed some tests where 
>>>>>>>>>>>>> intention was to append vm options rather than to override them.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> webrev: http://cr.openjdk.java.net/~lmesnik/8240698/webrev.00/ 
>>>>>>>>>>>>> <http://cr.openjdk.java.net/~lmesnik/8240698/webrev.00/>
>>>>>>>>>>>>> 
>>>>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8240698 
>>>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8240698>
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Leonid
>> 
> 

Reply via email to