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

[Magnus Ihse Bursie]

Hi,

Can I please get a review for this, simplified version of the patch? 
This only contain trivial changes, like this:

-  private List    objects; // ArrayList<ScopeValue>
+  private List<ObjectValue> objects;

Basically all changes are to the container types List or Map (and a few 
changes from Class to Class<?>).

If the list of all the files look horrible in the webrev, have a look at 
the patch file instead, it is easier to scroll through and check the 
changes:
http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02/open.patch

/Magnus

On 2020-03-30 16:25, Magnus Ihse Bursie wrote:
>
>
> 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. 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.
> Ok, I did just this. Here is an updated webrev. It contain the bulk of 
> the changes, but all changes are -- I dare not say trivially obvious, 
> but at least no-brainers. Hopefully it should be easier to review so I 
> can get this pushed and out of the way.
>
> This also means that it is not possible to turn on the warning just yet.
>
> http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02 
>
>
> /Magnus
>>
>> 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
>>
>>
>