[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent
From:       Magnus Ihse Bursie <magnus.ihse.bursie () oracle ! com>
Date:       2020-03-30 14:25:13
Message-ID: a0ad7e59-ae77-c805-9cca-c18ab51a7505 () oracle ! com
[Download RAW message or body]



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

[prev in list] [next in list] [prev in thread] [next in thread]