Initial JNI configuration #157

xevor11 · 2024-07-09T13:31:26Z

Cleaned up java.c, added the requested changes from PR#150, ready for feedback and comments

configure.ac

libcob/common.h

m4/ax_jni_include_dir.m4

m4/ax_prog_java.m4

cobc/codegen.c

nberth · 2024-07-10T16:36:25Z

cobc/codegen.c

+				}
+				output ("cob_resolve_java ();");
+				output_newline ();
+				output_block_close ();


Right here you need to generate a call to cob_call_java(…) with the appropriate handle name call_java_%s. Arguments will come in a subsequent PR.

cobc/codegen.c

nberth · 2024-07-10T16:39:26Z

cobc/ppparse.output

This file should not be committed.

libcob/java.c

nberth · 2024-07-10T16:42:49Z

po/[email protected]

Everything under po should not be committed either.

configure.ac

GitMensch

the real next change that should be done is an actual test for calling a static java method that has no parameters, to verify that java.c indeed works

GitMensch · 2024-07-11T08:56:58Z

cobc/codegen.c

@@ -146,6 +152,7 @@ static struct literal_list	*literal_cache = NULL;
 static struct field_list	*field_cache = NULL;
 static struct field_list	*local_field_cache = NULL;
 static struct call_list		*call_cache = NULL;
+static struct java_call_list *call_java_cache = NULL;


minor formatting: use a tab before the *

GitMensch · 2024-07-11T08:58:25Z

cobc/codegen.c

+lookup_java_call(const char *p) {
+    struct java_call_list *clp;
+
+    for (clp = call_java_cache; clp; clp = clp->next) {
+        if (strcmp(p, clp->call_name) == 0) {
+            return;
+        }
+    }
+    clp = cobc_parse_malloc(sizeof(struct java_call_list));


minor formatting: a space before the (, see lookup_call below (and sorry for not providing an automated formatting rule via gnu indent yet)

GitMensch · 2024-07-11T08:59:56Z

cobc/codegen.c

 static void
 lookup_call (const char *p)
 {
 	struct call_list *clp;
-


minor formatting: please keep this nl here, reduces the diff :-)

GitMensch · 2024-07-11T09:06:46Z

cobc/codegen.c

+			if(
+#ifdef HAVE_JNI 
+				strncmp("Java.", s, 6) == 0
+#else
+				0
+#endif
+			) {


It is possible that the compiler isn't build with JNI, but the runtime is - and we don't call into JNI here either, so please use the code unconditional; in any case this whole block should be moved, either to the start of lookup_func_call, or (if there aren't too much places) in the caller with having this code be placed completely in a different function.
Mind the formatting = space before ( in that.

GitMensch · 2024-07-11T09:08:49Z

cobc/codegen.c

+#endif
+			) {
+				lookup_java_call(s + 6);
+				output_line ("if (call_java_%s == NULL || cob_glob_ptr->cob_physical_cancel)", name_str);


Is there a way to CANCEL a java class (and if yes to really unload it without jni teardown)?
I guess not - and in this case the check for physical cancel should be dropped.

GitMensch · 2024-07-11T09:11:17Z

cobc/codegen.c

+				output_block_open();
+				output_prefix();
+				output ("call_java_%s = ", name_str);
+				char *first_dot = strchr(s + 6, '.');


To be able to compile that with C89 (still a requirement for 4.x, may be dropped later), move this line to the start of the block.

configure.ac

GitMensch · 2024-07-11T10:01:11Z

configure.ac

+AC_ARG_ENABLE([java],
+              [AC_HELP_STRING([--disable-java],
+                              [disable Java Interoperability])])
+AS_IF([test X$enable_java != Xno], [


ARC_ARG_ENABLE is more about optional features (like the debug log) and less about external packages where AC_ARG_WITH is preferable (and would also match what configure has for example for libxml2).

See https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.72/html_node/External-Software.html and adjust (you may postpone that part until you have actual running tests, though).

In any case please move the JNI block directly before the libxml2 part.

GitMensch · 2024-07-11T10:05:18Z

libcob/common.h

+// #ifdef HAVE_JNI
+// #include "jni.h"


these commented line and the one above should be dropped

GitMensch · 2024-07-11T10:31:06Z

I hope my commit to java.c did not break anything... (I just felt that the explanations would take much longer and those are more related to libcob in general than the changes of this project itself).
... if not: thank you for cleaning it up.

And yes, once there is a running simple test, we should update the branch from gcos4 which will then also include the CI. for further commits.

xevor11 · 2024-07-12T12:53:29Z

I hope my commit to java.c did not break anything... (I just felt that the explanations would take much longer and those are more related to libcob in general than the changes of this project itself). ... if not: thank you for cleaning it up.

And yes, once there is a running simple test, we should update the branch from gcos4 which will then also include the CI. for further commits.

Update: Fixed warnings

cobc/codegen.c

nberth · 2024-07-26T12:19:02Z

cobc/codegen.c

+    char *method_name;
+    const char *class_name;
+
+    lookup_java_call(p + 5);


Here you need to do some mangling: at the moment maybe substitute .s into some other character like _.

cobc/codegen.c

GitMensch · 2024-07-29T14:35:40Z

cobc/codegen.c

+    struct java_call_list *clp;
+
+    for (clp = call_java_cache; clp; clp = clp->next) {
+        if (strcmp (p, clp->call_name) == 0) {
+            return;
+        }
+    }
+    clp = cobc_parse_malloc (sizeof(struct java_call_list));
+    clp->call_name = p;
+    clp->next = call_java_cache;
+    call_java_cache = clp;


please indent on the left with tab

GitMensch · 2024-07-29T14:36:26Z

cobc/codegen.c

-	for (clp = func_call_cache; clp; clp = clp->next) {
-		if (strcmp (p, clp->call_name) == 0) {
-			return;
-		}
-	}
-	clp = cobc_parse_malloc (sizeof (struct call_list));
-	clp->call_name = p;
-	clp->next = func_call_cache;
-	func_call_cache = clp;
+    for (clp = func_call_cache; clp; clp = clp->next) {
+        if (strcmp(p, clp->call_name) == 0) {
+            return;
+        }
+    }
+    clp = cobc_parse_malloc(sizeof(struct call_list));
+    clp->call_name = p;
+    clp->next = func_call_cache;
+    func_call_cache = clp;


indentation got wrong, please clean up, which will also lead to a smaller diff

GitMensch

Requesting mostly formatting changes (and the variable name pointed out by Nicolas) for now - we really would need a testcase that showcase a call to a static, preferably parameterless, jre internal function works fine.

The one "real" change is to move the check for the java call into the parser - see review comments for the details.

And even if it doesn't work it should still be added to a new testcase at the end of run_extensions.at

GitMensch · 2024-07-29T14:38:31Z

cobc/codegen.c

+			if(strncasecmp("Java.", s, 5) == 0) {
+				output_java_call(s + 5);
+				return;
+			} else {


drop that else and the closing brace as you have a return already and don't need the additional scope (and create smaller diffs that way)

GitMensch · 2024-07-29T14:40:40Z

ChangeLog

+	* ax_prog_java.m4: Added macro for jni check
+	* ax_jni_include_dir.m4: Added macro for jni check
+    * configure.ac: added support for Java interoperability through JNI


space indent here, leave an empty line before and after the name, please

GitMensch · 2024-07-29T14:56:51Z

cobc/codegen.c

@@ -7531,6 +7582,10 @@ output_call (struct cb_call *p)
 		/* Dynamic link */
 		if (name_is_literal_or_prototype) {
 			s = get_program_id_str (p->name);
+			if(strncasecmp("Java.", s, 5) == 0) {


this comparision should go into parser.y

gnucobol/cobc/parser.y

Lines 12529 to 12530 in 00f6832

if (CB_LITERAL_P ($3)) {

cb_check_conformance ($3, $7, $8);

I suggest that after we found out if this is a java call or not, we set the call_convention to a new value matching a define JAVA_CALL. This has the benefit that we can then also check the parameter conformance later (codegen is too later for that) and output a warning if the current environment does not support Java interop.
In this function here we can then directly at the start or even in the caller get to output_call_java().

@xevor11 To help with that, I've just rebased java-interop on top of master (which is trunk on the upstream). Now you can rebase your jni-config branch on top of that so you'll have access to the piece of code that's linked above.

After that you can define CB_CONV_JAVA in tree.h, and then do the check for the "Java." prefix in parser.y (I think right before the call to cb_check_conformance, but I'm not sure what that latter function does exactly).

yes right before that function, which checks that something is according to a known prototype - in the case of Java that should check that only "java interop compatible" arguments are passed.

xevor11 · 2024-07-30T00:36:55Z

Addressing this specific system call:

System.getProperty("java.version")

I wrote a simple method call:

if ((cob_global_exception & 0x0b00) == 0x0b00) cob_global_exception = 0;
    if (call_java_class_method_call.funcvoid == NULL || cob_glob_ptr->cob_physical_cancel)
  {
    call_java_class_method_call.funcvoid = cob_resolve_cobol ("java_class_method_call", 0, 0);
  }

however internally it must be create the VM and retrieve the methods from the JNI Function Table?

GitMensch · 2024-08-21T08:48:31Z

As noted in the other PR where we had the last commit before: please adjust libcob/Changelog for that and revert the definition of lt_dladdsearchdir and the changes to the defines above; the use of JAVA_HOME should definitely be documented in gnucobol.texi.
Please adjust the new CI files as well (no install.log needed, adding the || make check TESTSUITEFLAGS="--recheck --verbose" for getting details on the failing testcase runs, replacing the commented NIST tests by a single note that those are not useful to be executed with this build as the java parts are not used there).

Afterwards (and after integrating the Windows CI that is work in progress) - what would be next for this PR? Checking that the arguments are one of the cobol java types and handle those?

nberth · 2024-08-21T12:29:27Z

Afterwards (and after integrating the Windows CI that is work in progress) - what would be next for this PR? Checking that the arguments are one of the cobol java types and handle those?

I was thinking, it may be more manageable to restrict this PR to only deal with void (void) methods, and leave handling of parameters and returned values in a dedicated PR.
In this case, what is missing for this PR are:

proper detection and reporting of calls to missing methods (one of the new tests should not pass as is);
reporting/erroring on CALL "Java.…" with parameters and/or returned values (maybe with a CB_PENDING, probably to be added in parser.y);
moving detection of malformed method names to the parsing phase (CALL "Java.someMethodWihtoutClass" should be reported before codegen);
adjustments to the ChangeLogs and the documentation;
(probably some other missing items).

libcob/java.c

GitMensch · 2024-08-21T14:36:25Z

It will be quite beneficial if we use the x86 MSYS2 version for testing an old JDK (version 8).

Can you please try to put a cached download of https://github.com/adoptium/temurin8-binaries/releases/download/jdk8u422-b05/OpenJDK8U-jdk_x86-32_windows_hotspot_8u422b05.zip in use - only for mingw32?

nberth · 2024-08-21T14:53:14Z

It will be quite beneficial if we use the x86 MSYS2 version for testing an old JDK (version 8).

Can you please try to put a cached download of https://github.com/adoptium/temurin8-binaries/releases/download/jdk8u422-b05/OpenJDK8U-jdk_x86-32_windows_hotspot_8u422b05.zip in use - only for mingw32?

Yes that's a good idea.

Note Java 8 from Oracle seems to be the default on the Windows CIs (https://github.com/actions/runner-images/blob/main/images/windows/Windows2022-Readme.md#java) — although I might not be aware of all intricacies behind versioning of Java VMs/JDKs.

GitMensch · 2024-08-21T18:53:20Z

That's an interesting doc and we may should reset the JAVA_HOME to the 21 one for all other Windows environments then.

Changes mostly include: * New option `--with{out}-java` for `configure` script * Runtime support for calling `static void (void)` methods * Lookup of JVM library when JAVA_HOME is set, and pre-load when possible * Two preliminary tests Co-authored-by: Nicolas Berthier <[email protected]> Co-authored-by: Simon Sobisch <[email protected]>

nberth · 2024-08-22T08:47:44Z

That's an interesting doc and we may should reset the JAVA_HOME to the 21 one for all other Windows environments then.

cf #173

…ted calls

cobc/parser.y

tests/testsuite.src/run_java.at

xevor11 · 2024-08-23T13:36:57Z

There is another PR #174 which includes the non-void method handling as well

cobc/parser.y

tests/testsuite.src/run_java.at

GitMensch

The overall state is quite good!

Biggest issues:

from glancing at the code I think CALL...[NOT] ON EXCEPTION is broken (adjust the tests as requested and we'll see)
the build system need some changes
@nberth has a bit to do on the delay-load part (incl. ChangeLog entries)
if possible I'd like to see the parameter check already, but we can leave this for later as well

GitMensch · 2024-09-12T15:10:50Z

DEPENDENCIES.md

+    The JDK is distributed under various open-source licenses depending on the vendor and version. Common licenses include the GNU General Public License (GPL) and the Oracle Binary Code License Agreement.
+
+To enable JNI support, ensure that the JDK is installed on your system, and set the appropriate environment variables (e.g., JAVA_HOME) to point to the JDK installation directory.


please word-wrap as for SCREEN SECTION

GitMensch · 2024-09-12T15:12:08Z

NEWS

@@ -20,7 +20,7 @@ NEWS - user visible changes				-*- outline -*-


 * New GnuCOBOL features
-
+** Initial support for Java interoperability through JNI (new optional dependency JDK)


please adjust as for "JSON GENERATE" (just check in the same file searching for "json"):

what is available now

what is needed for support (I guess this will only be a dependency for runtime support, e. g. all checks in cobc work identical without that)

add the related configure option / flags

word-wrap your new content to col80

GitMensch · 2024-09-12T15:20:30Z

ChangeLog

+	* ax_prog_java.m4: Added macro for jni check
+	* ax_jni_include_dir.m4: Added macro for jni check


combine both m4 entries to one entry (comma separate filename, include their "m4/" prefix)

make the configure entry more specific, like the 2020-07-19 one

add something like NEWS, DEPENDENCIES: updated for JNI

GitMensch · 2024-09-12T15:25:15Z

cobc/codegen.c

This file and most others in cobc miss a ChangeLog entry

GitMensch · 2024-09-12T16:14:51Z

cobc/codegen.c

@@ -147,6 +147,7 @@ static struct literal_list	*literal_cache = NULL;
 static struct field_list	*field_cache = NULL;
 static struct field_list	*local_field_cache = NULL;
 static struct call_list		*call_cache = NULL;
+static struct call_list	*call_java_cache = NULL;


tab-indent please

GitMensch · 2024-09-12T19:40:32Z

libcob/java.c

+	if ((*env)->ExceptionCheck(env)) {
+		(*env)->ExceptionDescribe(env);


instead of ExceptionDescribe which puts the message unconditionally to stderr and implicit frees the exception, the message should be extracted and setup into last error - see call.c for the later;

no idea if there's a cleaner way (please give it a short check), but ChatGPT said we need to use JNI to get the details, giving the following example (which at least needs to be converted to C89 compat [vars+comments]:

// Find the Throwable class jclass throwableClass = (*env)->FindClass(env, "java/lang/Throwable"); // Get the getMessage() method from Throwable class jmethodID getMessageMethod = (*env)->GetMethodID(env, throwableClass, "getMessage", "()Ljava/lang/String;"); if (getMessageMethod != NULL) { // Call getMessage() on the exception object jstring message = (jstring)(*env)->CallObjectMethod(env, exception, getMessageMethod); // Convert the Java String (jstring) to a C string (char*) const char *messageChars = (*env)->GetStringUTFChars(env, message, NULL); printf("Exception message: %s\n", messageChars); (*env)->ReleaseStringUTFChars(env, message, messageChars); // Clean up } // Find java.io.StringWriter and java.io.PrintWriter classes jclass stringWriterClass = (*env)->FindClass(env, "java/io/StringWriter"); jclass printWriterClass = (*env)->FindClass(env, "java/io/PrintWriter"); // Create a new StringWriter object jobject stringWriter = (*env)->NewObject(env, stringWriterClass, (*env)->GetMethodID(env, stringWriterClass, "<init>", "()V")); // Create a new PrintWriter object that wraps the StringWriter jobject printWriter = (*env)->NewObject(env, printWriterClass, (*env)->GetMethodID(env, printWriterClass, "<init>", "(Ljava/io/Writer;)V"), stringWriter); // Call exception.printStackTrace(printWriter) to write the stack trace to the StringWriter jclass throwableClass = (*env)->GetObjectClass

instead of the convenience function ExceptionCheck use ExceptionOccurred which returns either NULL or the jthrowable we need for the code above

GitMensch · 2024-09-12T19:45:58Z

libcob/call.c

+#else
+	static int first_java = 1;
+
+	COB_UNUSED (method_handle);


instead of the unused: check if it is empty (before the #if WITH_JNI) and set the relevant exceptions then; scoping the complete#else in a separate block {}

GitMensch · 2024-09-12T19:50:37Z

cobc/codegen.c

+	output_line("cob_call_java(call_java_%s);\n", mangled);
+	output_newline();
+	output_block_close();
+}


do an exception check afterwards, as done for normal calls; also add the ON EXCEPTION/ NOT ON EXCEPTION codegen; possibly by moving those parts out of output_call() and executing them also in the 'CB_CONV_JAVA` part.

GitMensch · 2024-09-12T19:53:17Z

tests/testsuite.src/run_java.at

+AT_SETUP([CALL Java static void (void) (missing class)])
+AT_KEYWORDS([extensions jni])
+
+AT_SKIP_IF([test "$COB_HAS_JNI" = "no"])
+
+AT_DATA([prog.cob], [
+       IDENTIFICATION   DIVISION.
+       PROGRAM-ID.      prog.
+       PROCEDURE        DIVISION.
+           CALL "Java.Test.isAMissingClass"
+           STOP RUN.
+])
+
+AT_CHECK([$COMPILE prog.cob], [0], [], [])
+AT_CHECK([$COBCRUN_DIRECT ./prog], [1], [],
+[libcob: prog.cob:5: error: Java class 'Test' not found
+])
+AT_CLEANUP


duplicate this test with ON EXCEPTION and DISPLAY "java call not successful" in there; this duplicated test also doesn't need to be skipped if we don't have JNI as it should run in the same exception

GitMensch · 2024-09-12T20:09:43Z

tests/testsuite.src/run_java.at

+       PROGRAM-ID.      prog.
+       PROCEDURE        DIVISION.
+           CALL "Java.Test.printHelloWorld"
+           STOP RUN.


add NOT ON EXCEPTION DISPLAY "Java call worked" to this call

GitMensch · 2024-09-14T17:28:40Z

I've got word from the original author of the java related macros used and that's (part of) his answer:

I'm afraid I cannot help and that it would be better to make these macros as deprecated/abandoned.

I'm likely to adjust the part configure for that (but that likely takes some days until I get to this), @nberth can you please check the review related to the delay-load part and @xevor11 could you please check on the rest?

xevor11 marked this pull request as draft July 9, 2024 13:31

xevor11 marked this pull request as ready for review July 9, 2024 13:32