Patchwork PATCH: PR bootstrap/54209: [4.8 Regression] Failed to build gcc for Android/x86

login
register
mail settings
Submitter H.J. Lu
Date Aug. 9, 2012, 4:39 p.m.
Message ID <20120809163953.GA5322@intel.com>
Download mbox | patch
Permalink /patch/176192/
State New
Headers show

Comments

H.J. Lu - Aug. 9, 2012, 4:39 p.m.
Hi,

Bionic C library doesn't provide link.h.  This patch reverts revision
186788:

http://gcc.gnu.org/ml/gcc-cvs/2012-04/msg00740.html

OK to install?

Thanks.

H.J.
---
2012-08-09  H.J. Lu  <hongjiu.lu@intel.com>

	PR bootstrap/54209
	* unwind-dw2-fde-dip.c (USE_PT_GNU_EH_FRAME): Don't define for
	Bionic C library.
Ian Taylor - Aug. 9, 2012, 10:17 p.m.
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
H.J. Lu - Aug. 9, 2012, 11:01 p.m.
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.
Ian Taylor - Aug. 9, 2012, 11:14 p.m.
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
H.J. Lu - Aug. 14, 2012, 7:38 p.m.
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. Lu - Aug. 14, 2012, 7:39 p.m.
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.
Maxim Kuvyrkov - Aug. 14, 2012, 10:47 p.m.
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>
Ian Taylor - Aug. 14, 2012, 11:27 p.m.
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
H.J. Lu - Aug. 14, 2012, 11:40 p.m.
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.
H.J. Lu - Aug. 14, 2012, 11:42 p.m.
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.
H.J. Lu - Aug. 20, 2012, 2:02 p.m.
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.

Patch

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