2

I am following the Google tips to implement a JNI layer between my Android app and my C++ library. It suggests to use the following code to register native methods when the library is loaded:

JNIEXPORT jint JNI_OnLoad(JavaVM* vm, void* reserved) {
    JNIEnv* env;
    if (vm->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6) != JNI_OK) {
        return JNI_ERR;
    }

    ...

    // Register your class' native methods.
    static const JNINativeMethod methods[] = {
        {"nativeFoo", "()V", reinterpret_cast<void*>(nativeFoo)},
        {"nativeBar", "(Ljava/lang/String;I)Z", reinterpret_cast<void*>(nativeBar)},
    };
    int rc = env->RegisterNatives(c, methods, sizeof(methods)/sizeof(JNINativeMethod));
    ...
}

I am quite new to C++ so I decided to use clang-tidy to ensure my C++ code is modern and safe. clang-tidy reports:

error: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast,-warnings-as-errors]

According to the clang-tidy documentation:

cppcoreguidelines-pro-type-reinterpret-cast

This check flags all uses of reinterpret_cast in C++ code.

Use of these casts can violate type safety and cause the program to access a variable that is actually of type X to be accessed as if it were of an unrelated type Z.

This rule is part of the “Type safety” profile of the C++ Core Guidelines, see https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Pro-type-reinterpretcast.

So I have a few options:

  1. Disable this check and risk using reinterpret_cast inappropriately elsewhere
  2. Ignore the check everywhere where I need to use it and create a messy codebase
  3. Find some alternative way of implementing this more safely

I would like to do 3 if it's possible but I'm not quite sure where to start.

jsa
  • 1,050
  • 10
  • 22
  • 1
    JNI is C API. There is no way around it. You can encapsulate casts into some wrapper, or find some existing ones, but in the end someone somewhere will need to do the cast. Pretty sure clang-tidy allows to enable/disable specific warning on per file basis. Is this an acceptable compromise for you? – Dan M. Mar 31 '20 at 14:20
  • What is the signature of `JavaVM`’s `GetEnv` “method”? – Davis Herring Mar 31 '20 at 14:54
  • There's always the option of not using `RegisterNatives`, and instead name your functions so that they can be found by the VM (e.g. `JNIEXPORT void JNICALL Java_com_foo_Bar_nativeBar`). – Michael Mar 31 '20 at 17:20
  • @DanM. Thanks. As far as I understand, clang-tidy allows you to disable the check on a per-file basis but I am currently using code like this in several files to split my JNI code up. So for now the simplest and most flexible workaround for me is to just disable the check altogether. – jsa Apr 02 '20 at 11:47
  • @DavisHerring The signature is `jint GetEnv(JavaVM *vm, void **env, jint version);` – jsa Apr 02 '20 at 11:47
  • @Michael I prefer to use `RegisterNatives` because, according to the docs I linked: `The advantages of RegisterNatives are that you get up-front checking that the symbols exist, plus you can have smaller and faster shared libraries by not exporting anything but JNI_OnLoad.` – jsa Apr 02 '20 at 11:47

1 Answers1

0

It’s not clear why GetEnv wants a void** rather than a JNIEnv** (what other type would you use?), but you can avoid the reinterpret_cast and the concomitant undefined behavior(!) with a temporary variable:

void *venv;
if (vm->GetEnv(&venv, JNI_VERSION_1_6) != JNI_OK) {
    return JNI_ERR;
}
const auto env=static_cast<JNIEnv*>(venv);

Note that a static_cast from void* is every bit as dangerous as a reinterpret_cast, but its use here is correct, unavoidable, and shouldn’t produce linter warnings.

In the function pointer case there’s nothing to be done: reinterpret_cast is the only, correct choice (and for void* rather than some placeholder function pointer type like void (*)() its correctness is not guaranteed by C++, although it works widely and POSIX does guarantee it). You can of course hide that conversion in a function (template) so that the linter can be told to ignore only it, but make sure to use a clear name to avoid “hiding” the conversion.

Davis Herring
  • 36,443
  • 4
  • 48
  • 76
  • Thank you, this does eliminate the lint warning in that instance and now I understand a bit more about what is happening in the code too. For anyone interested, I found this SO answer relevant https://stackoverflow.com/a/310489/6449273 Do you know if it's possible to resolve the other instances of the warning in the code (casting a function pointer to void * e.g. reinterpret_cast(nativeFoo) )? I suspect it is not possible after reading this https://stackoverflow.com/q/36645660/6449273 – jsa Apr 03 '20 at 11:32
  • @jsa: You’re right; I edited to summarize. Sorry that I didn’t even notice the second `reinterpret_cast` at first! – Davis Herring Apr 03 '20 at 14:29