jni_mh.h with jint as 32-bit long about jni-bind

[jwhpryor]

Thanks for taking a look at this and for the detailed context in your bug. Also, please be aware of #197 which first mentions this.

I believe the issue is arising here:
https://github.com/google/jni-bind/blob/main/implementation/proxy.h#L53

proxy.h and its counterpart proxy_definitions.h describe the mappings that JNI Bind uses to map the declaration (i.e. Method<jlong>{}) to what the argument lookup should be (e.g. jlong).

Unfortunately, if these two types are the same, it's going to complicate what that declaration should look like. The compiler won't be able to differentiate between the two. i.e.

typedef  int A ;
typedef int B  ;

static_assert(std::is_same_v<A, B>);  // compiler treats as identical.

On the one hand, it's good JNI Bind catches this (it is a goal to prevent widening or shortening, real bugs that have occurred on non-JNI Bind code I've worked on), on the other, it's a shame that this is a deficiency.

I'm not sure what's right here. I think a syntax like follows makes sense:

static constexpr jni::Class { "kClass",
  jni::Method { "intM",  jni::Return<void>{}, Params { jint{} } },
  jni::Method { "longM",  jni::Return<void>{}, Params { Type<jlong>{} } },
  jni::Method { 
       "overloadM", 
       jni::Overload { jni::Return<void>{}, Params { Type<jint>{} } },
       jni::Overload { jni::Return<void>{}, Params { Type<jlong>{} } },
  },
};

LocalObject<kClass> obj{};
obj("intM", 1);  // Fine
obj("intM", jlong{1}); // Impossible to prevent, won't compile on Linux, doesn't shorten or widen
// obj("intM", jlong{BIG_VAL}); // Compiles *nowhere*, would cause shortening.
obj("intM", jint{1}); // Compiles everywhere.

obj("longM", 1);  // Impossible to prevent, compiles everywhere.
obj("longM", jlong{1}); // Compiles everywhere.
obj("longM", jlong{BIG_VAL}); // Compiles Linux only, Windows complains of shortening.

// obj("overloadM", 1);  // FAILS on *Windows only*, ambiguous.
obj("overloadM", jint{1});  // FAILS on *Windows only*, ambiguous.
// obj("overloadM", jlong{1}); // FAILS on *Windows only*, ambiguous.
// obj("overloadM", jlong{BIG_VAL}); // FAILS on *Windows only*, ambiguous.

I actually am not sure what the best way around this is. I'm definitely open to a different syntax than above, but, JNI Bind relies heavily on making inferences from types. If those types become ambiguous, thins become tricky. The above is basically forcing no implicit conversions.

I'd also be open to having some kind of "ambiguity ranking" where things work without ambiguity, but anything ambiguous that isn't the default (e.g. int vs long) requires being explicit.

What do you think?

[therealkenc]

I think AllKeys is the culprit too, but I don't get entirely what is causing the collision, since on llvm/clang x86_64-pc-windows-msvc, jint is a 32-bit long and jlong is __int64.

You could do a syntax like Type<>. My gut says in those examples with literals, if you really want jlong, you need to decorate with a C-language LL, because in Java, a long is a 64-bit integer.

Ideally this code should compile as-is without further decoration.

JNIEXPORT void JNICALL Java_com_coralblocks_javatocppandback_jni_1bind_HelloWorld_goToNativeSide
  (JNIEnv *env, jclass klass, jint x, jstring msg) {
   static constexpr jni::Class kClass {
      "com/coralblocks/javatocppandback/jni_bind/HelloWorld",
      jni::Method {
          "sayHello",
          jni::Return < void > {},
          jni::Params < jint , jstring> {}
      }
    };
    jni::LocalObject < kClass > helloWorld {};
    helloWorld("sayHello", x, msg);
}

Only one method matches on all platforms, regardless of how the jdk typedefs jint. If you're saying that needs to be jni::Params < Type<jint> , jstring> then I suspect everyone will just end up using that as the defacto syntax.

[jwhpryor]

Hmm that's strange you're seeing that collision there if the types are different. Wasn't the entire issue that it was being defined as 32 bit but only on Windows? Are you sure it's the same two types colliding?

----- edited to remove a minor confusion I had

I agree that using Type everywhere would become the "defacto syntax". I think this would be bad because the syntax that's currently so widely used is more terse (i.e. better).

The problem is, that if there's a platform where jint and jdouble are indistinguishable, there's no way JNI Bind knows what method signature to use, because you don't know if Params has jint of jdouble.

Perhaps if some arbitrary order is imposed on jint and jdouble. If you use it on Linux, it just works (like normal), but if you use it on Windows you have to say Type<jdouble> if you want jdouble, but not for jint.

This has the downside that folks can now compile something that basically doesn't work (if you pass an int and you incorrectly use jdouble in your definition, you will lookup the int method). The upside is that we can warn folks if they use a double with int declared (when they meant to say Type<jdouble>).

[therealkenc]

32 bit but only on Windows

No, jint is 32-bit on WIndows and 32-bit on Linux. Because Java int is 32-bits everywhere in the universe. The difference is Linux defines jint as:

typedef int jint;

And Windows defines it as:

// 'long' is always 32 bit on windows so this matches what jdk expects
typedef long jint;

If I take a big hammer and make the typedef to a C int on Windows, all is well with jni-bind.

// 'long' is always 32 bit on windows so this matches what jdk expects
typedef int jint; // local change long -> int  --KC

Nothing material changes, because both C long and C int are 32-bit on llvm/clang x86_64-pc-windows-msvc.

But without that change, jni-bind fails to compile with the gist from earlier.

In all universes, both Linux and Windows, this code below will assert regardless of whether it is jint is C long or C int. Because of course it does. A Java long is 64-bit on all platforms.

#include <type_traits>
#include <jni.h>

/*  Linux
        JAVA_HOME="/usr/lib/jvm/java-19-openjdk-amd64"
    Windows:
        JAVA_HOME="C:/Program Files/Java/jdk-19"
 */
int main(int argc, char *argv[])
{
    // static_assert(false) in known universe
    static_assert(std::is_same_v<jlong, jint>);
}

The problem is, that if there's a platform where jint and jdouble are indistinguishable

There is no platform where jint and jlong (sic) are indistinguishable. They are not the same. What does happen is jni-bind fails to compile if jint is typedef long and not int.

[therealkenc]

I took this a little further, but couldn't take it home. Some ascii art annotations on the error message inline commented with AllKeys from proxy.h:

// error: static assertion failed due to requirement 
// 'AllUnique_v<void, unsigned char, bool, signed char, short, long, 
//                 jboolean^  jboolean?^    jbyte^  jshort^      ^<- that's the jint on x86_64-pc-windows Oracle JDK 19
//    float, long, long long, char, unsigned short, double, std::string, _jstring *, char *, const char *, std::string_view, 
// jfloat^    |      |     jbyte^      jchar^   jdouble^      
//     wut?!->^      ^<- jlong everywhere
//    jni::RefBaseTag<_jstring *>, _jobject *, 
//    jni::RefBaseTag<_jobject *>, jni::LoaderTag, jni::Object, jni::Self, _jarray *, 
//    jni::RefBaseTag<_jarray *>, jni::ArrayTag<_jarray *>, _jobjectArray *, jni::RefBaseTag<_jobjectArray *>, 
//    jni::ArrayTag<_jobjectArray *>, _jintArray *, jni::RefBaseTag<_jintArray *>, jni::ArrayTag<_jintArray *>, 
//    _jbooleanArray *, jni::RefBaseTag<_jbooleanArray *>, jni::ArrayTag<_jbooleanArray *>, _jbyteArray *, 
//    jni::RefBaseTag<_jbyteArray *>, jni::ArrayTag<_jbyteArray *>, _jcharArray *, jni::RefBaseTag<_jcharArray *>, 
//    jni::ArrayTag<_jcharArray *>, _jshortArray *, jni::RefBaseTag<_jshortArray *>, jni::ArrayTag<_jshortArray *>, 
//    _jdoubleArray *, jni::RefBaseTag<_jdoubleArray *>, jni::ArrayTag<_jdoubleArray *>, _jfloatArray *, 
//    jni::RefBaseTag<_jfloatArray *>, jni::ArrayTag<_jfloatArray *>, _jlongArray *, 
//    jni::RefBaseTag<_jlongArray *>, jni::ArrayTag<_jlongArray *>>': FindIdxOfVal only operates on unique sets.

// CDecls for all declarable types (these index into proxy definitions).
using AllKeys =
    Corpus_t<JniUserDefinedCorpusTag, void, jboolean, jbyte, jshort, jint,
             jfloat, jlong, jchar, jdouble, jstring, jobject, Self, jarray,
             jobjectArray, jintArray, jbooleanArray, jbyteArray, jcharArray,
             jshortArray, jdoubleArray, jfloatArray, jlongArray>;

If we can explain where that "extra" long is coming from in the template pack, we've explained the problem. I went down the rabbit hole of how the whole pack is constructed, but alas as of this writing cannot. It is not coming from <include/win32/jni_mh.h>. That supplies the first long (for jint) and the long long (for jlong).

[*edit I noticed after I posted that jchar and jbyte were incorrect, and in fixing that, discovered that a jboolean is unsigned char.]

[therealkenc]

Here is the culprit:

template <typename LongType>
struct Proxy<LongType,
             typename std::enable_if_t<std::is_same_v<LongType, jlong>>>
    : public ProxyBase<jlong> {
  using AsArg = std::tuple<long, jlong>;
  using AsDecl = std::tuple<long, jlong>;

  template <typename OverloadSelection, typename T>
  static constexpr bool kViable = IsConvertibleKey<T>::template value<long> ||
                                  IsConvertibleKey<T>::template value<jlong>;

  static jlong ProxyAsArg(jlong val) { return val; }

  // jlong is a smaller type on ARM than x86.
  // When jlong is not equivalent, we upcast to the wider type.
  template <typename T,
            typename = std::enable_if_t<std::is_same_v<T, long> &&
                                        !std::is_same_v<jlong, long>>>
  static jlong ProxyAsArg(T val) {
    return jlong{val};
  }
}

Which is probably what you were trying to say earlier. A jlong (aka Java long aka Java Long) is a C long long everywhere.

As to the Type<> syntax, sure, I guess, if you think that will address the problem. Whatever solution so long as the below compiles everywhere, as it is not ambiguous anywhere.

extern "C" JNIEXPORT void JNICALL Java_com_example_jnicalc_JniTest_greet(
    JNIEnv*, jclass, jstring jmsg, jobject jcallback)
{
    static constexpr jni::Class kClass{ 
        "com/example/jnicalc/JniTest",
        jni::Method{
            "hello",
            jni::Overload{ jni::Return<void>{}, jni::Params{ jint{}, jlong{} } },
            jni::Overload{ jni::Return<void>{}, jni::Params{ jlong{}, jint{} } },
        } 
    };
    jni::LocalObject<kClass> obj{};
    jint one{1};
    jlong two{2};
    obj("hello", one, two);
    obj("hello", two, one);
}

> Task :CalcApplication.main()
hello (int, long) -> (1, 2)
hello (long, int) -> (2, 1)
leaving

Right now this fails to compile.

[jwhpryor]

Thanks for the super detailed bug descriptions. I think I understand what's up, it may take me a bit to get a fix in place, I'll try to get back ASAP.

[jwhpryor]

Apologies for taking so long, I did try to dig into this further this weekend but frustratingly, getting a setup to repro the issue has been tricky.

Long story short, I tried forcing the types as you've described above, but couldn't repro on my Linux box, probably because of subtleties with other types. I then tried to put together a Windows box with MSVC, but realized I was wasting time putting together environment so I gave up, and tried to compile JNI Bind with Godbolt.

This got further, but I started to run into independent issues just compiling JNI Bind at all (and the issues are occurring well before the above failure). E.g. ValsEqual doesn't even work with MSVC: Godbolt (code is slightly reduced for brevity). This looks like a compiler bug to me.

I've been slowly knocking out these other issues (they will need to be fixed anyways), but, what are you compiling JNI Bind with so that it get's close on Windows?

My gut tells me that adding Proxy definition for int which conditionally enables an additional Proxy definition for otherly sized ints is maybe going to help (I think the AllUnique is a red herring), but it's hard for me because as I said I'm struggling with repro.

Sorry for the delay,

[therealkenc]

E.g. ValsEqual doesn't even work with MSVC.

It won't compile with MSVC. Environment is llvm/clang x86_64-pc-windows-msvc aka clang-cl.exe. You can get it from the llvm-project here, ie LLVM-17.0.2-win64.exe.

My local fix for what it is worth, is this at here:

#ifndef _MSVC_LANG
template <typename LongType>
struct Proxy<LongType,
             typename std::enable_if_t<std::is_same_v<LongType, jlong>>>
    : public ProxyBase<jlong> {
  using AsArg = std::tuple<long, jlong>;
  using AsDecl = std::tuple<long, jlong>;

  template <typename OverloadSelection, typename T>
  static constexpr bool kViable = IsConvertibleKey<T>::template value<long> ||
                                  IsConvertibleKey<T>::template value<jlong>;

  static jlong ProxyAsArg(jlong val) { return val; }

  // jlong is a smaller type on ARM than x86.
  // When jlong is not equivalent, we upcast to the wider type.
  template <typename T,
            typename = std::enable_if_t<std::is_same_v<T, long> &&
                                        !std::is_same_v<jlong, long>>>
  static jlong ProxyAsArg(T val) {
    return jlong{val};
  }
};
#endif