934715 - Properly pass CFLAGS/CXXFLAGS to ICU.

Status: RESOLVED DUPLICATE of bug 912371

(Firefox Build System :: General, defect)

Description

[Christian Holler (:decoder)]

In bug 933231, we noticed that the ICU build system does not use our CFLAGS/CXXFLAGS but will use our LDFLAGS. This will cause ASan builds to break after that patch lands. This also means that currently, the whole ICU subsystem is built without ASan instrumentation.

I suggest we pass in our CFLAGS/CXXFLAGS prior to ICU's own flags. I've made a patch for this and it builds on Linux, but I had to explicitly include -frtti in that patch because ICU depends on rtti and we're by default building with -fno-rtti. All the other flags we're using did not cause trouble for me.

Comment 1

[Christian Holler (:decoder)]

Attachment: icu-asan.patch

Glandium, can you tell me if this patch looks ok, or how you would do it? This builds on Linux and I'm doing a try-run now for the other OSes.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 827079 [details] [diff] [review]
icu-asan.patch

Review of attachment 827079 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/configure.in
@@ +4207,5 @@
>      ICU_LINK_OPTS="--enable-static --disable-shared"
>      # Force the ICU static libraries to be position independent code
> +    # ICU needs rtti to build properly
> +    ICU_CFLAGS="$CFLAGS $DSO_PIC_CFLAGS"
> +    ICU_CXXFLAGS="$CXXFLAGS $DSO_PIC_CFLAGS -frtti"

This will obviously fail with msvc.

Comment 3

[Christian Holler (:decoder)]

Attachment: Patch v2

This version of the patch also deals properly with MSVC, confirmed to be working on try :) Flagging ted for review as suggested by glandium.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 827583 [details] [diff] [review]
Patch v2

Review of attachment 827583 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/configure.in
@@ +4204,5 @@
>      esac
>  
>      # To reduce library size, use static linking
>      ICU_LINK_OPTS="--enable-static --disable-shared"
> +    

Trailing ws

Comment 5

[Christian Holler (:decoder)]

m_kato already made almost the same changes now in bug 912371. I'll close this as a dup.