Samsung: dynamic support for different native changes? about android_art HOT 10 OPEN

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.

from android_art.

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?

from android_art.

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.

from android_art.

@wanam
If you're still looking for a way, please have a look at Liliniser@f69c556.
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

from android_art.

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

from android_art.

@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

from android_art.

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 (Liliniser@70fc6d9#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.

from android_art.

@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)
#1 pc 00132eeb /system/lib/libart.so (art::ClassLinker::InitFromImage()+574)
#2 pc 0031c3e9 /system/lib/libart.so (art::Runtime::Init(std::__1::vector<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits, std::_1::allocator >, void const>, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits, std::_1::allocator >, void const> > > const&, bool)+4816)
#3 pc 0031e07d /system/lib/libart.so (art::Runtime::Create(std::__1::vector<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits, std::_1::allocator >, void const>, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits, std::_1::allocator >, void const> > > const&, bool)+668)
#4 pc 0024d5f3 /system/lib/libart.so (JNI_CreateJavaVM+526)
#5 pc 000633d9 /system/lib/libandroid_runtime.so (android::AndroidRuntime::startVm(_JavaVM**, _JNIEnv**, bool)+4816)
#6 pc 0006359f /system/lib/libandroid_runtime.so (android::AndroidRuntime::start(char const*, android::Vectorandroid::String8 const&, bool)+258)
#7 pc 00004ca7 /system/bin/app_process32_xposed (main+790)
#8 pc 00017419 /system/lib/libc.so (__libc_init+44)
#9 pc 00004e54 /system/bin/app_process32_xposed (_start+96)

Thank you

from android_art.

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

from android_art.

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:

from android_art.