Open therealkenc opened 1 year ago
Thanks for taking a look at this and for the detailed context in your bug. Also, please be aware of https://github.com/google/jni-bind/issues/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?
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.
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>
).
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
.
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
.]
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.
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.
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,
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
I got jni-bind working on Windows but needed to work-around a difference in how Oracle JDK 19 defines
jint
.On Linux:
On Windows (comment from the JDK preserved):
If I compile on Windows, I get TMP errors:
Full gist here.
I'm not above changing the
typedef
in the JDK, but if it is possible to make jni-bind tolerant of the types encapsulated byjni_md.h
that would be even better.Either way, for my own edification, what I don't entirely grok is why the template specialization is failing to begin with. I don't see it. The
jlong
type is on Windows is:There is no actual difference in the types on Windows and Linux. The types are unique (32-bit and 64-bit literals). Yet
AllUnique<>
fails.