diff mbox

[1/2] icu: Delete inapropriate flag at link

Message ID 1403194417-19986-1-git-send-email-maxime.hadjinlian@gmail.com
State Accepted
Headers show

Commit Message

Maxime Hadjinlian June 19, 2014, 4:13 p.m. UTC
From: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>

Theses flags may cause libicudata to not be loaded on ARM EABIHF
because it would be built for ARM EABI only.

Signed-off-by: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
Cc: Thomas Petazzoni  <thomas.petazzoni@free-electrons.com>
---
 .../icu/icu-004-link-icudata-as-data-only.patch    | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 package/icu/icu-004-link-icudata-as-data-only.patch

--
2.0.0

Comments

Arnout Vandecappelle June 20, 2014, 9:27 p.m. UTC | #1
On 19/06/14 18:13, Maxime Hadjinlian wrote:
> From: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
> 
> Theses flags may cause libicudata to not be loaded on ARM EABIHF
> because it would be built for ARM EABI only.
> 
> Signed-off-by: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
> Cc: Thomas Petazzoni  <thomas.petazzoni@free-electrons.com>
> ---
>  .../icu/icu-004-link-icudata-as-data-only.patch    | 32 ++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 package/icu/icu-004-link-icudata-as-data-only.patch
> 
> diff --git a/package/icu/icu-004-link-icudata-as-data-only.patch b/package/icu/icu-004-link-icudata-as-data-only.patch
> new file mode 100644
> index 0000000..b69cec1
> --- /dev/null
> +++ b/package/icu/icu-004-link-icudata-as-data-only.patch
> @@ -0,0 +1,32 @@
> +From d5d0c4bb7cc9aa4a132ec0bea13255aee50c1cf9 Mon Sep 17 00:00:00 2001
> +From: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
> +Date: Fri, 6 Jun 2014 14:55:58 +0200
> +Subject: [PATCH] link icudata as data only

 I guess this should be "Don't link icudata as data only"

> +
> +This patch came straight from Debian.
> +It fix a trouble when libicudata would not respect some flags and would
> +for example end up being built for ARM EABI instead of ARM EABIHF

 I don't see how -nostdlib -nodefaultlibs could have that effect - especially in
buildroot, where we always force the abi. Do you have an example configuration
where this happens?

 That said, I also don't see a reason why you would want the -nostdlib
-nodefaultlibs because in the end it will be linked with those libraries anyway...

 Regards,
 Arnout


> +
> +Signed-off-by: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
> +---
> + source/config/mh-linux | 4 +++-
> + 1 file changed, 3 insertions(+), 1 deletion(-)
> +
> +diff --git a/source/config/mh-linux b/source/config/mh-linux
> +index 531a3b2..5a2a7c4 100644
> +--- a/source/config/mh-linux
> ++++ b/source/config/mh-linux
> +@@ -21,7 +21,9 @@ LD_RPATH= -Wl,-zorigin,-rpath,'$$'ORIGIN
> + LD_RPATH_PRE = -Wl,-rpath,
> +
> + ## These are the library specific LDFLAGS
> +-LDFLAGSICUDT=-nodefaultlibs -nostdlib
> ++#LDFLAGSICUDT=-nodefaultlibs -nostdlib
> ++# Debian change: linking icudata as data only causes too many problems.
> ++LDFLAGSICUDT=
> +
> + ## Compiler switch to embed a library name
> + # The initial tab in the next line is to prevent icu-config from reading it.
> +--
> +2.0.0.rc2
> +
> --
> 2.0.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
> 
>
Maxime Hadjinlian June 21, 2014, 10:51 a.m. UTC | #2
On Fri, Jun 20, 2014 at 11:27 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 19/06/14 18:13, Maxime Hadjinlian wrote:
>> From: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
>>
>> Theses flags may cause libicudata to not be loaded on ARM EABIHF
>> because it would be built for ARM EABI only.
>>
>> Signed-off-by: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
>> Cc: Thomas Petazzoni  <thomas.petazzoni@free-electrons.com>
>> ---
>>  .../icu/icu-004-link-icudata-as-data-only.patch    | 32 ++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>  create mode 100644 package/icu/icu-004-link-icudata-as-data-only.patch
>>
>> diff --git a/package/icu/icu-004-link-icudata-as-data-only.patch b/package/icu/icu-004-link-icudata-as-data-only.patch
>> new file mode 100644
>> index 0000000..b69cec1
>> --- /dev/null
>> +++ b/package/icu/icu-004-link-icudata-as-data-only.patch
>> @@ -0,0 +1,32 @@
>> +From d5d0c4bb7cc9aa4a132ec0bea13255aee50c1cf9 Mon Sep 17 00:00:00 2001
>> +From: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
>> +Date: Fri, 6 Jun 2014 14:55:58 +0200
>> +Subject: [PATCH] link icudata as data only
>
>  I guess this should be "Don't link icudata as data only"
Ah yes !
>
>> +
>> +This patch came straight from Debian.
>> +It fix a trouble when libicudata would not respect some flags and would
>> +for example end up being built for ARM EABI instead of ARM EABIHF
>
>  I don't see how -nostdlib -nodefaultlibs could have that effect - especially in
> buildroot, where we always force the abi. Do you have an example configuration
> where this happens?
When I build a Qt5 apps for ARM EABIHF, the libicudata.so ends up
being built without the hard float, which leads to unusable binary.
>
>  That said, I also don't see a reason why you would want the -nostdlib
> -nodefaultlibs because in the end it will be linked with those libraries anyway...
As said, this patch come straight from the Debian packages and seemed
like a good idea, and it proved to solve the issues. I haven't digged
enough to explain the 'why' it solves it.
>
>  Regards,
>  Arnout
>
>
>> +
>> +Signed-off-by: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
>> +---
>> + source/config/mh-linux | 4 +++-
>> + 1 file changed, 3 insertions(+), 1 deletion(-)
>> +
>> +diff --git a/source/config/mh-linux b/source/config/mh-linux
>> +index 531a3b2..5a2a7c4 100644
>> +--- a/source/config/mh-linux
>> ++++ b/source/config/mh-linux
>> +@@ -21,7 +21,9 @@ LD_RPATH= -Wl,-zorigin,-rpath,'$$'ORIGIN
>> + LD_RPATH_PRE = -Wl,-rpath,
>> +
>> + ## These are the library specific LDFLAGS
>> +-LDFLAGSICUDT=-nodefaultlibs -nostdlib
>> ++#LDFLAGSICUDT=-nodefaultlibs -nostdlib
>> ++# Debian change: linking icudata as data only causes too many problems.
>> ++LDFLAGSICUDT=
>> +
>> + ## Compiler switch to embed a library name
>> + # The initial tab in the next line is to prevent icu-config from reading it.
>> +--
>> +2.0.0.rc2
>> +
>> --
>> 2.0.0
>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot@busybox.net
>> http://lists.busybox.net/mailman/listinfo/buildroot
>>
>>
>
>
> --
> Arnout Vandecappelle                          arnout at mind be
> Senior Embedded Software Architect            +32-16-286500
> Essensium/Mind                                http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
> GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
Thomas Petazzoni June 21, 2014, 5:14 p.m. UTC | #3
Dear Arnout Vandecappelle,

On Fri, 20 Jun 2014 23:27:19 +0200, Arnout Vandecappelle wrote:

>  I don't see how -nostdlib -nodefaultlibs could have that effect - especially in
> buildroot, where we always force the abi. Do you have an example configuration
> where this happens?

Yes:

BR2_arm=y
BR2_cortex_a8=y
BR2_ARM_EABIHF=y
BR2_TOOLCHAIN_EXTERNAL=y
BR2_PACKAGE_ICU=y

With this configuration, all libraries are properly built EABIhf. For
example, libicutu.so:

$ LANG=C arm-linux-gnueabihf-readelf -A output/target/usr/lib/libicutu.so.51.2 
Attribute Section: aeabi
File Attributes
  Tag_CPU_name: "7-A"
  Tag_CPU_arch: v7
  Tag_CPU_arch_profile: Application
  Tag_ARM_ISA_use: Yes
  Tag_THUMB_ISA_use: Thumb-2
  Tag_FP_arch: VFPv3-D16
  Tag_ABI_PCS_wchar_t: 4
  Tag_ABI_FP_rounding: Needed
  Tag_ABI_FP_denormal: Needed
  Tag_ABI_FP_exceptions: Needed
  Tag_ABI_FP_number_model: IEEE 754
  Tag_ABI_align_needed: 8-byte
  Tag_ABI_align_preserved: 8-byte, except leaf SP
  Tag_ABI_enum_size: int
  Tag_ABI_HardFP_use: SP and DP
  Tag_ABI_VFP_args: VFP registers
  Tag_CPU_unaligned_access: v6
  Tag_Virtualization_use: TrustZone

However, libicudata.so is not built properly:

$ LANG=C arm-linux-gnueabihf-readelf -A output/target/usr/lib/libicudata.so.51.2 
Attribute Section: aeabi
File Attributes
  Tag_CPU_name: "Cortex-A8"
  Tag_CPU_arch: v7
  Tag_CPU_arch_profile: Application
  Tag_ARM_ISA_use: Yes
  Tag_THUMB_ISA_use: Thumb-2
  Tag_FP_arch: VFPv3-D16
  Tag_Virtualization_use: TrustZone

I was actually with Maxime in Paris a couple of weeks ago to debug this
issue. He had a Qt application that was not starting properly due to
this problem, because the dynamic library loader was refusing to load
the libicudata library. After applying the patch for Debian to remove
the problematic flags and rebuilding icu, we were able to start his Qt
application with no problem.

Also, the Debian changelog is pretty clear about what they've done, and
explicitly mentions the armhf problem. From
http://metadata.ftp-master.debian.org/changelogs//main/i/icu/icu_52.1-3_changelog:

icu (4.8.1.1-2) unstable; urgency=low

  * debian/patches/icudata-stdlibs.patch: Link stdlibs to libicudata so we
    get reasonably sane ELF headers on armhf.  Thanks Adam Conrad
    <adconrad@ubuntu.com>.  (Closes: #653457)

 -- Jay Berkenbilt <qjb@debian.org>  Wed, 04 Jan 2012 09:52:11 -0500

See also https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=653457 for
more details.

Hopefully, these details will be enough to justify the patch. Though I
agree with you that Maxime's description of the patch should have been
more detailed.

Best regards,

Thomas
Arnout Vandecappelle June 24, 2014, 6:06 p.m. UTC | #4
On 21/06/14 19:14, Thomas Petazzoni wrote:
[snip]
> See also https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=653457 for
> more details.

 That reference (and the failing config) indeed helps. Therefore:

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>


 Regards,
 Arnout

> 
> Hopefully, these details will be enough to justify the patch. Though I
> agree with you that Maxime's description of the patch should have been
> more detailed.
Thomas Petazzoni June 29, 2014, 10:48 a.m. UTC | #5
Dear Maxime Hadjinlian,

On Thu, 19 Jun 2014 18:13:36 +0200, Maxime Hadjinlian wrote:
> From: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
> 
> Theses flags may cause libicudata to not be loaded on ARM EABIHF
> because it would be built for ARM EABI only.
> 
> Signed-off-by: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
> Cc: Thomas Petazzoni  <thomas.petazzoni@free-electrons.com>
> ---
>  .../icu/icu-004-link-icudata-as-data-only.patch    | 32 ++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 package/icu/icu-004-link-icudata-as-data-only.patch

Thanks, patch applied, after adding more details following the
discussion with Arnout.

Thomas
diff mbox

Patch

diff --git a/package/icu/icu-004-link-icudata-as-data-only.patch b/package/icu/icu-004-link-icudata-as-data-only.patch
new file mode 100644
index 0000000..b69cec1
--- /dev/null
+++ b/package/icu/icu-004-link-icudata-as-data-only.patch
@@ -0,0 +1,32 @@ 
+From d5d0c4bb7cc9aa4a132ec0bea13255aee50c1cf9 Mon Sep 17 00:00:00 2001
+From: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
+Date: Fri, 6 Jun 2014 14:55:58 +0200
+Subject: [PATCH] link icudata as data only
+
+This patch came straight from Debian.
+It fix a trouble when libicudata would not respect some flags and would
+for example end up being built for ARM EABI instead of ARM EABIHF
+
+Signed-off-by: Maxime Hadjinlian <maxime.hadjinlian@devialet.com>
+---
+ source/config/mh-linux | 4 +++-
+ 1 file changed, 3 insertions(+), 1 deletion(-)
+
+diff --git a/source/config/mh-linux b/source/config/mh-linux
+index 531a3b2..5a2a7c4 100644
+--- a/source/config/mh-linux
++++ b/source/config/mh-linux
+@@ -21,7 +21,9 @@ LD_RPATH= -Wl,-zorigin,-rpath,'$$'ORIGIN
+ LD_RPATH_PRE = -Wl,-rpath,
+
+ ## These are the library specific LDFLAGS
+-LDFLAGSICUDT=-nodefaultlibs -nostdlib
++#LDFLAGSICUDT=-nodefaultlibs -nostdlib
++# Debian change: linking icudata as data only causes too many problems.
++LDFLAGSICUDT=
+
+ ## Compiler switch to embed a library name
+ # The initial tab in the next line is to prevent icu-config from reading it.
+--
+2.0.0.rc2
+