RFR (L): 8241807: JDWP needs update for hidden classes

Fri Apr 24 20:20:17 UTC 2020

Hi

I have several comments about coding and desing. These tests might be 
used as examples for new JDI tests based on this framework so I think it 
would be better to polish them.

General comment:

1. You often check results (not null, exactly one result).

It makes sense to use jdk.test.lib.Asserts for such verification. Just 
to reduce number of code and fail with standard exceptions.

2. Wildcard imports like 'import com.sun.jdi.*;' are not recommended.

3. I recommend to catch Throwable instead of Exception especially in 
non-main threads. Just to ensure that nothing is missed.

4. nsk.share.Failure is subclass of RuntimeException so you don't need 
to add it to throws. Now sometimes it is added, sometimes not. It looks 
inconsistent.

5. There are lot of code duplication in debugger and debugge classes. 
Does it make a sense to merge it into base classes?

http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassPrepareEvent/classPrepEvent001.java.html

6. The getField can't return null, it verifies result. So check in line 
191-193 is not needed.

  190         Field field = getField(hcRefType, "hcField");
  191         if (field == null) {
  192             throw new Failure("TEST FAILED: cannot enable ModificationWhatchpointRequest: hcField not found");
  193         }

7. I would recommend to move all initialization from run in constructor 
and make most of variable 'final' it helps to ensure that they are 
initialized.

  297     public int run(final String args[], final PrintStream out) {
  298         ArgumentHandler argHandler = new ArgumentHandler(args);
  299
  300         log = new Log(out, argHandler);
  301         eventTimeout = argHandler.getWaitTime() * 60 * 1000; // milliseconds
  302         launchDebuggee(argHandler);

8. Pair looks inconsistent. start saves handler as class field while 
join use as parameter.

305             startEventHandler();

330             joinEventHandler(eventHandler);

I think that something like

EventHandler eventHandler = startEventHandler();
joinEventHandler(eventHandler);

looks better and restrict usage of eventHandler. You also migt move 
methods startEventHandler into EventHandler itself.

9. I suggest to make setHCRefType private and rename getHCRefType to 
something like receiveHCRefType. To make more clear that it is not a 
simple getter, but method which wait for event.

  361         // Save hidden class ReferenceType when its ClassPrepare event is received.
  362         synchronized void setHCRefType(ReferenceType type) {
  363             hcRefType = type;
  364             notifyAll();
  365         }
  366
  367         // This method is used by the debugger main thread to wait and get
  368         // the hidden class reference type from its ClassPrepare event.
  369         // The readyCmdSync with the debuggeee is not enough because a
  370         // ClassPrepare event sent over JDWP protocol has some delay.
  371         // A wait/notify sync is to ensure the debugger gets non-null value.
  372         synchronized ReferenceType getHCRefType() {
  373             try {
  374                 while (hcRefType == null) {
  375                     wait();
  376                 }
  377             } catch (InterruptedException ie) {
  378                 throw new Failure("Unexpected InterruptedException in waiting for HC prepare: " + ie);
  379             }
  380             return hcRefType;
  381         }

10. We don't run tests with -ea/esa so assertions usually disabled. Use 
Assertions from test lib instead. The comment 1) is recommendation only 
but 'assert' is just ignored.

  436             assert name.indexOf("HiddenClass") > 0 && name.indexOf("/0x") > 0;

http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassPrepareEvent/classPrepEvent001a.java.html

11. Check if jdk.test.lib.classloader.ClassLoadUtils. 
getClassPath(String className) works for you instead of

   70     private static String getClassPath(Class<?> klass) {

12. You could use try(log = new PrintStream("Debuggee.log")) {} here:

   82         log = new PrintStream("Debuggee.log");
..

   86         try {
..
   90         }
   91         log.close();

13. It is better to throw RuntimeException and don't add throws 
Exception in all methods.

95     private void syncWithDebugger(IOPipe pipe) throws Exception {

http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassUnloadEvent/classUnloadEvent001a.java.html

You could use single if for this.

  122         if (command == null) {
  123             throw new Exception("Failed: pipe.readln() returned null instead of COMMAND_QUIT");
  124         } else if (!command.equals(classUnloadEvent001.COMMAND_QUIT)) {
  125             throw new Exception("Failed COMMAND_QUIT sync with debugger, got unknown command: " + command);
  126         }

14. Setting to null is not required to collect objects here.

151         hcObj = null;
152         hc = null;

Probably I didn't catch all issues, but let discuss this list first.

Leonid

On 4/23/20 9:01 PM, serguei.spitsyn at oracle.com wrote:
> Please, review a fix for the sub-task:
> https://bugs.openjdk.java.net/browse/JDK-8241807
>
> Webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-events.4/
>
>
> Summary;
>   This sub-task is to cover any hidden class related remaining issues 
> in the JDI and JDWP implementation.
>   In fact, no more issues were discovered with this extra test coverage.
>   So, this update includes only two new nsk jdi tests:
> || test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassPrepareEvent
> || test/hotspot/jtreg/vmTestbase/nsk/jdi/HiddenClass/ClassUnloadEvent
>
>   These tests are to provide a test coverage  which was impossible to 
> provide with the jdb testing framework.
>   It includes ClassPrepare, Breakpoint and ModificationWatchpoint 
> events with class filters.
>
> Testing:
>   The mach5 job is in progress:
> https://mach5.us.oracle.com/mdash/jobs/sspitsyn-HiddenClass-jdi-event-tests-20200424-0350-10464032
>
> Thanks,
> Serguei
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200424/c4d6b059/attachment-0001.htm>