Last Comment Bug 693323 - NodeList DOM bindings break the build with clang
: NodeList DOM bindings break the build with clang
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: :Ehsan Akhgari (busy, don't ask for review please)
:
Mentors:
Depends on:
Blocks: clang 648801
  Show dependency treegraph
 
Reported: 2011-10-10 09:25 PDT by :Ehsan Akhgari (busy, don't ask for review please)
Modified: 2011-10-12 11:34 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
build failure log (28.15 KB, text/plain)
2011-10-10 09:25 PDT, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details
Patch (v1) (1.05 KB, patch)
2011-10-11 12:05 PDT, :Ehsan Akhgari (busy, don't ask for review please)
peterv: review+
Details | Diff | Review

Description :Ehsan Akhgari (busy, don't ask for review please) 2011-10-10 09:25:49 PDT
Created attachment 565953 [details]
build failure log

The libxul build now fails.  I'm attaching a log to the bug.
Comment 1 Peter Van der Beken [:peterv] 2011-10-10 15:08:03 PDT
Hmm, weird, no idea what's wrong :-/.
Comment 2 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-11 08:16:30 PDT
I'll try bisecting the range to see if I can find the offending patch...
Comment 3 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-10-11 09:19:12 PDT
I could not reproduce this with this mozconfig:

----------------------------
ac_add_options --disable-optimize
ac_add_options --enable-debug
ac_add_options --enable-libxul

ac_add_options --enable-application=browser
ac_add_options --enable-tests
ac_add_options --enable-trace-malloc
ac_add_options --enable-accessibility

# For NSS symbols
export MOZ_DEBUG_SYMBOLS=1
ac_add_options --enable-debug-symbols="-gdwarf-2"

# Enable parallel compiling
mk_add_options MOZ_MAKE_FLAGS="-j4"

# Needed to enable breakpad in application.ini
export MOZILLA_OFFICIAL=1
---------------------------------

I am building on 10.7, with clang 141650 and mozilla-central 9e090190e01e01c43770642de032f3b6b76c7e4c (the git clone)
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-11 10:03:38 PDT
Bisecting points to http://hg.mozilla.org/mozilla-central/rev/71625e542826 as the first broken revision.

The interesting thing which happened there is templatizing the NodeList class.  It's possible that clang is not instantiating the templatized functions for some reason.  Using nm to find __ZN3xpc3dom8NodeListI17nsIHTMLCollectionE21getPropertyDescriptorEP9JSContextP8JSObject4jsidbP20JSPropertyDescriptor for example only points to dom_quickstubs.o referencing it, but no .o file is defining it.

I'm currently building with gcc to see which .o file defines it there.
Comment 5 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-11 10:17:23 PDT
That symbol is defined in dom_quickstubs.o for a gcc build...
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-11 11:06:55 PDT
So, it seems like explicitly instantiating NodeList<nsINodeList> and NodeList<nsIHTMLCollection> fixes the build by causing clang to instantiate everything correctly.

I don't really know what's causing gcc to instantiate the template and clang not to, but I don't care too much to find out the cause.  What I suspect is that we are only using NodeList's static members in this translation unit, so technically the compiler doesn't need to instantiate the class.

Now, I have to figure out what needs to change on the trunk, because the NodeList class does not exist (at least in that form) any more.
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-11 12:05:24 PDT
Created attachment 566290 [details] [diff] [review]
Patch (v1)
Comment 8 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-11 12:06:51 PDT
My patch basically instantiates the templates explicitly instead of relying on the compiler to do it for us.
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-11 12:07:34 PDT
http://tbpl.mozilla.org/?tree=Try&rev=f0546be1ca70
Comment 10 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-10-11 13:06:16 PDT
This has been fixed in recent clangs, I reducing the case to have a log if someone else hits this problem.
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-11 16:10:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/01c60492772c
Comment 12 Mozilla RelEng Bot 2011-10-11 16:31:13 PDT
Try run for f0546be1ca70 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f0546be1ca70
Results (out of 18 total builds):
    success: 17
    warnings: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-f0546be1ca70
Comment 13 Marco Bonardo [::mak] 2011-10-12 03:17:27 PDT
https://hg.mozilla.org/mozilla-central/rev/01c60492772c
Comment 14 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-10-12 11:34:22 PDT
The reduced testcase is

void NewProxyObject(void *);
template<class T> struct NodeList  {
  static NodeList instance;
  NodeList();
  virtual void getPropertyDescriptor();
  static void create();
};
void foobar() {
  NodeList<int>::create();
}
template<class T> NodeList<T>::NodeList() {
}
template<class T> void NodeList<T>::getPropertyDescriptor() {
}
template<class T> void NodeList<T>::create() {
    NewProxyObject(&NodeList<T>::instance);
}
template<class T> NodeList<T> NodeList<T>::instance;

Older versions of clang would not cause a template instantiation and get an undefined reference to getPropertyDescriptor from the vtable (which is used in the constructor, which is used to initialize the static variable).

On the clang side this was http://llvm.org/bugs/show_bug.cgi?id=10020 which got fixed by r132331.

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