RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent

[Chris Plummer]

Hi Magus,

I haven't looked at the changes yet, other to see that there are many 
files touched, but after reading below (and only partly understanding 
since I don't know this area well), I was wondering if this issue 
wouldn't be better served with multiple passes made to fix the warnings. 
Start with a straight forward one where you are maybe only making one or 
two types of changes, but that affect a large number of files and don't 
cascade into other more complicated changes. This will get a lot of the 
noise out of the way, and then we can focus on some of the harder issues 
you bring up below.

As for testing, I think the following list will capture all of them, but 
can't say for sure:

open/test/hotspot/jtreg/serviceability/sa
open/test/hotspot/jtreg/resourcehogs/serviceability/sa
open/test/jdk/sun/tools/jhsdb
open/test/jdk/sun/tools/jstack
open/test/jdk/sun/tools/jmap
open/test/hotspot/jtreg/gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java
open/test/hotspot/jtreg/compiler/ciReplay/TestSAClient.java 
open/test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java

Chris

On 3/25/20 12:29 PM, Magnus Ihse Bursie wrote:
> With the recent fixes in JDK-8241310, JDK-8237746 and JDK-8241073, and 
> the upcoming fixes to remove the deprecated nashorn and jdk.rmi, the 
> JDK build is very close to producing no warnings when compiling the 
> Java classes.
>
> The one remaining sinner is jdk.hotspot.agent. Most of the warnings 
> here are turned off, but unchecked and deprecation cannot be 
> completely silenced.
>
> Since the poor agent does not seem to receive much love nowadays, I 
> took it upon myself to fix these warnings, so we can finally get a 
> quiet build.
>
> I started to address the unchecked warnings. Unfortunately, this was a 
> much bigger task than I anticipated. I had to generify most of the 
> module. On the plus side, the code is so much better now. And most of 
> the changes were trivial, just tedious.
>
> There are a few places were I'm not entirely happy with the current 
> solution, and that at least merits some discussion.
>
> I have resorted to @SuppressWarnings in four classes: ciMethodData, 
> MethodData, TableModelComparator and VirtualBaseConstructor. All of 
> them has in common that they are doing slightly fishy things with 
> classes in collections. I'm not entirely sure they are bug-free, but 
> this patch leaves the behavior untouched. I did some efforts to sort 
> out the logic, but it turned out to be too hairy for me to fix, and it 
> will probably require more substantial changes to the workings of the 
> code.
>
> To make the code valid, I have moved ConstMethod to extend Metadata 
> instead of VMObject. My understanding is that this is benign (and 
> likely intended), but I really need for someone who knows the code to 
> confirm this. I have also added a FIXME to signal this. I'll remove 
> the FIXME as soon as I get confirmation that this is OK.
> (The reason for this is the following piece of code from 
> Metadata.java: metadataConstructor.addMapping("ConstMethod", 
> ConstMethod.class))
>
> In ObjectListPanel, there is some code that screams "dead" with this 
> change. I added a FIXME to point this out:
>     for (Iterator<Oop> iter = elements.iterator(); iter.hasNext(); ) {
>       if (iter.next() instanceof Array) {
>         // FIXME: Does not seem possible to happen
>         hasArrays = true;
>         return;
>       }
> It seems that if you start pulling this thread, even more dead code 
> will unravel, so I'm not so eager to touch this in the current patch. 
> But I can remove the FIXME if you want.
>
> My first iteration of this patch tried to generify the IntervalTree 
> and related class hierarchy. However, this turned out to be impossible 
> due to some weird usage in AnnotatedMemoryPanel, where there seemed to 
> be confusion as to whether the tree stored Annotations or Addresses. 
> I'm not entirely convinced the code is correct, it certainly looked 
> and smelled very fishy. However, I reverted these changes since I 
> could not get them to work due to this, and it was not needed for the 
> goal of just getting rid of the warning.
>
> Finally, I have done no testing apart from verifying that it builds. 
> Please advice on suitable tests to run.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8241618
> WebRev: 
> http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.01
>
> /Magnus