Samsung: dynamic support for different native changes?

[wanam]

Hi @rovo89,

This is a question rather than a bug report.

We have a new issue with Samsung pushing 2 different native changes for its Marshmallow updates, most of the devices have only one native change https://github.com/wanam/android_art/commit/3e5b9ff8c8bcec3d3819f7371cbe38d9863a9cc4 , but few other devices (N910P/G and N915F) come with two more (old) changes https://github.com/wanam/android_art/commit/ddda1ab2313ff94f66bd83489ec7c26949cfbc9a and https://github.com/wanam/android_art/commit/0705ffac83d797d6e2da68bfdd398c446ef7a424 , so i'm currently handling this with two branches.

My question is there is anyway i can make only one build version that supports all those changes? i mean a way to detect at the linker level (or maybe before) if we need to register a new native method or declare a new fields.

Thanks.

[rovo89]

i mean a way to detect at the linker level (or maybe before) if we need to register a new native method or declare a new fields.

Why that early and outside of ART's control? Registering a native method only if it's defined in Java shouldn't be a big issue.

And regarding the fields... Well, you somehow need a condition. The commits I gave you already contain a check to adjust the offsets only on Samsung ROMs. If this isn't enough, it should be fairly simple to use a more specific condition as long as you can detect which variant to use. Ideally, the offsets would be retrieved from the Java definition, but I'm not sure about the effort and whether it's possible for such low-level classes as DexCache.

[wanam]

Ideally, the offsets would be retrieved from the Java definition, but I'm not sure about the effort and whether it's possible for such low-level classes as DexCache.

That's what I'm looking for, any idea how i can achieve this or where i can start looking?

[rovo89]

ART includes many test cases. I would first look for one which compares the offsets from the Java side to those on the native side. I'm almost sure that something like this exists.

[Liliniser]

@wanam If you're still looking for a way, please have a look at https://github.com/Liliniser/android_art/commit/f69c55659a7906480e4c873d285a9416d1ef7463. I also wanted to merge separated branches into one but as I'm not a skilled c/c++ programmer, this is the best that I could think of.

Now I'm afraid that my code could have logical errors or potential issue so I wish @rovo89 or @wanam could review it.

I tested this with a deodexed rom switching a couple of 'core-libart.jar's and seemed to work fine.

Thank you

[wanam]

@Liliniser Nice work!

I have some questions/suggestions: 1- Did you try to insert your workaround here (https://github.com/Liliniser/android_art/blob/f69c55659a7906480e4c873d285a9416d1ef7463/runtime/class_linker.cc#L433) instead of here (https://github.com/Liliniser/android_art/blob/f69c55659a7906480e4c873d285a9416d1ef7463/runtime/class_linker.cc#L456)? This would save you an extra call "java_lang_DexCache->SetObjectSize(mirror::DexCache::InstanceSize());" 2- Add a "break;" here (https://github.com/Liliniser/android_art/blob/f69c55659a7906480e4c873d285a9416d1ef7463/runtime/class_linker.cc#L302) since we don't need to check remaining fields. 3- Do we still need to keep the check "IsSamsungROM()"?

I admit i'm not so familiar with c++ development, so i hope @rovo89 could review the code, i will give it a try this weekend anyway.

[Liliniser]

@wanam

1. That's indeed a better place but it still needs an iteration over 'boot_class_path' after all, doesn't it?
2. Thank you for the catch!
3. Well.. maybe.. not?

I fixed the commit based on your suggestion. But regarding question no. 1, I'm not sure at the moment. Thank you

[wanam]

Nice, i just tried it on a GS7 (G930F - Stock TW 6.0.1) and a Oneplus3 (CM 13) with no issues, i made a few other changes to your commit: 1- Removed this block (https://github.com/Liliniser/android_art/commit/70fc6d94fd3215a237f980b6b3c9b388fa2b1fee#diff-b57f4c04ee2b7d3f2a2a3d0abbee3e35R1167) and moddified the bellow loop:

  CHECK_EQ(oat_file.GetOatHeader().GetDexFileCount(),
           static_cast<uint32_t>(dex_caches->GetLength()));

  for (int32_t i = 0; i < dex_caches->GetLength(); i++) {
    StackHandleScope<1> hs2(self);
    Handle<mirror::DexCache> dex_cache(hs2.NewHandle(dex_caches->Get(i)));
    const std::string& dex_file_location(dex_cache->GetLocation()->ToModifiedUtf8());
    const OatFile::OatDexFile* oat_dex_file = oat_file.GetOatDexFile(dex_file_location.c_str(),
                                                                     nullptr);
    CHECK(oat_dex_file != nullptr) << oat_file.GetLocation() << " " << dex_file_location;
    std::string error_msg;
    std::unique_ptr<const DexFile> dex_file = oat_dex_file->OpenDexFile(&error_msg);
    if (dex_file.get() == nullptr) {
      LOG(FATAL) << "Failed to open dex file " << dex_file_location
                 << " from within oat file " << oat_file.GetLocation()
                 << " error '" << error_msg << "'";
      UNREACHABLE();
    } else {
      dexCacheExtraFieldsWorkaround(dex_file.get());
    }

    if (kSanityCheckObjects) {
      SanityCheckArtMethodPointerArray(dex_cache->GetResolvedMethods(), nullptr,
                                       image_pointer_size_, space);
    }

2- Removed this check https://github.com/Liliniser/android_art/blob/xposed-marshmallow/runtime/native/samsung.cc#L22 since failed method registering is ignored.

It still needs more testing to avoid any regression.

[Liliniser]

@wanam Didn't it crash on this line? I mean, it should've crashed on alt-devices?

const std::string& dex_file_location(dex_cache->GetLocation()->ToModifiedUtf8());

Precisely, dex_cache->GetLocation() calls DexCache::LocationOffset() and at this moment, sDexCacheJavaClassHasExtraFields is set to false. That's why I put the block before the loop, only checking core-libart.jar once.

Correct me if I missed something.

FIY, here is a crash log `* * * * * * * * * * * * * * * * Build fingerprint: 'samsung/graceltexx/gracelte:6.0.1/MMB29K/N930FXXU1APG7:user/release-keys' Revision: '0' ABI: 'arm' pid: 331, tid: 331, name: main >>> zygote <<< signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x8 r0 bea96a2c r1 00000000 r2 b3ebc918 r3 b3ebf350 r4 bea96a2c r5 bea96a20 r6 00000000 r7 b4ba4f00 r8 b4b47500 r9 00000010 sl b4ba4f00 fp 70b48604 ip 00000000 sp bea96988 lr b3bb0eef pc b3d2224c cpsr 600f0030

backtrace:

00 pc 002a424c /system/lib/libart.so (art::mirror::String::ToModifiedUtf8()+11)

#01 pc 00132eeb  /system/lib/libart.so (art::ClassLinker::InitFromImage()+574)
#02 pc 0031c3e9  /system/lib/libart.so (art::Runtime::Init(std::__1::vector<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, void const_>, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, void const_> > > const&, bool)+4816)
#03 pc 0031e07d  /system/lib/libart.so (art::Runtime::Create(std::__1::vector<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, void const_>, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, void const_> > > const&, bool)+668)
#04 pc 0024d5f3  /system/lib/libart.so (JNI_CreateJavaVM+526)
#05 pc 000633d9  /system/lib/libandroid_runtime.so (android::AndroidRuntime::startVm(_JavaVM**, _JNIEnv**, bool)+4816)
#06 pc 0006359f  /system/lib/libandroid_runtime.so (android::AndroidRuntime::start(char const*, android::Vectorandroid::String8 const&, bool)+258)
#07 pc 00004ca7  /system/bin/app_process32_xposed (main+790)
#08 pc 00017419  /system/lib/libc.so (__libc_init+44)
#09 pc 00004e54  /system/bin/app_process32_xposed (_start+96)

Thank you

[wanam]

Correct, i tested only on a devices having "sDexCacheJavaClassHasExtraFields=false", that's why i didn't notice it.

[rovo89]

It's hard for me to review as I don't have the time to investigate possible solutions myself.

Maybe the analysis of the class could be done when it's being linked, or is that too late?

Also, is there a way to maybe retrieve the actual offsets for the fields from the class instead of merely detecting whether the exist? Then it wouldn't be necessary to statically add 4/8, but the offsets could be returned from memory (e.g. a struct that is filled during analysis).

As for the native method registering, I think it would be better to use the return_errors parameter: https://github.com/rovo89/android_art/blob/93eccd115adcc11e65238e5d31a9b9e6fc733287/runtime/jni_internal.cc#L2080 Or maybe add a prefix to the method signature declaration that signals the special behavior, similar to fast JNI: https://github.com/rovo89/android_art/blob/93eccd115adcc11e65238e5d31a9b9e6fc733287/runtime/jni_internal.cc#L2140 That might conflict with the official JNI specs though, so it should probably be restricted to boot image classes (also to avoid that app developers use this "feature").