Closed Bug 934715 Opened 6 years ago Closed 6 years ago

Properly pass CFLAGS/CXXFLAGS to ICU.

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 912371

People

(Reporter: decoder, Assigned: decoder)

References

(Blocks 1 open bug)

Details

(Keywords: sec-want)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch icu-asan.patch (obsolete) — Splinter Review
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.
Attachment #827079 - Flags: feedback?(mh+mozilla)
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.
Attachment #827079 - Flags: feedback?(mh+mozilla) → feedback+
Attached patch Patch v2Splinter Review
This version of the patch also deals properly with MSVC, confirmed to be working on try :) Flagging ted for review as suggested by glandium.
Attachment #827079 - Attachment is obsolete: true
Attachment #827583 - Flags: review?(ted)
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
Attachment #827583 - Flags: review?(ted) → review+
m_kato already made almost the same changes now in bug 912371. I'll close this as a dup.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 912371
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.