Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i#2154 Android64: Adjust loader to remove 'CURIOSITIES' #7205

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

i#2154 Android64: Adjust loader to remove 'CURIOSITIES' #7205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,8 +448,11 @@ elseif (APPLE AND X64)
# Set to higher than _PAGEZERO which is [0..0x1'00000000).
set(preferred_base "0x171000000" CACHE STRING "Preferred library base address")
elseif (ANDROID64)
# 64-bit Android needs a base of 0x1000 so that libdynamorio.so
# does not get overwritten by the vDSO.
# i#7195: 64-bit Android needs a base of 0x1000 so that the ELF header is
# positioned immediately before the .text portion of the file.
# It seems that for Android, this value is an offset from the start of the
# .text portion rather than a proper address, so the ELF header will be
# placed 0x1000 bytes before the start of the .text portion.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_preferred_base_start_and_end() for lld (the new Android linker, right?) is passing -Ttext so it's not setting the preferred base of the header segment but only the text segment. This is why there is this discrepancy between the preferred "base" and the header, and why a gap is allowed.

What does readelf -l say on libdynamorio.so for the different values here? Presumably it matches the runtime behavior and shows the gap between the header and text segments.

Even if there were a different way to set the header segment base (--section-start with a blank name??) it may not help if the kernel ignores it and always places it up high in the address space: so maybe this -Ttext=0x1000 is the best solution, and maybe -Ttext=0 is ignored or something (presumably readelf -l on that shows a big gap?).

Please update the comment here to make it clear this is not really the header base but instead the text segment base only, and the linker puts a big gap if left at default or set to 0, which is why we set it to 0x1000.

set(preferred_base "0x1000" CACHE STRING "Preferred library base address")
else ()
set(preferred_base "0x71000000" CACHE STRING "Preferred library base address")
Expand Down
6 changes: 6 additions & 0 deletions core/lib/globals_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,12 @@ typedef struct _instr_t instr_t;
# define IF_NOT_ANDROID(x) x
#endif

#ifdef ANDROID32
# define IF_NOT_ANDROID32(x)
#else
# define IF_NOT_ANDROID32(x) x
#endif

#ifdef X64
# define IF_X64(x) x
# define IF_X64_ELSE(x, y) x
Expand Down
11 changes: 8 additions & 3 deletions core/unix/module_elf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,10 +406,15 @@ module_walk_program_headers(app_pc base, size_t view_size, bool at_map, bool dyn
LOG(GLOBAL, LOG_INTERP | LOG_VMAREAS, 2,
"%s " PFX ": %s dynamic info\n", __FUNCTION__, base,
out_data->have_dynamic_info ? "have" : "no");
/* i#1860: on Android a later os_module_update_dynamic_info() will
* fill in info once .dynamic is mapped in.
/* i#1860: on 32-bit Android a later
* os_module_update_dynamic_info() will fill in info
* once .dynamic is mapped in.
* i#XXX: This is not needed on newer versions of 64-bit
* Android, however we are not able to test with newer
* versions of 32-bit Android, so this may still be
* required.
*/
IF_NOT_ANDROID(ASSERT(out_data->have_dynamic_info));
IF_NOT_ANDROID32(ASSERT(out_data->have_dynamic_info));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not ASSERT_CURIOSITY: this is a regular fatal assert. I don't see any ASSERT_CURIOSITY being changed here: please update the PR title and description as it talks about CURIOSITIES which does not match the PR changes,

}
});
}
Expand Down
5 changes: 4 additions & 1 deletion core/unix/os.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8482,9 +8482,12 @@ mmap_check_for_module_overlap(app_pc base, size_t size, bool readable, uint64 in
});
}
os_get_module_info_unlock();
#ifdef ANDROID
#ifdef ANDROID32
/* i#1860: we need to keep looking for the segment with .dynamic as Android's
* loader does not map the whole file up front.
* i#XXX: This is not needed on newer versions of 64-bit Android, however we
* are not able to test with newer versions of 32-bit Android, so this may
* still be required.
*/
if (ma != NULL && at_map && readable)
os_module_update_dynamic_info(base, size, at_map);
Expand Down
Loading