Bug 104642

I haven't yet found a proper fix bug here's a workaround you can consider to use:

https://github.com/intel/external-mesa/commit/df502da74998b45fbdd82455c7c9c98242db6d86

The problem is with the address comparison check in build_id callback (build_id_find_nhdr_callback). This comparison fails on Android when running 32bit. One alternative would be to always add '0x8000' to dlpi_addr during comparison on 32bit but that seems a bit weird.

Regarding the length, I've been also considering if we are missing required build flags somewhere. We are setting it currently in these places:

src/intel/Android.vulkan.mk:LOCAL_LDFLAGS += -Wl,--build-id=sha1
Android.mk:MESA_DRI_LDFLAGS := -Wl,--build-id=sha1

It *should* be enough I think .. but maybe not.

Hi Tapani,

Thanks a lot for sharing the workaround

It seams a regression introduced by 5c98d3825ccbed9054a1bb2de607116b2b31d48b "util: Query build-id by symbol address, not library name".
Is Chad Versace already having a look?

In the former coding of build_id_find_nhdr_callback() there was a comment:

-   /* The first object visited by callback is the main program.
-    * Android's libc returns a NULL pointer for the first executable.
-    */
-   if (info->dlpi_name == NULL)
-      return 0;

and NULL was checked; does last return 0 mean that nothing was done for Android, if Android libc returns systematically NULL?

In any case, I think code should also be robust to unconformant libraries and should not crash.

Just a question for my knowledge, where does the add '0x8000' to dlpi_addr during comparison on 32bit comes from, is it due to some "Android thing"?

Mauro

(In reply to Mauro Rossi from comment #3)
> Hi Tapani,
> 
> Thanks a lot for sharing the workaround
> 
> It seams a regression introduced by 5c98d3825ccbed9054a1bb2de607116b2b31d48b
> "util: Query build-id by symbol address, not library name".
> Is Chad Versace already having a look?

As far as I know, no.
 
> In the former coding of build_id_find_nhdr_callback() there was a comment:
> 
> -   /* The first object visited by callback is the main program.
> -    * Android's libc returns a NULL pointer for the first executable.
> -    */
> -   if (info->dlpi_name == NULL)
> -      return 0;
> 
> and NULL was checked; does last return 0 mean that nothing was done for
> Android, if Android libc returns systematically NULL?

This callback gets called for binary itself + libraries. This NULL check is not needed anymore since address check is better guard (if it worked ..). It works fine on desktop, both 32bit and 64bit. On Android it also works fine on 64bit.
 
> In any case, I think code should also be robust to unconformant libraries
> and should not crash.

Agreed, but if it's a linker bug I think then we want to get it fixed in bionic as well. Let's try to avoid duct-taping if possible.
 
> Just a question for my knowledge, where does the add '0x8000' to dlpi_addr
> during comparison on 32bit comes from, is it due to some "Android thing"?

That seems to be the amount of offset there is so that comparison will work. I haven't figured out why this is.

This commit should fix it properly: https://github.com/me176c-dev/android_external_mesa/commit/07b637bcef660b3bf12d6854153692026ef3c698

Can you test this patch, Mauro?

The main problem is that the build-id code doesn't handle the (unusual) case that there is an offset between the base address and the LOAD segment (as indicated by the virtual address in the ELF header). On Android 32-bit, the offset comes from the "relocation_packer"[1] in the Android build system. It seems to re-pack the binary and adds the offset for whatever reason.

The ELF header tells us the offset so we just need to look through the header and add the offset to the base address (dlpi_addr) before comparing it to dli_fbase.

[1] https://android.googlesource.com/platform/bionic/+/master/tools/relocation_packer

Hi Stephan,

with the patch mentioned in comment #5
the problem is resolved, Chrome app and Firefox app are not crashing anymore.

Please proceed in having the fix merged in mesa 18.0 cycle

Thanks
Mauro

Pushed to master. It should find its way to the stable releases in due time.

commit 02e2009b929a0f101540b9b55c5f0ed859d1b3be
Author: Stephan Gerhold <stephan@gerhold.net>
Date:   Wed Jan 24 15:13:24 2018 +0100

    util/build-id: Fix address comparison for binaries with LOAD vaddr > 0