Last Comment Bug 674806 - Allow Fennec to build with clang
: Allow Fennec to build with clang
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla9
Assigned To: Jim Chen [:jchen] [:darchons]
:
Mentors:
Depends on: 676308
Blocks: clang
  Show dependency treegraph
 
Reported: 2011-07-27 19:26 PDT by Jim Chen [:jchen] [:darchons]
Modified: 2011-09-12 18:46 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
a. Use cast in compound literal (862 bytes, patch)
2011-08-03 11:38 PDT, Jim Chen [:jchen] [:darchons]
blassey.bugs: review+
Details | Diff | Review
b. Include ctype.h (738 bytes, patch)
2011-08-03 11:40 PDT, Jim Chen [:jchen] [:darchons]
cjones.bugs: review+
Details | Diff | Review
c. Use -std=gnu89 for malloc wrappers (791 bytes, patch)
2011-08-03 11:44 PDT, Jim Chen [:jchen] [:darchons]
khuey: review+
Details | Diff | Review
d. Use char*, not void* for pointer arithmetic (4.95 KB, patch)
2011-08-03 11:46 PDT, Jim Chen [:jchen] [:darchons]
mwu.code: review+
Details | Diff | Review
e. add -fPIC flag for nss (1023 bytes, patch)
2011-08-03 11:55 PDT, Jim Chen [:jchen] [:darchons]
respindola: review-
Details | Diff | Review
Non-PIC allocation testcase (138 bytes, text/plain)
2011-08-17 15:38 PDT, Jim Chen [:jchen] [:darchons]
no flags Details
e. add -fPIC flag for nss (v2) (1.12 KB, patch)
2011-08-18 17:03 PDT, Jim Chen [:jchen] [:darchons]
mh+mozilla: review-
Details | Diff | Review
e. add -fPIC flag for nss (v3) (1.04 KB, patch)
2011-08-19 12:22 PDT, Jim Chen [:jchen] [:darchons]
mh+mozilla: review+
Details | Diff | Review

Description Jim Chen [:jchen] [:darchons] 2011-07-27 19:26:08 PDT
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) 2011-07-27 19:36:58 PDT
Cool :-)
Are you trying to build it for x86 or ARM?
Comment 2 Jim Chen [:jchen] [:darchons] 2011-07-27 19:51:08 PDT
(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) 2011-07-27 20:04:10 PDT
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 :-)
Comment 4 Jim Chen [:jchen] [:darchons] 2011-08-03 11:38:20 PDT
Created attachment 550439 [details] [diff] [review]
a. Use cast in compound literal

Clang is more strict about this.
Comment 5 Jim Chen [:jchen] [:darchons] 2011-08-03 11:40:02 PDT
Created attachment 550440 [details] [diff] [review]
b. Include ctype.h

Clang doesn't like seeing tolower() without including ctype.h first
Comment 6 Jim Chen [:jchen] [:darchons] 2011-08-03 11:44:05 PDT
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
Comment 7 Jim Chen [:jchen] [:darchons] 2011-08-03 11:46:53 PDT
Created attachment 550446 [details] [diff] [review]
d. Use char*, not void* for pointer arithmetic

Using void* produces an error in clang
Comment 8 Jim Chen [:jchen] [:darchons] 2011-08-03 11:55:24 PDT
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
Comment 9 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-03 12:08:49 PDT
(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) 2011-08-03 12:09:33 PDT
(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] 2011-08-03 16:40:38 PDT
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 Jim Chen [:jchen] [:darchons] 2011-08-04 10:09:06 PDT
(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 14 Michael Wu [:mwu] 2011-08-04 19:26:41 PDT
(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 16 Alon Zakai (:azakai) 2011-08-05 09:57:45 PDT
Reopening for parts c,d,e
Comment 17 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-07 19:04:02 PDT
Comment on attachment 550444 [details] [diff] [review]
c. Use -std=gnu89 for malloc wrappers

r=me
Comment 18 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-07 19:05:04 PDT
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) 2011-08-08 05:29:08 PDT
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) 2011-08-08 05:59:31 PDT
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 Jim Chen [:jchen] [:darchons] 2011-08-15 15:32:41 PDT
(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] 2011-08-15 23:08:40 PDT
(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) 2011-08-16 06:26:00 PDT
> 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 Jim Chen [:jchen] [:darchons] 2011-08-17 15:38:57 PDT
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.
Comment 25 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-08-18 08:50:23 PDT
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 Jim Chen [:jchen] [:darchons] 2011-08-18 17:03:31 PDT
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.
Comment 27 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-18 19:06:47 PDT
Why are you using shell syntax ('${DSO_CFLAGS}') instead of make syntax?
Comment 28 Mike Hommey [:glandium] 2011-08-18 21:53:36 PDT
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 Jim Chen [:jchen] [:darchons] 2011-08-19 12:22:57 PDT
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.
Comment 30 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-22 08:26:23 PDT
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 Jim Chen [:jchen] [:darchons] 2011-08-30 16:50:09 PDT
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) 2011-08-30 17:15:11 PDT
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 34 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-04 11:34:21 PDT
Jim, it would be great if you could post instructions on how to build Fennec with Clang somewhere.  Thanks!
Comment 35 Jim Chen [:jchen] [:darchons] 2011-09-12 18:46:03 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.