Add tests for fieldwatch and fix x86 codegen bug #6257
Conversation
|
@smlambert @gacholio @pdbain-ibm Can I get a review please? |
|
Jenkins test extended windows,plinux jdk8,jdk11 |
|
Jenkins test extended win,plinux jdk8,jdk11 |
|
Any files that tests are creating should be inside the workspace folder which gets deleted at the end of the run. If there are any PRs where it appears that tests create files that are put into other folders on the system like into /tmp we should change the PR to keep the detritus within the containing folder the tests were run, so that they can be optionally archived and deleted off the machines. |
| @@ -65,7 +65,7 @@ RUN apt-get update \ | |||
| && rm -rf /var/lib/apt/lists/* | |||
|
|
|||
| # Install Docker module to run test framework | |||
| RUN echo yes | cpan install JSON Text::CSV | |||
| RUN echo yes | cpan install JSON Text::CSV XML::Parser | |||
There was a problem hiding this comment.
I think we're also missing curl in the above apt-get command. I just built the Docker image and it complained that it is not installed. We may want to address this here given we're fixing the XML::Parser dependency.
There was a problem hiding this comment.
We can likely let the tests complete and make note of them here because adding a new change will erase records of the testing from the PR.
There was a problem hiding this comment.
In the previous attempt at this pull request, the discussion around whether we should be keeping this Dockerfile or deleting them was mentioned (discussion in #5276). Also note that the dependency on XML::Parser was removed in a cautious manner for the z/OS platform only to enable z/OS testing.
As discussed already, post-July release we will revisit the prototype Java rewrite for next phase of removing dependencies.
There was a problem hiding this comment.
Should we still add curl and XML::Parser to the current Dockerfile? I'm fine either way as I'm aware of the issue. Was hoping to save some time for others who may try to use the buildenv/docker/test/Dockerfile as it's currently missing prerequisites referenced via the Reproducing Test Failures Locally wiki article [1] [2].
[1] https://github.com/eclipse/openj9/blob/master/test/docs/Prerequisites.md
[2] https://github.com/eclipse/openj9/wiki/Reproducing-Test-Failures-Locally (step 0)
Edit:
Sorry I misread. I see the discussion now in [3]. Seems @keithc-ca's suggestion was to separate this Dockerfile change in a separate PR if I'm understanding correctly. Sounds like a reasonable action.
[3] #6148 (comment)
I suspect I'm not the only one who learned a new word today: "detritus". Thanks @smlambert ! |
|
Adding @gacholio as a reviewer for the c code as he expressed interest to review native test code to begin to have it follow the coding standard (note: the corpus of the native test code written by the dev team not following the coding standard at present... a case of better late than never one supposes). |
runtime/tests/jvmtitests/src/com/ibm/jvmti/tests/fieldwatch/fw001.c
Outdated
Show resolved
Hide resolved
runtime/tests/jvmtitests/src/com/ibm/jvmti/tests/fieldwatch/fw001.c
Outdated
Show resolved
Hide resolved
test/functional/cmdLineTests/jvmtitests/src/com/ibm/jvmti/tests/fieldwatch/fw001.java
Outdated
Show resolved
Hide resolved
And the "standard" is largely however the reviewer feels that day :) |
|
@dchopra001 Can this test be run without the JIT? I think that's an important feature (field event correctness, regardless of the run mode). |
Yes. I have the following test config in |
runtime/tests/jvmtitests/src/com/ibm/jvmti/tests/fieldwatch/fw001.c
Outdated
Show resolved
Hide resolved
runtime/tests/jvmtitests/src/com/ibm/jvmti/tests/fieldwatch/fw001.c
Outdated
Show resolved
Hide resolved
runtime/tests/jvmtitests/src/com/ibm/jvmti/tests/fieldwatch/fw001.c
Outdated
Show resolved
Hide resolved
runtime/tests/jvmtitests/src/com/ibm/jvmti/tests/fieldwatch/fw001.c
Outdated
Show resolved
Hide resolved
runtime/tests/jvmtitests/src/com/ibm/jvmti/tests/fieldwatch/fw001.c
Outdated
Show resolved
Hide resolved
test/functional/cmdLineTests/jvmtitests/src/com/ibm/jvmti/tests/fieldwatch/fw001.java
Outdated
Show resolved
Hide resolved
test/functional/cmdLineTests/jvmtitests/src/com/ibm/jvmti/tests/fieldwatch/fw001.java
Outdated
Show resolved
Hide resolved
test/functional/cmdLineTests/jvmtitests/src/com/ibm/jvmti/tests/fieldwatch/fw001.java
Outdated
Show resolved
Hide resolved
runtime/tests/jvmtitests/src/com/ibm/jvmti/tests/fieldwatch/fw001.c
Outdated
Show resolved
Hide resolved
| @@ -207,6 +207,36 @@ | |||
| <type>native</type> | |||
| </types> | |||
| </test> | |||
|
|
|||
|
|
|||
| <test> | |||
There was a problem hiding this comment.
@smlambert Does this need to be a completely separate test? If it's fast, I see no reason not to include it in the sanity JVMTI test suite, rather than running it separately.
There was a problem hiding this comment.
I am fine to have it as part of the sanity suite instead of extended, though considering both suites now get run daily and both must pass (we no longer have the notion of non-gold), I think it is also sufficient to keep it in the extended level. Should we decide to move it to sanity, we can change line 229 below to be <level>sanity</level>
And to answer the other question, @dchopra001 could have combined this into some other test, but it is nice to have the granularity here to run it as its own standalone target.
| package com.ibm.jvmti.tests.fieldwatch; | ||
|
|
||
| class TestDriver | ||
| { |
There was a problem hiding this comment.
A few more misplaced curlies in this file.
There was a problem hiding this comment.
Ah yeah I forgot about this one. I've fixed the tab spacing and braces issues here: 708b40a751
|
@dchopra001 can we squash the review commits? It seems all review comments pertaining to the tests themselves have been addressed. Once the squash happens we can kick off a PR build to ensure functionality again. Given this is in the test component @smlambert would you sign up to be an assigned committer for this one? |
|
count=0 also means that the fields will be unresolved when you first encounter them - perhaps that's related? |
Yep. I'm pretty sure that's related. We go down a unique path in the unresolved case. What's strange is that we are able to report successfully once before the NPE is thrown. With |
|
To be honest, I still don’t see how we would compromise any quality if we:
Otherwise we need to spend quite a bit of time to try and run these tests locally on Power. The above will save us time and unlock Power implementation, eg why do we need to wait until Intel defects are fixed? We can also disable the tests for Intel as they have been for a while now anyway. |
When generating code for a watched field event, if an unresolved static field is being read, we do not intialize the fieldClassReg correctly. The fieldClassReg is used to test if a field is being watched. Failure to initialize this register correctly results in an invalid test being generated at runtime, causing a bug. This commit correctly initializes the fieldClassReg for the missing corner case. Signed-off-by: Dhruv Chopra <[email protected]>
|
I was able to reproduce this in a local unit test and now know what the problem is. It looks like I missed a corner case when consolidating routines for fieldwatch on x86. See cf5e106 for more details and the fix. With the above commit, I'm able to get passes on both 32-bit and 64-bit JVMs on x86Linux in my local testing. I'm not sure why this was only failing on 32-bit. The bug should be equally likely on 64-bit x86 machines as well. (It's possible that the test drivers are too simple and so we were just getting lucky on 64-bit due to the extra registers). As a result of all the consolidation we did, the logic in this specific area became kind of convoluted. This explains why I missed the corner case on x86. At some point later I'll come back to rethink some of this. I'm sure we can make this simpler across all platforms. I have pushed a fix to this problem here. We can launch tests again. They should pass now. Note: I checked the Power and Z implementations for this scenario and they shouldn't have this problem. |
|
That’s the best case scenario. Thanks so much for the quick resolution! |
|
Jenkins test extended all jdk8,jdk11 |
|
There's a PPC failure in the build. The failure can't be related to the changes in this PR since there's nothing here that affects PPC. Here are the details of the failure anyway: |
|
TestAttachErrorHandling_0 sounds familiar. @pshipton is it some known failure? |
|
@keithc-ca would you like to review or we can merge? |
|
@dchopra001 do you think we should run it with noncompressedrefs as well? |
Yes, I forgot to add |
|
Jenkins test extended all jdk8,jdk11 |
|
@dchopra001 I think field watch failed on z. |
This isn't a functional issue. I think I need to specify the options in a completely different class of tests. |
|
Can I get some help here? How can I add a separate set of tests to for non-compressed ref JVMs? Is there an existing example I can look at? |
|
Large heap variant builds don't exist on many OpenJ9 platforms. Additionally we don't test on all 32-bit platforms. I've opened an issue here to discuss how we can run these tests on those platforms: I've also backed out the commit that added tests for |
|
The issue is that you need to add the -XX option. Perhaps a better way would be to use the normal JVMTI mechanism (as I suggested before), and add the inline option separately. This would mean running a single test in multiple modes, which already occurs in the suite (not sure about the other JIT options you want - the single-parse of the -Xjit line makes this kind of thing very difficult to achieve). |
|
Since AIX failure is accounted for and non-compressed refs build is out of the scope of this PR (an issue opened) I will merge. |
dchopra001
changed the title
This commit was recently reverted here: #6148. I have fixed the issues and so am opening another PR.
A bug fix was also made on the x86 platform. See cf5e106 for details.