<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">Hi Magnus,<br>
<br>
The changes look good. Just one minor issue:<br>
<br>
<a class="moz-txt-link-freetext" href="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">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</a><br>
<br>
Copyright was updated, but no other changes in this file.<br>
<br>
BTW, do you know why there were cases in the old code of having
the right generic type in a comment. For example:<br>
<br>
1177Â Â Â Â private static List/*<Field>*/
getInstanceFields(InstanceKlass ik) {<br>
<br>
and<br>
<br>
1276Â Â Â Â private Map classDataCache = new HashMap(); //
<InstanceKlass, ClassData><br>
<br>
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.<br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 4/14/20 2:24 AM, Magnus Ihse Bursie wrote:<br>
</div>
<blockquote type="cite"
cite="mid:6c008d7c-db6a-e9ad-5b49-e90703e19bad@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
Hi,<br>
<br>
Can I please get a review for this, simplified version of the
patch? This only contain trivial changes, like this:<br>
<pre style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none; word-wrap: break-word; white-space: pre-wrap;">- private List objects; // ArrayList<ScopeValue>
+ private List<ObjectValue> objects;</pre>
Basically all changes are to the container types List or Map (and
a few changes from Class to Class<?>).<br>
<br>
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:<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02/open.patch"
moz-do-not-send="true">http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02/open.patch</a><br>
<br>
/Magnus<br>
<br>
<div class="moz-cite-prefix">On 2020-03-30 16:25, Magnus Ihse
Bursie wrote:<br>
</div>
<blockquote type="cite"
cite="mid:a0ad7e59-ae77-c805-9cca-c18ab51a7505@oracle.com"> <br>
<br>
On 2020-03-25 20:52, Chris Plummer wrote: <br>
<blockquote type="cite">Hi Magus, <br>
<br>
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. <br>
</blockquote>
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.
<br>
<br>
This also means that it is not possible to turn on the warning
just yet. <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02"
moz-do-not-send="true">http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02</a>
<br>
<br>
/Magnus <br>
<blockquote type="cite"> <br>
As for testing, I think the following list will capture all of
them, but can't say for sure: <br>
<br>
open/test/hotspot/jtreg/serviceability/sa <br>
open/test/hotspot/jtreg/resourcehogs/serviceability/sa <br>
open/test/jdk/sun/tools/jhsdb <br>
open/test/jdk/sun/tools/jstack <br>
open/test/jdk/sun/tools/jmap <br>
open/test/hotspot/jtreg/gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java
<br>
open/test/hotspot/jtreg/compiler/ciReplay/TestSAClient.java
open/test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java <br>
<br>
Chris <br>
<br>
On 3/25/20 12:29 PM, Magnus Ihse Bursie wrote: <br>
<blockquote type="cite">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. <br>
<br>
The one remaining sinner is jdk.hotspot.agent. Most of the
warnings here are turned off, but unchecked and deprecation
cannot be completely silenced. <br>
<br>
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. <br>
<br>
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. <br>
<br>
There are a few places were I'm not entirely happy with the
current solution, and that at least merits some discussion.
<br>
<br>
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. <br>
<br>
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. <br>
(The reason for this is the following piece of code from
Metadata.java: metadataConstructor.addMapping("ConstMethod",
ConstMethod.class)) <br>
<br>
In ObjectListPanel, there is some code that screams "dead"
with this change. I added a FIXME to point this out: <br>
   for (Iterator<Oop> iter = elements.iterator();
iter.hasNext(); ) { <br>
     if (iter.next() instanceof Array) { <br>
       // FIXME: Does not seem possible to happen <br>
       hasArrays = true; <br>
       return; <br>
     } <br>
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. <br>
<br>
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.
<br>
<br>
Finally, I have done no testing apart from verifying that it
builds. Please advice on suitable tests to run. <br>
<br>
Bug: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8241618"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8241618</a>
<br>
WebRev:
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.01"
moz-do-not-send="true">http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.01</a><br>
<br>
/Magnus <br>
</blockquote>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>