Initial JNI configuration

[xevor11]

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

[nberth]

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.

[nberth]

This file should not be committed.

[nberth]

Everything under po should not be committed either.

[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]

minor formatting: use a tab before the *

[GitMensch]

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

[GitMensch]

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

[GitMensch]

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]

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]

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.

[GitMensch]

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]

these commented line and the one above should be dropped

[GitMensch]

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]

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

[nberth]

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

[GitMensch]

please indent on the left with tab

[GitMensch]

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]

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]

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

[GitMensch]

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().

[nberth]

@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).

[GitMensch]

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]

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?

[xevor11]

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

[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]

please word-wrap as for SCREEN SECTION

[GitMensch]

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]

• 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]

This file and most others in cobc miss a ChangeLog entry

[GitMensch]

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]

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]

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]

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]

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

[GitMensch]

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?

[nberth]

@xevor11 I think you have started moving this check and the one below back into cb_check_conformance; so I'll stop pushing changes to this branch to let you advance with that.