Firefox has a relatively straightforward C++ codebase that largely parses with clang++. This bug organizes various issues with semantic analysis and code generation that have come up. I have about 20 bugs to file so far, so the dependency list is going to be getting longer pretty quickly.
Excellent. I wanted to do this last summer, but I found no time. I'm all over these bugs.
Very nice Eli!
Reassigning to Eli so I stop getting two mails about these bugs :)
Created attachment 3911 [details] Current diff for Firefox on my computer I figure it'd be nice to give a quick status update. With the attached patch to Mozilla and a couple minor patches to clang I haven't gotten around to committing yet, the build process progresses smoothly all the way to linking libxul, where it explodes due to link errors (primarily caused by bug 5557 and some code I was forced to comment out to work around bug 5549). My mozconfig: . $topsrcdir/browser/config/mozconfig mk_add_options AUTOCONF=autoconf2.13 mk_add_options MOZ_OBJDIR=/home/eli/mozbin ac_add_options --disable-debug ac_add_options --disable-optimize ac_add_options --disable-crashreporter ac_add_options --disable-tests
How are we doing on Firefox now?
(In reply to comment #5) > How are we doing on Firefox now? With an updated patch, Firefox builds but crashes during startup; I haven't looked that closely, but my best guess is that it's some sort of vtable issue.
(In reply to comment #6) > (In reply to comment #5) > > How are we doing on Firefox now? > > With an updated patch, Firefox builds but crashes during startup; I haven't > looked that closely, but my best guess is that it's some sort of vtable issue. Is the patch working around bugs in Clang or bugs in Firefox?
Created attachment 4210 [details] Updated firefox patch Going through the issues per file: configure.in: clang issue: Workaround for bug 4062 (although this arguably isn't a bug). firefox issue: Fix a completely nasty configure check which greps for a warning. content/html/content/src/nsHTMLMediaElement.cpp: clang issue: Workaround for a crash which I haven't reduced yet. docshell/base/nsDocShell.cpp: clang issue: Bug I'll file soon: -Wreturn-type warning for the following: int a() { if (__builtin_expect(1,1)) return 1; } (Note that the Firefox build uses -Werror=return-type.) embedding/browser/gtk/src/EmbedPrivate.cpp: firefox issue?: Initializer required for const struct. gfx/thebes/public/gfxPlatformGtk.h: firefox issue: Add declaration so lookup from template works correctly. ipc/chromium/src/base/port.h: clang issue?: ::__builtin_va_copy doesn't work. ipc/chromium/src/base/task.h: firefox issue: Lookup in dependent base. js/src/configure.in: clang issue: Workaround for bug 4062. js/src/jstracer.cpp: ?: I forget what the issue was with VisitGlobalSlots. firefox issue: Simple bug; the code inside the ifdef is only used because of a workaround elsewhere. js/src/jstypedarray.cpp: firefox issue?: error using typedef to define member. firefox issue?: definition of specialization member after use. js/src/jstypes.h: clang issue: workaround bug 5728. js/src/nanojit/LIR.h: firefox issue: lookup in dependent base. js/src/nanojit/avmplus.h: clang issue: workaround bug 5728. layout/style/nsCSSRuleProcessor.cpp: firefox issue?: initializer required for const struct. memory/jemalloc/jemalloc.h: clang issue?: redeclarations not accepted by clang. modules/plugin/test/testplugin/nptest.cpp: modules/plugin/test/testplugin/nptest_gtk2.cpp: modules/plugin/test/testplugin/nptest_utils.cpp: clang issue: some unreduced issues with system c++ headers. storage/src/mozStorageStatement.cpp: firefox issue?: initializer required for const struct. toolkit/components/places/src/SQLFunctions.cpp: firefox issue?: initializer required for const struct. toolkit/xre/nsAppRunner.cpp: firefox issue?: initializer required for const struct. xpcom/base/nsAutoRef.h: firefox issue: lookup in dependent base xpcom/base/nsDebugImpl.cpp: firefox issue?: initializer required for const struct. xpcom/base/nsTraceRefcntImpl.cpp: firefox issue?: initializer required for const struct. xpcom/base/nscore.h: clang issue: workaround bug 5728. xpcom/glue/nsBaseHashtable.h: firefox issue: lookup in dependent base xpcom/glue/nsClassHashtable.h: firefox issue: lookup in dependent base xpcom/glue/nsEnumeratorUtils.cpp: firefox issue?: initializer required for const struct. xpcom/glue/nsInterfaceHashtable.h: firefox issue: lookup in dependent base xpcom/glue/nsRefPtrHashtable.h: firefox issue: lookup in dependent base xpcom/glue/nsTPtrArray.h: firefox issue: lookup in dependent base xpcom/io/nsUnicharInputStream.cpp firefox issue?: initializer required for const struct.
I checked the issue with VisitGlobalSlots; the issue is that the Firefox code is broken, but g++ doesn't catch it because the template is never instantiated. The -Wreturn-type issue is now filed as bug 6274. I think I'm convinced that the js/src/jstypedarray.cpp issues are Firefox bugs. dgregor, would you mind taking a quick look at the following issues? I'm not sure if these are clang bugs: Redeclaration of malloc: #include <cstdlib> void *malloc(size_t); Initializer required for const struct: struct x { x(); }; struct y : x {}; void a() { const y z; } ::__builtin_va_copy doesn't work: void a() { __builtin_va_list x, y; ::__builtin_va_copy(x, y); }
The initializer-for-const-variable thing is a clang bug: [dcl.init]p6: If a program calls for the default initialization of an object of a const-qualified type T, T shall be a class type with a user-provided default constructor.
I guess I should clarify: I feel that the standard obviously means "a class type with a non-trivial default constructor".
(In reply to comment #11) > I guess I should clarify: I feel that the standard obviously means "a class > type with a non-trivial default constructor". I think I understand why you feel that way, but the standard is clear in what it says, and EDG also diagnoses this code as an error. "t.C", line 3: error: const variable "z" requires an initializer -- class "y" has no explicitly declared default constructor void a() { const y z; } ^
(In reply to comment #9) > I checked the issue with VisitGlobalSlots; the issue is that the Firefox code > is broken, but g++ doesn't catch it because the template is never instantiated. > > The -Wreturn-type issue is now filed as bug 6274. > > I think I'm convinced that the js/src/jstypedarray.cpp issues are Firefox bugs. > > dgregor, would you mind taking a quick look at the following issues? I'm not > sure if these are clang bugs: > Redeclaration of malloc: > #include <cstdlib> > void *malloc(size_t); On Darwin I don't see an error... could you attach preprocessed input? > Initializer required for const struct: > struct x { x(); }; > struct y : x {}; > void a() { const y z; } This is a Firefox bug; Clang and EDG are doing the right thing according to the standard. That said, John thinks the standard is wrong so he'll be filing a core issue about it :) > ::__builtin_va_copy doesn't work: > void a() { __builtin_va_list x, y; ::__builtin_va_copy(x, y); } Clang bug
(In reply to comment #13) > > Redeclaration of malloc: > > #include <cstdlib> > > void *malloc(size_t); > > On Darwin I don't see an error... could you attach preprocessed input? The declaration of malloc is as follows: extern void *malloc (size_t __size) throw () __attribute__ ((__malloc__)) ; And the error is the following: <stdin>:2:7: error: exception specification in declaration does not match previous declaration Note that it seems that gcc is somehow special-casing the system headers. > > Initializer required for const struct: > > struct x { x(); }; > > struct y : x {}; > > void a() { const y z; } > > This is a Firefox bug; Clang and EDG are doing the right thing according to the > standard. That said, John thinks the standard is wrong so he'll be filing a > core issue about it :) Okay. > > ::__builtin_va_copy doesn't work: > > void a() { __builtin_va_list x, y; ::__builtin_va_copy(x, y); } > > Clang bug Okay; will file.
(In reply to comment #14) > (In reply to comment #13) > > > Redeclaration of malloc: > > > #include <cstdlib> > > > void *malloc(size_t); > > > > On Darwin I don't see an error... could you attach preprocessed input? > > The declaration of malloc is as follows: > extern void *malloc (size_t __size) throw () __attribute__ ((__malloc__)) ; > > And the error is the following: > <stdin>:2:7: error: exception specification in declaration does not match > previous declaration > > Note that it seems that gcc is somehow special-casing the system headers. Okay, we should do the same. > > > ::__builtin_va_copy doesn't work: > > > void a() { __builtin_va_list x, y; ::__builtin_va_copy(x, y); } > > > > Clang bug > > Okay; will file. Don't bother; it's fixed in r95966.
(In reply to comment #14) > (In reply to comment #13) > > > Redeclaration of malloc: > > > #include <cstdlib> > > > void *malloc(size_t); > > > > On Darwin I don't see an error... could you attach preprocessed input? > > The declaration of malloc is as follows: > extern void *malloc (size_t __size) throw () __attribute__ ((__malloc__)) ; > > And the error is the following: > <stdin>:2:7: error: exception specification in declaration does not match > previous declaration > > Note that it seems that gcc is somehow special-casing the system headers. Should be fixed in r95969.
Created attachment 4221 [details] Updated firefox patch Updated patch; stuff which still needs to be addressed to consider the patch mature: It appears js/src/jstypedarray.cpp is affected by a variant of bug 6179; I'm not sure if it's precisely the same issue, though. I still need to reduce the crash from content/html/content/src/nsHTMLMediaElement.cpp. Bug 5728 is the cause of some messy parts of the patch.
(In reply to comment #17) > Created an attachment (id=4221) [details] > Updated firefox patch > > Updated patch; stuff which still needs to be addressed to consider the patch > mature: > > It appears js/src/jstypedarray.cpp is affected by a variant of bug 6179; I'm > not sure if it's precisely the same issue, though. 6179 is now fixed; we'll see if it fixes this issue or not.
(In reply to comment #18) > 6179 is now fixed; we'll see if it fixes this issue or not. It does; there's still a minor issue in js/src/jstypedarray.cpp which I'm not sure is a bug, though, which boils down to something like the following: template <typename T> class X { static int x, *y; }; template <> int *X<int>::y = &X<int>::x; template <> int X<int>::x = 3;
(In reply to comment #19) > (In reply to comment #18) > > 6179 is now fixed; we'll see if it fixes this issue or not. > > It does; there's still a minor issue in js/src/jstypedarray.cpp which I'm not > sure is a bug, though, which boils down to something like the following: > > template <typename T> class X { static int x, *y; }; > template <> int *X<int>::y = &X<int>::x; > template <> int X<int>::x = 3; Clang's diagnostic about an explicit specialization following an implicit instantiation for this example is correct. EDG produces a diagnostic for the same issue.
Created attachment 4434 [details] Updated firefox patch Updated to Firefox trunk, and accounting for clang bugfixes; still crashes on startup with what looks like some sort of vtable issue.
I just tried building with trunk on Linux x86-64, and it now produces a working webbrowser! One file doesn't compile with clang, though, because of bug 3933, and another required an additional change which might be a clang bug. I'll update with more later.
(In reply to comment #22) > I just tried building with trunk on Linux x86-64, and it now produces a working > webbrowser! One file doesn't compile with clang, though, because of bug 3933, > and another required an additional change which might be a clang bug. I'll > update with more later. That's awesome! Thanks, Eli.
Created attachment 4939 [details] Updated firefox patch Updated to trunk Firefox/clang; build with -fno-access-control pending the fix to bug 7230. One new change I'm not entirely sure of; is the following valid? class A; class B; template <typename T> void f(B* x) { (void)static_cast<A*>(x); }; class A {}; class B : public A {}; void g() { f<int>(0); }
(In reply to comment #24) > > One new change I'm not entirely sure of; is the following valid? > class A; > class B; > template <typename T> void f(B* x) { (void)static_cast<A*>(x); }; > class A {}; > class B : public A {}; > void g() { f<int>(0); } No. Nothing in that template function is dependent, so it can all be validated at parse time, in the context of the definition, and not in the context of the instantiation. See specifically [temp.res]p8: "If a type used in a non-dependent name is incomplete at the point at which a template is defined but is complete at the point at which an instantiation is done, and if the completeness of that type affects whether or not the program is well-formed or affects the semantics of the program, the program is ill-formed; no diagnostic is required." Since the completeness of B and A affect whether the program is well-formed, and they fulfill the criteria of this sentence, the program is ill-formed. That Clang actually emits the diagnostic is a QoI issue.
Created attachment 4946 [details] Updated firefox patch Updated for recent clang fixes. I believe every change in this patch except for the xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp change and possibly some of the configure changes indicate real bugs in the Firefox code.
Have you tried this recently Eli? Is this meta bug still needed, since there is only one blocking bug left?
(In reply to comment #27) > Have you tried this recently Eli? Is this meta bug still needed, since there is > only one blocking bug left? I'd like to keep this open until it's possible to build stock Firefox with clang.
(In reply to comment #28) > I'd like to keep this open until it's possible to build stock Firefox with > clang. Maybe you could post links to the Mozilla bugs here?
Bug reports filed so far: https://bugzilla.mozilla.org/show_bug.cgi?id=573210 (Fix already committed.) https://bugzilla.mozilla.org/show_bug.cgi?id=576355 http://code.google.com/p/webm/issues/detail?id=114
(In reply to comment #30) > http://code.google.com/p/webm/issues/detail?id=114 That patch seems like it's covering up a clang bug. If the inlines were in a header file, and were "too big to inline", they'd have to be emitted somewhere, no?
(Or would it work if it were "inline" instead of "__inline"?)
(In reply to comment #31) > (In reply to comment #30) > > http://code.google.com/p/webm/issues/detail?id=114 > > That patch seems like it's covering up a clang bug. If the inlines were in a > header file, and were "too big to inline", they'd have to be emitted somewhere, > no? With C99 inlining semantics, no definition of the inline function is emitted, so you need to have a non-inline definition somewhere. What we typically see is that the code will link with -O2 but not without optimization, since the former ends up inlining the function rather than creating an external reference. There's more information about C99's inlining rules here: http://www.greenend.org.uk/rjk/2003/03/inline.html
Just filed: https://bugzilla.mozilla.org/show_bug.cgi?id=576363 https://bugzilla.mozilla.org/show_bug.cgi?id=576367
Just filed: https://bugzilla.mozilla.org/show_bug.cgi?id=579686 https://bugzilla.mozilla.org/show_bug.cgi?id=579689
Just filed: https://bugzilla.mozilla.org/show_bug.cgi?id=579692
Created attachment 5242 [details] Updated firefox patch Current diff (for reference). Note that Firefox now requires an implementation of '#pragma GCC visibility' to link on Linux, so I've been working on that...
Just filed: https://bugzilla.mozilla.org/show_bug.cgi?id=579786 https://bugzilla.mozilla.org/show_bug.cgi?id=579788
Created attachment 5329 [details] Updated firefox patch
Created attachment 5426 [details] update firefox clang patch for x64-linux
Eli- I updated your patch to work for x86-64 Linux, about half dozen minor changes were needed. This applies against yesterday's Mozilla head. Thanks!
The resulting binary simply loops indefinitely without displaying anything. It creates ~/.mozilla/firefox but put no files there. Oh well :(.
The same patch permits a clean build on x86-Linux (Ubuntu 10.04, clang 112379) but the resulting binary segfaults. Under valgrind it seems to work fine.
Argh... my patch fails to creating a buildable firefox on MacOS 10.6. There are both configure and header file issues. I may slog through the problems but not clear I can make time during the next couple weeks.
This is now just depending on one bug (Bug 3933) so we don't need a tracking bug anymore. Any future issues should be treated as random other bugs, we don't need a new tracking bug for this. Woo