Re: RFR: JDK-8241421 Cleanup handling of jtreg
Looks good to me. Note, the changes in jib-profiles.js will not change any behavior, but being explicit is better than trusting defaults. /Erik On 2020-03-30 07:49, Magnus Ihse Bursie wrote: On 2020-03-23 14:06, Erik Joelsson wrote: Hello, On 2020-03-23 02:21, Magnus Ihse Bursie wrote: We're doing some odd things in the build system for jtreg, most of it left-overs from the older build systems. We have no need for JTREGEXE and it should be removed. We should also use the standard naming pattern of JTREG_HOME. The environment variable name JT_HOME is defined by Jtreg itself so I think it makes sense to use that. Configure is still honoring it, but run-test-prebuilt is not. We can of course use a different variable name internally, but I don't really see the reason as JT_HOME is the generally accepted one. In jib-profiles.js, the configure_args for jtreg were not specified as they were generated by default (a feature you requested way back when jib was initially designed). I don't mind it being explicitly set though. You can use "home_path" to have jib automatically find the single "jtreg" subdir inside the archive. Adding $JT_HOME/bin to the path is done to support (the rarely used) "jib run" command. It enables you to have jib provide an environment for running a command directly, such as jtreg. I don't want that removed. For the same reason, I want JT_HOME to remain globally set as the jtreg launcher script could otherwise pick up a different JT_HOME from the users environment. I have now updated the patch to remove the rename from JT_HOME to JTREG_HOME, and will be saving that for a separate bug when Jon has implemented the rename in jtreg and we have upgraded to use that jtreg. That means that the current fix is more or less just "stop using JTREGEXE". I should perhaps rename the bug to reflect this. I hope I understood your comments about jib-profiles.js correctly. Updated webrev: http://cr.openjdk.java.net/~ihse/JDK-8241421-cleanup-jtreg/webrev.02 /Magnus /Erik The test in BuildFailureHandler.gmk is a bit different -- it is supposed to fail. I have run it both with and without the patch, and verified that the test result and test log are basically the same, except for normal runtime variance like time stamps and certain randomness. I have also verified that testing through the Oracle CI system still works as expected. Bug: https://bugs.openjdk.java.net/browse/JDK-8241421 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8241421-cleanup-jtreg/webrev.01 /Magnus
Re: RFR: JDK-8241421 Cleanup handling of jtreg
On 2020-03-23 14:06, Erik Joelsson wrote: Hello, On 2020-03-23 02:21, Magnus Ihse Bursie wrote: We're doing some odd things in the build system for jtreg, most of it left-overs from the older build systems. We have no need for JTREGEXE and it should be removed. We should also use the standard naming pattern of JTREG_HOME. The environment variable name JT_HOME is defined by Jtreg itself so I think it makes sense to use that. Configure is still honoring it, but run-test-prebuilt is not. We can of course use a different variable name internally, but I don't really see the reason as JT_HOME is the generally accepted one. In jib-profiles.js, the configure_args for jtreg were not specified as they were generated by default (a feature you requested way back when jib was initially designed). I don't mind it being explicitly set though. You can use "home_path" to have jib automatically find the single "jtreg" subdir inside the archive. Adding $JT_HOME/bin to the path is done to support (the rarely used) "jib run" command. It enables you to have jib provide an environment for running a command directly, such as jtreg. I don't want that removed. For the same reason, I want JT_HOME to remain globally set as the jtreg launcher script could otherwise pick up a different JT_HOME from the users environment. I have now updated the patch to remove the rename from JT_HOME to JTREG_HOME, and will be saving that for a separate bug when Jon has implemented the rename in jtreg and we have upgraded to use that jtreg. That means that the current fix is more or less just "stop using JTREGEXE". I should perhaps rename the bug to reflect this. I hope I understood your comments about jib-profiles.js correctly. Updated webrev: http://cr.openjdk.java.net/~ihse/JDK-8241421-cleanup-jtreg/webrev.02 /Magnus /Erik The test in BuildFailureHandler.gmk is a bit different -- it is supposed to fail. I have run it both with and without the patch, and verified that the test result and test log are basically the same, except for normal runtime variance like time stamps and certain randomness. I have also verified that testing through the Oracle CI system still works as expected. Bug: https://bugs.openjdk.java.net/browse/JDK-8241421 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8241421-cleanup-jtreg/webrev.01 /Magnus
Re: RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent
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 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
Re: RFR: JDK-8241421 Cleanup handling of jtreg
On 2020-03-27 16:37, Jonathan Gibbons wrote: My reading of the jtreg code is that JT_HOME is only used in the startup script, and it does not require it to be set; it will figure it out if needed. It is not required within the Java source code. I'm thinking to change the script to use JTREG_HOME, JTREG_JAVA (instead of JT_HOME, JT_JAVA), and that I will default these new names to the values of the old names, as a backwards compatibility measure. I also notice some inconsistent variations on the name in comments that I will also fix. Does this seem a reasonable approach? It does indeed. /Erik
Re: RFR: JDK-8241463 Move build tools to respective modules
On 2020-03-28 10:34, Weijun Wang wrote: On Mar 24, 2020, at 3:03 AM, Magnus Ihse Bursie wrote: I have also moved the build tools to the org.openjdk.buildtools.* package name space (inspired by Skara), instead of the strangely named build.tools.* name space. Is this really necessary? Skara is a standalone tool that have a public interface, but the build tools are entirely internal. They can even be non public. Well, no, of course it is not *necessary*. We can move the tools without changing their package. Or we can change their package without moving them. Or we can do nothing. However, I think it would be sensible to do so. It will clarify that the tools are build tools written for the openjdk project. And it will make us walk the walk, not just talk the talk, by adhering to the long-standing standards of naming packages. Like, really, what kind of package name is "build.tools.*"??? It just sucks on so many levels. Why not even "buildtools.*"? What on earth is that dot doing there? It's not like we have "build.non-tools.*" ... Since we'd be moving the files anyway, the cost of changing the package naming is negligible, so I think it's good to use this chance to fix it. /Magnus --Max
