Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8274982: Add a test for 8269574. #5889

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 132 additions & 0 deletions test/hotspot/jtreg/compiler/jvmti/TriggerBuiltinExceptionsTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/**
* @test
* @bug 8269574
* @summary Verifies that exceptions are reported correctly to JVMTI in the compiled code
* @requires vm.jvmti
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also require the JIT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a requirement for the c1 or c2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really, we don't add @requires for jit compiler. There are a lot of tests that fail with Xnt.

Copy link
Member

@dholmes-ora dholmes-ora Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests outside of the compiler area which explicitly use features like WB.enqueueMethodForCompilation which explicitly will fail if there is no JIT either require the JIT or exclude running with Zero. See for example:

./runtime/Nestmates/protectionDomain/TestDifferentProtectionDomains.java
./runtime/Unsafe/InternalErrorTest.java
./runtime/exceptionMsgs/AbstractMethodError/AbstractMethodErrorTest.java

EDIT: except of course this test is in the compiler area . Okay perhaps overkill - sorry for the noise.

* @library /test/lib /
*
* @build sun.hotspot.WhiteBox
* @build compiler.jvmti.TriggerBuiltinExceptionsTest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicit build directive should not be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks

*
* @run driver jdk.test.lib.helpers.ClassFileInstaller sun.hotspot.WhiteBox
* @run main/othervm/native
* -Xbootclasspath/a:.
* -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
* -XX:-BackgroundCompilation
* -XX:-TieredCompilation
* -agentlib:TriggerBuiltinExceptions
* compiler.jvmti.TriggerBuiltinExceptionsTest
*/

package compiler.jvmti;

import compiler.testlibrary.CompilerUtils;

import java.lang.reflect.Method;
import static java.lang.Integer.valueOf;

import jdk.test.lib.Asserts;
import jdk.test.lib.Utils;

import sun.hotspot.WhiteBox;


public class TriggerBuiltinExceptionsTest {
private static final WhiteBox WB = WhiteBox.getWhiteBox();
private static final int ITERATIONS = 30; //Arbitrary value, feel free to change
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: space after //

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

private static final long COMP_TIMEOUT = 2000L; //Arbitrary value, feel free to change (millis)
private static final int COMP_LEVEL = CompilerUtils.getMaxCompilationLevel();

private static int caughtByJavaTest = 0;
private static native int caughtByJVMTIAgent();

public static void methodToCompile(int i, Object src[], Object[] dest) {
try {
int idxFromSrc = (int)(src[i]); // NPEs, CastExceptions;
int rangeAdjust = 3 / (idxFromSrc % 3); // Each 3rd is division by 0
Object value = src[i - rangeAdjust]; // Array indexing is broken
dest[i] = (Long)value; // ArrayStoreException or CastException
} catch (Exception e) {
caughtByJavaTest += 1;
}
}

/**
* Makes sure that method is compiled.
*/
private static void compileMethodOrThrow(Method method) {
boolean enqueued = WB.enqueueMethodForCompilation(method, COMP_LEVEL);
if (!enqueued) {
throw new Error(String.format("%s can't be enqueued for compilation on level %d",
method, COMP_LEVEL));
}
Asserts.assertTrue(
Utils.waitForCondition(() -> WB.isMethodCompiled(method), COMP_TIMEOUT),
String.format("Method hasn't been compiled in %d millis", COMP_TIMEOUT));
}

/**
* 1. Compiles method with no profiling information;
* 2. Causes deoptimization using arguments that causes hot throws;
* 3. Compiles the method again;
* 4. Checks that exceptions within compiled code are registered by JVMTI agent.
*/
public static void main(String[] args) throws Throwable {
// Preparing the method
final Method method = TriggerBuiltinExceptionsTest.class.getMethod(
"methodToCompile", int.class, Object[].class, Object[].class);
WB.deoptimizeMethod(method);
TriggerBuiltinExceptionsTest.compileMethodOrThrow(method);

// Preparing source - badly formed, supposed-to-be-int array
final Object[] src = new Object[] {
valueOf(0), null, null, valueOf(3), null, valueOf(5), "string",
Long.valueOf(Long.MAX_VALUE), null, valueOf(9), "string"};
final Object[] dst = new Integer[ITERATIONS];

// 1. Should cause deoptimization as array is null
// 2. Make the throw hot to make the next compilation aware of it.
for (int i = 0; i < ITERATIONS; i++) {
TriggerBuiltinExceptionsTest.methodToCompile(i, src, dst);
}
Asserts.assertTrue(
Utils.waitForCondition(() -> !WB.isMethodCompiled(method), COMP_TIMEOUT),
String.format("Method hasn't been deoptimized in %d millis", COMP_TIMEOUT));

// Compile method with throw being hot
TriggerBuiltinExceptionsTest.compileMethodOrThrow(method);

// Gathering exceptions in compiled code
for (int i = 0; i < ITERATIONS; i++) {
TriggerBuiltinExceptionsTest.methodToCompile(i, src, dst);
}

Asserts.assertEQ(
TriggerBuiltinExceptionsTest.caughtByJVMTIAgent(), caughtByJavaTest,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason to use the class name prefix for methods? :
TriggerBuiltinExceptionsTest.compileMethodOrThrow
TriggerBuiltinExceptionsTest.methodToCompile
TriggerBuiltinExceptionsTest.caughtByJVMTIAgent
It is not really needed, tight?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style habits, acquired in previons job... fixed.

"Number of Exceptions caught by java code and JVMTI agent does not match");
}

}
88 changes: 88 additions & 0 deletions test/hotspot/jtreg/compiler/jvmti/libTriggerBuiltinExceptions.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

#include <string.h>
#include "jvmti.h"

static jint exceptions_caught = 0;

extern "C" {

void JNICALL
callbackException(jvmtiEnv *jvmti_env, JNIEnv* jni_env,
jthread thread, jmethodID method,
jlocation location, jobject exception,
jmethodID catch_method, jlocation catch_location) {
exceptions_caught += 1;
}
Comment on lines +32 to +37
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this will count all exceptions occurring in any thread and in relation to any method. There are behind-the-scenes exceptions that can occur which may cause this to count exceptions not related to the test. I think you need to only count those thrown in the method of interest, for reliability.


JNIEXPORT jint JNICALL
Agent_OnLoad(JavaVM *jvm, char *options, void *reserved) {
jvmtiEnv *jvmti = nullptr;

bool successful = true;
jint result = JNI_OK;
do {
result = jvm->GetEnv((void **) (&jvmti), JVMTI_VERSION);
if (result != JNI_OK) {
printf("Agent_OnLoad: Error in GetEnv in obtaining jvmtiEnv: %d\n", result);
break;
}

jvmtiEventCallbacks callbacks;
memset(&callbacks, 0, sizeof(callbacks));
callbacks.Exception = &callbackException;

result = jvmti->SetEventCallbacks(&callbacks, sizeof(jvmtiEventCallbacks));
if (result != JVMTI_ERROR_NONE) {
printf("Agent_OnLoad: Error in JVMTI SetEventCallbacks: %d\n", result);
break;
}

jvmtiCapabilities capabilities;
memset(&capabilities, 0, sizeof(capabilities));
capabilities.can_generate_exception_events = 1;
result = jvmti->AddCapabilities(&capabilities);
if (result != JVMTI_ERROR_NONE) {
printf("Agent_OnLoad: Error in JVMTI AddCapabilities: %d\n", result);
break;
}

result = jvmti->SetEventNotificationMode(JVMTI_ENABLE, JVMTI_EVENT_EXCEPTION, (jthread)NULL);
if (result != JVMTI_ERROR_NONE) {
printf("Agent_OnLoad: Error in JVMTI SetEventNotificationMode: %d\n", result);
break;
}

} while (false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the while (false) loop is needed.
You can always return JNI_ERR instead of break in all places where the
result != JVMTI_ERROR_NONE is detected and return JNI_OK at the end.
Is it to for one-return style?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remnants from a previous, draft version. Fixed (along with unnecessary 'successfull' variable removal).


return (result == JNI_OK) || (result == JVMTI_ERROR_NONE) ? JNI_OK : JNI_ERR;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems (result == JNI_OK) || (result == JVMTI_ERROR_NONE) should be (result == JNI_OK) && (result == JVMTI_ERROR_NONE).

Really, I think it would be better to replace
do {
...
break;
..
} while (false)
with multiply return.

It makes logic simpler and style compliant to all jvmti tests.

}

JNIEXPORT jint JNICALL
Java_compiler_jvmti_TriggerBuiltinExceptionsTest_caughtByJVMTIAgent(JNIEnv *env, jclass cls) {
return exceptions_caught;
}


} // extern "C"