| Message ID | 20120809163953.GA5322@intel.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 9, 2012 at 9:39 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: > > Bionic C library doesn't provide link.h. Does Bionic provide dl_iterate_phdr? If it does, I'll just note in passing that it would be straightforward to simply incorporate the required types and constants in unwind-dw2-fde-dip.c directly, and avoid the #include. If it doesn't, then of course nothing will make this code work correctly. Ian
On Thu, Aug 9, 2012 at 3:17 PM, Ian Lance Taylor <iant@google.com> wrote: > On Thu, Aug 9, 2012 at 9:39 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >> >> Bionic C library doesn't provide link.h. > > Does Bionic provide dl_iterate_phdr? If it does, I'll just note in > passing that it would be straightforward to simply incorporate the > required types and constants in unwind-dw2-fde-dip.c directly, and > avoid the #include. If it doesn't, then of course nothing will make > this code work correctly. That is a good idea. Pavel, can you look into it? Thanks.
On Thu, Aug 9, 2012 at 4:01 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Thu, Aug 9, 2012 at 3:17 PM, Ian Lance Taylor <iant@google.com> wrote: >> On Thu, Aug 9, 2012 at 9:39 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >>> >>> Bionic C library doesn't provide link.h. >> >> Does Bionic provide dl_iterate_phdr? If it does, I'll just note in >> passing that it would be straightforward to simply incorporate the >> required types and constants in unwind-dw2-fde-dip.c directly, and >> avoid the #include. If it doesn't, then of course nothing will make >> this code work correctly. > > That is a good idea. Pavel, can you look into it? You may find libiberty/simple-object-elf.c to be a useful guide. Ian
On Thu, Aug 9, 2012 at 3:17 PM, Ian Lance Taylor <iant@google.com> wrote: > On Thu, Aug 9, 2012 at 9:39 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >> >> Bionic C library doesn't provide link.h. > > Does Bionic provide dl_iterate_phdr? If it does, I'll just note in > passing that it would be straightforward to simply incorporate the > required types and constants in unwind-dw2-fde-dip.c directly, and > avoid the #include. If it doesn't, then of course nothing will make > this code work correctly. > dl_iterate_phdr is provided in libdl.so, which is always linked with dynamic executables: #define ANDROID_LIB_SPEC \ "%{!static: -ldl}" This patch fixes Android/x86 build on trunk. OK to install? Thanks.
On Tue, Aug 14, 2012 at 12:38 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Thu, Aug 9, 2012 at 3:17 PM, Ian Lance Taylor <iant@google.com> wrote: >> On Thu, Aug 9, 2012 at 9:39 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >>> >>> Bionic C library doesn't provide link.h. >> >> Does Bionic provide dl_iterate_phdr? If it does, I'll just note in >> passing that it would be straightforward to simply incorporate the >> required types and constants in unwind-dw2-fde-dip.c directly, and >> avoid the #include. If it doesn't, then of course nothing will make >> this code work correctly. >> > > dl_iterate_phdr is provided in libdl.so, which is always linked with > dynamic executables: > > #define ANDROID_LIB_SPEC \ > "%{!static: -ldl}" > > > This patch fixes Android/x86 build on trunk. OK to install? > > Thanks. > > -- > H.J. > ---- > 2012-08-14 H.J. Lu <hongjiu.lu@intel.com> > > PR bootstrap/54209 > * unwind-dw2-fde-dip.c (dl_phdr_info): New struct for Bionic C > library. > (ElfW): New macro for Bionic C library. > Don't include <link.h> for Bionic C library. Wrong patch. Here is the right one.
On 15/08/2012, at 7:39 AM, H.J. Lu wrote: > On Tue, Aug 14, 2012 at 12:38 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Thu, Aug 9, 2012 at 3:17 PM, Ian Lance Taylor <iant@google.com> wrote: >>> On Thu, Aug 9, 2012 at 9:39 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >>>> >>>> Bionic C library doesn't provide link.h. >>> >>> Does Bionic provide dl_iterate_phdr? If it does, I'll just note in >>> passing that it would be straightforward to simply incorporate the >>> required types and constants in unwind-dw2-fde-dip.c directly, and >>> avoid the #include. If it doesn't, then of course nothing will make >>> this code work correctly. >>> >> >> dl_iterate_phdr is provided in libdl.so, which is always linked with >> dynamic executables: >> >> #define ANDROID_LIB_SPEC \ >> "%{!static: -ldl}" >> >> >> This patch fixes Android/x86 build on trunk. OK to install? [Adding David Turner to CC as the main Bionic expert. Also reattaching HJ's current patch so that David can easily look at it. Link to the bug report: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54209] I think this patch will break MIPS Android build due to mismatch of ElfW(type) when _MIPS_SZPTR == 64. I think the right way to fix this is to make Bionic export link.h or already-existing linker.h, but I differ to Ian for final judgement. FWIW, I'm OK with using hard-coded definitions if link.h is absent, and using definitions from link.h if it is there. I.e., #ifdef HAVE_LINK_H # include <link.h> #else <YOUR PATCH> #endif This would allow Bionic to eventually catch up and provide link.h uniformly across all targets. I've looked into latest Android NDK distribution, and the situation with link.h is not uniform across targets. ARM and x86 don't have link.h, while MIPS does: --- /* For building unwind-dw2-fde-glibc.c for MIPS frame unwinding, we need to have <link.h> that defines struct dl_phdr_info, ELFW(type), and dl_iterate_phdr(). */ #include <sys/types.h> #include <elf.h> struct dl_phdr_info { Elf32_Addr dlpi_addr; const char *dlpi_name; const Elf32_Phdr *dlpi_phdr; Elf32_Half dlpi_phnum; }; #if _MIPS_SZPTR == 32 #define ElfW(type) Elf32_##type #elif _MIPS_SZPTR == 64 #define ElfW(type) Elf64_##type #endif int dl_iterate_phdr(int (*cb)(struct dl_phdr_info *info, size_t size, void *data), void *data); --- I'm not 100% sure where the above link.h comes from for MIPS, but since it's not present in Bionic sources, my guess is kernel's arch/mips/include directory. Checking ... No, not from the kernel sources. Hm... -- Maxim Kuvyrkov CodeSourcery / Mentor Graphics >> >> 2012-08-14 H.J. Lu <hongjiu.lu@intel.com> >> >> PR bootstrap/54209 >> * unwind-dw2-fde-dip.c (dl_phdr_info): New struct for Bionic C >> library. >> (ElfW): New macro for Bionic C library. >> Don't include <link.h> for Bionic C library. > > Wrong patch. Here is the right one. > > -- > H.J. > <gcc-pr54209.patch>
On Tue, Aug 14, 2012 at 3:47 PM, Maxim Kuvyrkov <maxim@codesourcery.com> wrote: > I think this patch will break MIPS Android build due to mismatch of ElfW(type) when _MIPS_SZPTR == 64. I think the right way to fix this is to make Bionic export link.h or already-existing linker.h, but I differ to Ian for final judgement. I think it would be better to export <link.h>. I don't know how feasible that is or how long it would take to become available. > FWIW, I'm OK with using hard-coded definitions if link.h is absent, and using definitions from link.h if it is there. I.e., > > #ifdef HAVE_LINK_H > # include <link.h> > #else > <YOUR PATCH> > #endif This is conceptually fine as long as we are clear that we are testing for the presence of link.h on the target, not the host. It can be hard for libgcc to reliably test for the presence of target-specific header files. Ian
On Tue, Aug 14, 2012 at 3:47 PM, Maxim Kuvyrkov <maxim@codesourcery.com> wrote: > On 15/08/2012, at 7:39 AM, H.J. Lu wrote: > >> On Tue, Aug 14, 2012 at 12:38 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>> On Thu, Aug 9, 2012 at 3:17 PM, Ian Lance Taylor <iant@google.com> wrote: >>>> On Thu, Aug 9, 2012 at 9:39 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >>>>> >>>>> Bionic C library doesn't provide link.h. >>>> >>>> Does Bionic provide dl_iterate_phdr? If it does, I'll just note in >>>> passing that it would be straightforward to simply incorporate the >>>> required types and constants in unwind-dw2-fde-dip.c directly, and >>>> avoid the #include. If it doesn't, then of course nothing will make >>>> this code work correctly. >>>> >>> >>> dl_iterate_phdr is provided in libdl.so, which is always linked with >>> dynamic executables: >>> >>> #define ANDROID_LIB_SPEC \ >>> "%{!static: -ldl}" >>> >>> >>> This patch fixes Android/x86 build on trunk. OK to install? > > [Adding David Turner to CC as the main Bionic expert. Also reattaching HJ's current patch so that David can easily look at it. Link to the bug report: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54209] > > I think this patch will break MIPS Android build due to mismatch of ElfW(type) when _MIPS_SZPTR == 64. I think the right way to fix this is to make Bionic export link.h or already-existing linker.h, but I differ to Ian for final judgement. > > FWIW, I'm OK with using hard-coded definitions if link.h is absent, and using definitions from link.h if it is there. I.e., > > #ifdef HAVE_LINK_H > # include <link.h> > #else > <YOUR PATCH> > #endif > > This would allow Bionic to eventually catch up and provide link.h uniformly across all targets. > > I've looked into latest Android NDK distribution, and the situation with link.h is not uniform across targets. ARM and x86 don't have link.h, while MIPS does: > --- > /* > For building unwind-dw2-fde-glibc.c for MIPS frame unwinding, > we need to have <link.h> that defines struct dl_phdr_info, > ELFW(type), and dl_iterate_phdr(). > */ > > #include <sys/types.h> > #include <elf.h> > > struct dl_phdr_info > { > Elf32_Addr dlpi_addr; > const char *dlpi_name; > const Elf32_Phdr *dlpi_phdr; > Elf32_Half dlpi_phnum; > }; > > #if _MIPS_SZPTR == 32 > #define ElfW(type) Elf32_##type > #elif _MIPS_SZPTR == 64 > #define ElfW(type) Elf64_##type > #endif > > int > dl_iterate_phdr(int (*cb)(struct dl_phdr_info *info, size_t size, void *data), > void *data); > --- > > I'm not 100% sure where the above link.h comes from for MIPS, but since it's not present in Bionic sources, my guess is kernel's arch/mips/include directory. Checking ... No, not from the kernel sources. Hm... > > -- Bionic is a 32-bit library. I don't know how _MIPS_SZPTR == 64 works with Bionic on mips.
On Tue, Aug 14, 2012 at 4:27 PM, Ian Lance Taylor <iant@google.com> wrote: > On Tue, Aug 14, 2012 at 3:47 PM, Maxim Kuvyrkov <maxim@codesourcery.com> wrote: > >> I think this patch will break MIPS Android build due to mismatch of ElfW(type) when _MIPS_SZPTR == 64. I think the right way to fix this is to make Bionic export link.h or already-existing linker.h, but I differ to Ian for final judgement. > > I think it would be better to export <link.h>. I don't know how > feasible that is or how long it would take to become available. Pavel, how long does it take to export <link.h> for Android/x86? >> FWIW, I'm OK with using hard-coded definitions if link.h is absent, and using definitions from link.h if it is there. I.e., >> >> #ifdef HAVE_LINK_H >> # include <link.h> >> #else >> <YOUR PATCH> >> #endif > > This is conceptually fine as long as we are clear that we are testing > for the presence of link.h on the target, not the host. It can be > hard for libgcc to reliably test for the presence of target-specific > header files. > That is also my concern.
On Fri, Aug 17, 2012 at 2:42 AM, Chupin, Pavel V <pavel.v.chupin@intel.com> wrote: > Submitted patch here: https://android-review.googlesource.com/#/c/41705 > > -- Pavel > link.h has been added to AOSP. I am closing PR 54209. Thanks.
diff --git a/libgcc/unwind-dw2-fde-dip.c b/libgcc/unwind-dw2-fde-dip.c index 92f8ab5..f57dc8c 100644 --- a/libgcc/unwind-dw2-fde-dip.c +++ b/libgcc/unwind-dw2-fde-dip.c @@ -54,11 +54,6 @@ #endif #if !defined(inhibit_libc) && defined(HAVE_LD_EH_FRAME_HDR) \ - && defined(__BIONIC__) -# define USE_PT_GNU_EH_FRAME -#endif - -#if !defined(inhibit_libc) && defined(HAVE_LD_EH_FRAME_HDR) \ && defined(__FreeBSD__) && __FreeBSD__ >= 7 # define ElfW __ElfN # define USE_PT_GNU_EH_FRAME