674806 - Allow Fennec to build with clang

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Cool :-)
Are you trying to build it for x86 or ARM?

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

(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 :)

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Sure, you should email it to cfe-commits and cc me ([e-mail])

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 :-)

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: a. Use cast in compound literal

Clang is more strict about this.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: b. Include ctype.h

Clang doesn't like seeing tolower() without including ctype.h first

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: d. Use char*, not void* for pointer arithmetic

Using void* produces an error in clang

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: e. add -fPIC flag for nss

clang needs this and it fits nicely with the other Android-specific options

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(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?

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(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?

Comment 11

[Michael Wu [:mwu]]

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.

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

(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?

Comment 13

[Alon Zakai (:azakai)]

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

Comment 14

[Michael Wu [:mwu]]

(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++.

Comment 15

[Marco Bonardo [:mak]]

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

Comment 16

[Alon Zakai (:azakai)]

Reopening for parts c,d,e

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

r=me

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 550452 [details] [diff] [review]
e. add -fPIC flag for nss

r=me

Comment 19

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

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 20

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

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.

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

(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.

Comment 22

[Mike Hommey [:glandium]]

(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).

Comment 23

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> 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!

Comment 24

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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.

Comment 25

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

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.

Comment 26

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Why are you using shell syntax ('${DSO_CFLAGS}') instead of make syntax?

Comment 28

[Mike Hommey [:glandium]]

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).

Comment 29

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: e. add -fPIC flag for nss (v3)

Thanks for the comments. Switched back to using ARCHFLAG and corrected the style mistake.

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 31

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 32

[Matt Brubeck (:mbrubeck)]

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.

Comment 33

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/a2b9d2cfc61a
http://hg.mozilla.org/mozilla-central/rev/8c9b24fb1487

Comment 34

[(no longer active)]

Jim, it would be great if you could post instructions on how to build Fennec with Clang somewhere.  Thanks!

Comment 35

[(inactive) Jim Chen [:jchen] [:darchons]]

(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.