Allow Fennec to build with clang

RESOLVED FIXED in mozilla9

Status

()

Core
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

Trunk
mozilla9
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
One of my internship projects is to get Fennec to build with clang, which I have the patches for.

Since we can already build with clang on desktop, the patches will be mostly on Fennec-only code.
(Assignee)

Updated

6 years ago
Assignee: nobody → nchen
Blocks: 574346
Status: NEW → ASSIGNED
Cool :-)
Are you trying to build it for x86 or ARM?
(Assignee)

Comment 2

6 years ago
(In reply to comment #1)
> Cool :-)
> Are you trying to build it for x86 or ARM?

For ARM on Android. Right now my setup is trunk clang/llvm with gcc/binutils from a custom build of the Android native development kit (NDK). And Fennec seems to run fine with the patches.

I actually have a patch for clang too. I saw that you've done a lot of clang development, maybe you can review it? I'm not familiar with the clang development process :)
Sure, you should email it to cfe-commits and cc me (rafael.espindola@gmail.com)

I remember that there were some android specific patches to gcc, but looking at the documentation it looks like it was just the driver: http://gcc.gnu.org/onlinedocs/gcc/GNU_002fLinux-Options.html :-)
(Assignee)

Updated

6 years ago
Depends on: 676308
(Assignee)

Comment 4

6 years ago
Created attachment 550439 [details] [diff] [review]
a. Use cast in compound literal

Clang is more strict about this.
Attachment #550439 - Flags: review?(blassey.bugs)
(Assignee)

Comment 5

6 years ago
Created attachment 550440 [details] [diff] [review]
b. Include ctype.h

Clang doesn't like seeing tolower() without including ctype.h first
Attachment #550440 - Flags: review?(jones.chris.g)
Attachment #550439 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 550444 [details] [diff] [review]
c. Use -std=gnu89 for malloc wrappers

Wasn't sure if using OS_CFLAGS is okay. CFLAGS didn't work. and other *_CFLAGS I tried applied to both .c and .cpp, even though I only wanted it to apply to ld_malloc_wrappers.c
Attachment #550444 - Flags: review?(khuey)
(Assignee)

Comment 7

6 years ago
Created attachment 550446 [details] [diff] [review]
d. Use char*, not void* for pointer arithmetic

Using void* produces an error in clang
Attachment #550446 - Flags: review?(mwu)
(Assignee)

Comment 8

6 years ago
Created attachment 550452 [details] [diff] [review]
e. add -fPIC flag for nss

clang needs this and it fits nicely with the other Android-specific options
Attachment #550452 - Flags: review?(khuey)
(In reply to comment #7)
> Created attachment 550446 [details] [diff] [review] [diff] [details] [review]
> d. Use char*, not void* for pointer arithmetic
> 
> Using void* produces an error in clang
What error?
(In reply to comment #6)
> Created attachment 550444 [details] [diff] [review] [diff] [details] [review]
> c. Use -std=gnu89 for malloc wrappers
> 
> Wasn't sure if using OS_CFLAGS is okay. CFLAGS didn't work. and other
> *_CFLAGS I tried applied to both .c and .cpp, even though I only wanted it
> to apply to ld_malloc_wrappers.c

Can you use -fgnu89-inline or fix the code?
Attachment #550440 - Flags: review?(jones.chris.g) → review+

Comment 11

6 years ago
Jim, can check with espindola to make sure the errors that occur without these patches should be patched on our side?

Also, nss should already be built with -fPIC with gcc. I wonder why we need to add it here for clang.
(Assignee)

Comment 12

6 years ago
(In reply to comment #9)
> (In reply to comment #7)
> > Created attachment 550446 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > d. Use char*, not void* for pointer arithmetic
> > 
> > Using void* produces an error in clang
> What error?

"error: arithmetic on a pointer to void". It is an undefined behavior according to the Internet.

(In reply to comment #10)
> (In reply to comment #6)
> > Created attachment 550444 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > c. Use -std=gnu89 for malloc wrappers
> > 
> > Wasn't sure if using OS_CFLAGS is okay. CFLAGS didn't work. and other
> > *_CFLAGS I tried applied to both .c and .cpp, even though I only wanted it
> > to apply to ld_malloc_wrappers.c
> 
> Can you use -fgnu89-inline or fix the code?

I'll check with blassey on getting rid of the inlines.

(In reply to comment #11)
> Jim, can check with espindola to make sure the errors that occur without
> these patches should be patched on our side?

Yeah. Most of these are clang being more strict about standards than gcc, which I think is worth fixing on our end (i.e. compound literal, void* arithmetic, etc.)

> Also, nss should already be built with -fPIC with gcc. I wonder why we need
> to add it here for clang.

Hm, do you know where -fPIC is configured?
Parts a and b are on inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/770498ac2b3e
http://hg.mozilla.org/integration/mozilla-inbound/rev/c69f76d027b2

Updated

6 years ago
Attachment #550446 - Flags: review?(mwu) → review+

Comment 14

6 years ago
(In reply to comment #12)
> > Also, nss should already be built with -fPIC with gcc. I wonder why we need
> > to add it here for clang.
> 
> Hm, do you know where -fPIC is configured?

in Linux.mk by DSO_CFLAGS.. which we override.

Bizarre. I wonder how it got set last time. I remember checking all the libraries for text relocations and the only major ones were from libstdc++.
http://hg.mozilla.org/mozilla-central/rev/770498ac2b3e
http://hg.mozilla.org/mozilla-central/rev/c69f76d027b2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Reopening for parts c,d,e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 550444 [details] [diff] [review]
c. Use -std=gnu89 for malloc wrappers

r=me
Attachment #550444 - Flags: review?(khuey) → review+
Comment on attachment 550452 [details] [diff] [review]
e. add -fPIC flag for nss

r=me
Attachment #550452 - Flags: review?(khuey) → review+
Comment on attachment 550444 [details] [diff] [review]
c. Use -std=gnu89 for malloc wrappers

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

Do you know what 'inline' in that file depends on gnu89 semantics? That code looks really strange. For example, __wrap_posix_memalign is declared inline but never called in that file. Do you know why it is declared inline?

If there is indeed a reason why they must be declared inline, this patch is OK with a comment explaining why we need gnu89 instead of c99 inline semantics.
Comment on attachment 550452 [details] [diff] [review]
e. add -fPIC flag for nss

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

Do you know why this is needed by clang but not by gcc? Similar cases I have seem on x86 where because a configure test was passing with gcc and failing with clang. If that is the case in here, we should fix the configure check, not hardcode a -fPIC in the Makefile.
Attachment #550452 - Flags: review+ → review-
(Assignee)

Comment 21

6 years ago
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #19)
> Comment on attachment 550444 [details] [diff] [review]
> 
> Do you know what 'inline' in that file depends on gnu89 semantics? That code
> looks really strange. For example, __wrap_posix_memalign is declared inline
> but never called in that file. Do you know why it is declared inline?
> 

Holding off for now because glandium says he's doing some work on this.

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #20)
> Comment on attachment 550452 [details] [diff] [review]
> e. add -fPIC flag for nss
> 
> Review of attachment 550452 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you know why this is needed by clang but not by gcc? Similar cases I have
> seem on x86 where because a configure test was passing with gcc and failing
> with clang. If that is the case in here, we should fix the configure check,
> not hardcode a -fPIC in the Makefile.

Finally had some time to look into this. The command line flags are the same between clang and gcc, and -fPIC is _not_ specified under either configuration for nss.

Seems like the difference is clang tends to generate certain absolute (i.e. non-PIC) relocations when -fPIC is not specified, whereas GCC does not generate them (even without -fPIC). So this is not a compiler bug, and the solution is to just specify -fPIC like the patch does.
(In reply to Jim Chen [:jchen] (mobile intern :) from comment #21)
> Finally had some time to look into this. The command line flags are the same
> between clang and gcc, and -fPIC is _not_ specified under either
> configuration for nss.

This is not entirely true. -fPIC *is* specified, *except* for Android, because DSO_CFLAGS, which normally contains -fPIC in the NSS build system, is overridden in security/manager/Makefile.in. IMHO, a better way to fix this would be to find a different variable to stick the android specific flags in. ARCHFLAG sounds good for that (it only contains -m64 or -m32, and only does so for ppc64 or 32-bits cross-build on x86-64).
> Seems like the difference is clang tends to generate certain absolute (i.e.
> non-PIC) relocations when -fPIC is not specified, whereas GCC does not
> generate them (even without -fPIC). So this is not a compiler bug, and the
> solution is to just specify -fPIC like the patch does.

Not sure about it not being an compiler bug. ARM is a funny architecture and maybe there is some guarantee about when -fPIC is needed. 

I do agree that it is probably good to just add -fPIC. If nothing else it is documentation.

Would you mind providing a preprocessed file that produce a non PIC relocation with clang and a PIC one with gcc? Thanks!
(Assignee)

Comment 24

6 years ago
Created attachment 553931 [details]
Non-PIC allocation testcase

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #23)
> 
> Would you mind providing a preprocessed file that produce a non PIC
> relocation with clang and a PIC one with gcc? Thanks!

Hm, so clang uses the movw and movt instructions. From what I read, they can be faster but they cannot be used in PIC relocations. Here's a test case that reproduces the problem.

> clang -ccc-host-triple arm-eabi -integrated-as -march=armv7-a -Os -c -o dsotest.o dsotest.c
> arm-linux-androideabi-objdump -d dsotest.o
> > 00000000 <test_func>:
> >    0: e3000000  movw  r0, #0  ; 0x0
> >    4: e3400000  movt  r0, #0  ; 0x0
> >    8: e12fff1e  bx  lr
> arm-linux-androideabi-readelf -r dsotest.o
> > Relocation section '.rel.text' at offset 0x2c0 contains 2 entries:
> >  Offset     Info    Type            Sym.Value  Sym. Name
> > 00000000  0000072b R_ARM_MOVW_ABS_NC 00000000   test_extern
> > 00000004  0000072c R_ARM_MOVT_ABS    00000000   test_extern

In comparison:

> arm-linux-androideabi-gcc -march=armv7-a -Os -c -o dsotest.o dsotest.c
> arm-linux-androideabi-objdump -d dsotest.o
> > 00000000 <test_func>:
> >    0: e59f3004  ldr r3, [pc, #4]  ; c <test_func+0xc>
> >    4: e79f0003  ldr r0, [pc, r3]
> >    8: e12fff1e  bx  lr
> >    c: 00000000  .word 0x00000000
> arm-linux-androideabi-readelf -r dsotest.o
> > Relocation section '.rel.text' at offset 0x37c contains 1 entries:
> >  Offset     Info    Type            Sym.Value  Sym. Name
> > 0000000c  00000b60 R_ARM_GOT_PREL    00000000   test_extern

I'll post another patch that uses ARCHFLAG to retain DSO_CFLAGS like glandium suggested.
ok, I agree that passing -fPIC is the correct fix. The gcc I have without PIC produces

       ldr     r0, .L2
.L2:
        .word   test_extern

So using -fPIC is probably a good thing for avoiding runtime relocations anyway.
(Assignee)

Comment 26

6 years ago
Created attachment 554263 [details] [diff] [review]
e. add -fPIC flag for nss (v2)

I opted to prepend existing DSO_CFLAGS and DSO_LDOPTS to Android-specific flags.

My reasoning over using ARCHFLAG is this allows DSO_CFLAGS and DSO_LDOPTS to stay as a pair as we override them, which feels more natural to me.
Attachment #550452 - Attachment is obsolete: true
Attachment #554263 - Flags: review?(khuey)
Why are you using shell syntax ('${DSO_CFLAGS}') instead of make syntax?
Comment on attachment 554263 [details] [diff] [review]
e. add -fPIC flag for nss (v2)

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

::: security/manager/Makefile.in
@@ +263,5 @@
>  	OS_RELEASE="2.6" \
>  	OS_PTHREAD= \
>  	STANDARDS_CFLAGS="-std=gnu89" \
> +	DSO_CFLAGS="${DSO_CFLAGS} ${CFLAGS} -DCHECK_FORK_GETPID -DRTLD_NOLOAD=0 -DANDROID_VERSION=$(ANDROID_VERSION) -include $(ABS_topsrcdir)/security/manager/android_stub.h" \
> +	DSO_LDOPTS="${DSO_LDOPTS} $(LDFLAGS) $(WRAP_MALLOC_CFLAGS) $(WRAP_MALLOC_LIB) " \

Other than the style issue kyle points out (though ${} is also supported for make variables), I'm not certain this is a good idea, because this will effectively force mozilla DSO_* variables over those used by NSS. That it only happens for Android is bothering. And that without modification, DSO_LDFLAGS contains "$@" in mozilla and not in NSS could be problematic too (in fact, i suspect it will be expanded when calling the sub-make, that will give some nice SONAME to the nss libraries).
Attachment #554263 - Flags: review?(khuey) → review-
(Assignee)

Comment 29

6 years ago
Created attachment 554505 [details] [diff] [review]
e. add -fPIC flag for nss (v3)

Thanks for the comments. Switched back to using ARCHFLAG and corrected the style mistake.
Attachment #554263 - Attachment is obsolete: true
Attachment #554505 - Flags: review?(khuey)
Comment on attachment 554505 [details] [diff] [review]
e. add -fPIC flag for nss (v3)

I think glandium has a better understanding of what this change does that I do, so redirecting.
Attachment #554505 - Flags: review?(khuey) → review?(mh+mozilla)
Attachment #554505 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 31

6 years ago
Comment on attachment 550444 [details] [diff] [review]
c. Use -std=gnu89 for malloc wrappers

glandium's work fixed this. Thanks! Parts d and e can land.
Attachment #550444 - Attachment is obsolete: true
Parts d and e pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b9d2cfc61a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9b24fb1487

This bug can be resolved when these two changesets merge to mozilla-central.
Status: REOPENED → ASSIGNED
Whiteboard: [inbound]
Target Milestone: mozilla8 → ---
http://hg.mozilla.org/mozilla-central/rev/a2b9d2cfc61a
http://hg.mozilla.org/mozilla-central/rev/8c9b24fb1487
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Jim, it would be great if you could post instructions on how to build Fennec with Clang somewhere.  Thanks!
(Assignee)

Comment 35

6 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #34)
> Jim, it would be great if you could post instructions on how to build Fennec
> with Clang somewhere.  Thanks!

tl;dr from IRC: the clang instructions are at https://wiki.mozilla.org/Android/Clang.

Note that for getting the NDK source, android.git.kernel.org is down ATM and hasn't shown any signs of life... But I just remembered you can probably use glandium's pre-built NDK from http://glandium.org/blog/?p=2146 instead, which should work with clang.
You need to log in before you can comment on or make changes to this bug.