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

[Magnus Ihse Bursie]

On 2020-03-25 20:52, Chris Plummer wrote:
> 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. 
Unfortunately, many changes tends to cling together -- for instance, 
class Foo has a List fooList of say Integer. If I change that to 
List<Integer>, then also the constructor needs to change, and the 
getFooList() method, and that in turn propagate to users of getFooList() 
etc. I tried to do this piecewise but for every line that I fixed I just 
ended up getting more and more places that needed fixing.

On the other hand, the patch I present *is* indeed mostly trivial. Apart 
from the places I mentioned below, the fixes are straightforward. And I 
opted out of fixing the tricky ones by disabling the warnings. My 
intention is to file a follow-up bug for these @SuppressWarnings to be 
fixed properly. However, doing that is unfortunately beyond the scope of 
what I'm able to do, since I do not have enough domain knowledge. The 
fixes in this patch is more or less "stupid" applications of adding 
generics with the correct type. (Basically, what I've done is to locate 
a problematic type, like fooList, and check the type of elements 
inserted and extracted of it, and created it as a generic of that type. 
Boring, but not really difficult.)

I realize the webrev can look daunting. Perhaps start by looking at the 
patch file, that will quickly show what kind of changes this is about. 
Also, 1/3 of the patch is just about updating those darned copyright 
years. :-(

> 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
Thank you! I'll run these through our test system.

/Magnus
>
> 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
>
>