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, [email protected] 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
