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

[Magnus Ihse Bursie]

On 2020-04-14 23:23, Chris Plummer wrote:
> Hi Magnus,
>
> The changes look good. Just one minor issue:
>
> http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Metadata.java.frames.html
>
> Copyright was updated, but no other changes in this file.
Thanks for noticing! I reverted that change before pushing.

>
> BTW, do you know why there were cases in the old code of having the 
> right generic type in a comment. For example:
>
> 1177     private static List/*<Field>*/ 
> getInstanceFields(InstanceKlass ik) {
>
> and
>
> 1276     private Map classDataCache = new HashMap(); // 
> <InstanceKlass, ClassData>
>
> It's almost as if some of the code was initially properly written for 
> generics, but then backported to work with a pre-generics version of jdk.
Yeah, isn't it? I have no clue, though. Most of the code is written 
completely oblivious to generics. Some parts have the typical 
pre-generics style of documenting in comments what types were expected, 
but most said nothing about the issue. And a few places has something 
that look like proper generics commented out. Maybe that's just the 
original programmer using C++ generics-inspired syntax as a way to 
comment the expected types?

/Magnus
>
> thanks,
>
> Chris
>
> On 4/14/20 2:24 AM, Magnus Ihse Bursie wrote:
>> 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
>>>>
>>>>
>>>
>>
>