Bug 29244 - undefined reference errors when linking due to gperf and inlining
Summary: undefined reference errors when linking due to gperf and inlining
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Andras Becsi
URL:
Keywords:
: 29778 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-09-14 07:17 PDT by Priit Laes (IRC: plaes)
Modified: 2010-10-04 02:36 PDT (History)
27 users (show)

See Also:


Attachments
webkit-resolve-findentity-link-error.patch (2.23 KB, patch)
2009-10-07 09:48 PDT, Priit Laes (IRC: plaes)
no flags Details | Formatted Diff | Diff
first draft: Refactor gperf hash generation to fix the debug build with gcc >= 4.4.0. (35.31 KB, patch)
2010-04-26 10:48 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
Refactor gperf code generation and usage to fix the debug build with gcc>4.4. (51.33 KB, patch)
2010-05-11 09:44 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
Refactor gperf v2 (54.76 KB, patch)
2010-05-11 14:13 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
Refactor gperf v2 update (54.72 KB, patch)
2010-05-11 14:34 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
Refactor gperf v3 (51.55 KB, patch)
2010-05-12 07:54 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
refactor gperf usage 1st iteration (32.38 KB, patch)
2010-05-19 16:04 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
centralize gperf usage (34.82 KB, patch)
2010-05-20 05:02 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
patch (25.04 KB, patch)
2010-06-22 06:58 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
patch (26.04 KB, patch)
2010-06-22 08:42 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
updated patch (27.42 KB, patch)
2010-06-23 11:27 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
updated patch (28.01 KB, patch)
2010-06-23 14:13 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
EFL port patch (2.91 KB, patch)
2010-06-24 10:41 PDT, Leandro Pereira
no flags Details | Formatted Diff | Diff
updated patch with EFL changes (33.50 KB, patch)
2010-07-09 02:05 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
updated patch (33.55 KB, patch)
2010-07-23 04:54 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
updated patch (32.98 KB, patch)
2010-08-09 01:37 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
updated patch (32.99 KB, patch)
2010-08-09 02:03 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
updated patch (33.60 KB, patch)
2010-08-09 05:23 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
updated to ToT (25.47 KB, patch)
2010-08-23 09:06 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
updated patch (25.33 KB, patch)
2010-08-30 09:40 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
updated to ToT (26.27 KB, patch)
2010-09-03 08:22 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Priit Laes (IRC: plaes) 2009-09-14 07:17:11 PDT
When configuring with --enable-debug. Linking fails with following errors:
[snip]
make  all-am
make[1]: Entering directory `/home/plaes/code/WebKit/WebKitBuild'
/bin/mkdir -p ./.deps/DerivedSources
  CCLD   Programs/GtkLauncher
./.libs/libwebkit-1.0.so: undefined reference to `findEntity(char const*, unsigned int)'
./.libs/libwebkit-1.0.so: undefined reference to `findColor(char const*, unsigned int)'
./.libs/libwebkit-1.0.so: undefined reference to `findProp(char const*, unsigned int)'
./.libs/libwebkit-1.0.so: undefined reference to `findValue(char const*, unsigned int)'
./.libs/libwebkit-1.0.so: undefined reference to `findDoctypeEntry(char const*, unsigned int)'
collect2: ld returned 1 exit status
make[1]: *** [Programs/GtkLauncher] Error 1
make[1]: Leaving directory `/home/plaes/code/WebKit/WebKitBuild'
make: *** [all] Error 2
[/snip]

All these symbols come from files that are autogenerated using gperf.
Comment 1 Priit Laes (IRC: plaes) 2009-09-14 07:28:22 PDT
After reading GCC bug report below, should we fix our gperf-files instead??

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41194
Comment 2 Priit Laes (IRC: plaes) 2009-09-15 22:40:16 PDT
From the discussion with gperf developer Bruno Haible:


> >   - the source code of the source file that #includes the processed file?
> Hum? It seems to be compiled straight into library...

OK, so that was compiled as a C file and thus created a 'findEntity' function
with C linkage.

> >   - the output of
> >        $ nm --dynamic .libs/libwebkit-1.0.so | grep findEntity
> [output]
>                        U _Z10findEntityPKcj
> 000000000138d1a8 T findEntity
> [/output]

Here you see that 'findEntity' exists as a function with C linkage but some
other compilation unit needs it as a function with C++ linkage.


What you need is either to #include the generated C file like this:
  #include "gperf-generated-c-code.c"
from a C++ compilation unit. Or directly name the generated file
"gperf-generated-c-code.cc", and tell gperf to generate C++ code instead
of ANSI C code.

Or to use the following declaration in your C++ compilation units:

  extern "C" {
  extern const struct Entity *findEntity (const char *str, unsigned int len);
  }
Comment 3 Jan Alonzo 2009-09-27 04:19:15 PDT
*** Bug 29778 has been marked as a duplicate of this bug. ***
Comment 4 Priit Laes (IRC: plaes) 2009-10-07 09:48:53 PDT
Created attachment 40800 [details]
webkit-resolve-findentity-link-error.patch

One out of 5 is done: no more "undefined reference to `findEntity(char const*, unsigned int)'"
Comment 5 Priit Laes (IRC: plaes) 2009-10-11 03:32:13 PDT
Just a note, so I won't forget:
< bdash> 1) Make the C++ gperf output work + add smaller wrapper functions to match the existing functions that are used
< bdash> 2) Stop including the .c files directly from .cpp files, give them matching .h files, and compile them separately on all platforms
< bdash> or do both (convert to using C++ and compile them separately)
Comment 6 Evan Martin 2009-11-13 10:32:00 PST
Chrome bug: http://code.google.com/p/chromium/issues/detail?id=26496 .
Nothing too useful there except to note they found that turning off optimizations (-O0 instead of -O1) is a workaround.
Comment 7 Evan Martin 2009-12-20 03:33:05 PST
Does this still happen?  I haven't heard anyone complain about it in a while.
Comment 8 Priit Laes (IRC: plaes) 2009-12-21 02:01:51 PST
(In reply to comment #7)
> Does this still happen?  I haven't heard anyone complain about it in a while.

Probably yes (I don't have access to my dev-machine to be 100% sure).
Comment 9 he zhitong 2009-12-24 00:24:32 PST
(In reply to comment #7)
> Does this still happen?  I haven't heard anyone complain about it in a while.

Yes, when linking chrome in latest chromium source minutes ago.

and change gcc flag -O0 to -O1 in webcore.target.mk fixes..
Comment 10 Stephan Aßmus 2010-02-11 15:21:53 PST
We experience exactly the same problem when building on Haiku, with GCC 4.3.3 and  with NDEBUG not being defined. The Haiku build system (not currently in SVN) is setup such that it uses -g for CC and C++ flags when NDEBUG is not defined. Our solution has been to simply patch the affected generated source code after generation, such that the whole #define block which tries to force inlining is wrapped in #ifdef NDEBUG ... #endif. Since this works, I believe the problem is not the linkage being used, since then it would also show up in release builds. The problem is more likely that GCC simply refuses to inline those functions in debug (-g) builds.
Comment 11 Ryan Leavengood 2010-02-14 11:18:34 PST
(In reply to comment #10)
> The problem is more likely that GCC simply refuses to inline
> those functions in debug (-g) builds.

Based on my reading of the GCC manuals, that is exactly what it does. Since inlining functions can make debugging more difficult, they aren't inlined in debug builds.
Comment 12 Andras Becsi 2010-04-24 08:01:24 PDT
(In reply to comment #5)
> Just a note, so I won't forget:
> < bdash> 1) Make the C++ gperf output work + add smaller wrapper functions to
> match the existing functions that are used
> < bdash> 2) Stop including the .c files directly from .cpp files, give them
> matching .h files, and compile them separately on all platforms
> < bdash> or do both (convert to using C++ and compile them separately)

I'm working on this since the debug linking problem also exists on the Qt port using gcc 4.4.2 and gcc 4.5.0.
Comment 13 Andras Becsi 2010-04-26 10:48:21 PDT
Created attachment 54317 [details]
first draft: Refactor gperf hash generation to fix the debug build with gcc >= 4.4.0.

Uploading the first draft and marking it r? to force a build check on the EWS bots. This patch is not complete yet, because the gyp file is not updated, so it will probably fail on the chromium bot. I would need some help from a gyp pro to fix it. Eric, maybe?
Comment 14 WebKit Review Bot 2010-04-26 10:52:06 PDT
Attachment 54317 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebEntities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:48:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 WebKit Review Bot 2010-04-26 11:13:48 PDT
Attachment 54317 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1907028
Comment 16 Darin Adler 2010-04-26 11:35:54 PDT
Comment on attachment 54317 [details]
first draft: Refactor gperf hash generation to fix the debug build with gcc >= 4.4.0.

Do all these changes have to happen at once? Is there a way to do this a step at a time?
Comment 17 Andras Becsi 2010-04-26 12:17:58 PDT
(In reply to comment #16)
> (From update of attachment 54317 [details])
> Do all these changes have to happen at once? Is there a way to do this a step
> at a time?

I could upload the make-hash-tools.pl and add the needed extra steps as separate rules to the build tools as a first step without touching the sources, and then update the sources and remove the old build rules as a second step if that would make the reviewing process easier.
Comment 18 Andras Becsi 2010-05-11 09:44:59 PDT
Created attachment 55713 [details]
Refactor gperf code generation and usage to fix the debug build with gcc>4.4.

Gperf code generation is done by a new script called WebCore/make-hash-tools.pl now,
and the generation options were moved into the gperf files, to make the options more understandable
and the generation process less redundant across the various ports.
Hitherto gperf generated C code, these files were included in multiple C++ files across WebCore
to access the functionality provided. This resulted in debug build failure with newer gcc versions
because of a new feature of gcc, which disables C style inlining in debug mode.
The make-hash-tools.pl script lets gperf generate C++ code for all gperf files now, which are compiled
in their own compilation unit.
The functionality provided by the generated code is wrapped behind HashTools.h, so there is no need
for multiple inclusions of generated C files to access these functions.

No new functionality added, no new tests needed.

Multiple patches would make the whole change confusing and error-prone, so leaving it in one peace.
The diff is big, but simple structured, hopefully the more lengthly explanation will make it easier to understand.
Comment 19 WebKit Review Bot 2010-05-11 09:47:36 PDT
Attachment 55713 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebEntities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:48:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 WebKit Review Bot 2010-05-11 10:14:48 PDT
Attachment 55713 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2225112
Comment 21 Kenneth Rohde Christiansen 2010-05-11 13:03:38 PDT
When setting r- (which should only be done by reviewers) please comment on why that was set negative. As I guess it is due to the build failures, and it is your own patch, removing the r? makes more sense.
Comment 22 Andras Becsi 2010-05-11 13:10:39 PDT
Comment on attachment 55713 [details]
Refactor gperf code generation and usage to fix the debug build with gcc>4.4.

Sorry for the mistake. The chromium build failure was the cause, I'm working blindly on that platform. The mac EWS doesn't seem to function though, it has no output and build on Leopard worked for me.
Comment 23 Andras Becsi 2010-05-11 14:13:52 PDT
Created attachment 55754 [details]
Refactor gperf v2

Ok, my bad. Mac EWS was correct, the previous patch didn't build on Leopard, it was a preliminary version of the working one.
Now it should also work on chromium.
Comment 24 Andras Becsi 2010-05-11 14:34:53 PDT
Created attachment 55757 [details]
Refactor gperf v2 update

Updated patch to ToT.
Comment 25 WebKit Review Bot 2010-05-11 14:40:23 PDT
Attachment 55757 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebEntities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:48:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 WebKit Review Bot 2010-05-11 15:59:00 PDT
Attachment 55757 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2194134
Comment 27 Andras Becsi 2010-05-12 03:52:17 PDT
Comment on attachment 55757 [details]
Refactor gperf v2 update

The chromium build system doesn't like me.
Comment 28 Andras Becsi 2010-05-12 07:54:04 PDT
Created attachment 55845 [details]
Refactor gperf v3

Multiple build systems are a torture.
Comment 29 WebKit Review Bot 2010-05-12 07:56:28 PDT
Attachment 55845 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebEntities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:48:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 30 Eric Seidel (no email) 2010-05-12 08:00:33 PDT
Attachment 55845 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2173167
Comment 31 Andras Becsi 2010-05-12 08:09:00 PDT
Comment on attachment 55845 [details]
Refactor gperf v3

Ok, then let's try to split this into smaller pieces.
Comment 32 WebKit Review Bot 2010-05-12 08:43:49 PDT
Attachment 55845 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2212168
Comment 33 Gustavo Sverzut Barbieri 2010-05-14 06:09:23 PDT
(In reply to comment #12)
> (In reply to comment #5)
> > Just a note, so I won't forget:
> > < bdash> 1) Make the C++ gperf output work + add smaller wrapper functions to
> > match the existing functions that are used
> > < bdash> 2) Stop including the .c files directly from .cpp files, give them
> > matching .h files, and compile them separately on all platforms
> > < bdash> or do both (convert to using C++ and compile them separately)
> 
> I'm working on this since the debug linking problem also exists on the Qt port using gcc 4.4.2 and gcc 4.5.0.

I don't fully agree with bdash's solution. If we generate C++ code and another object that is linked together, we will do more function calls for no good. If we have them included as they are right now, the function will effectively go away with optimizations (as happens now). I guess the extern "C" {} is the best solution, it will keep performance hit away and should solve the problem.
Comment 34 Andras Becsi 2010-05-14 08:37:17 PDT
> I don't fully agree with bdash's solution. If we generate C++ code and another 
> object that is linked together, we will do more function calls for no good. If we 
> have them included as they are right now, the function will effectively go away 
> with optimizations (as happens now). I guess the extern "C" {} is the best 
> solution, it will keep performance hit away and should solve the problem.

The problem is that extern "C" doesn't fix the linking issue.
Comment 35 Lucas De Marchi 2010-05-18 12:34:56 PDT
(In reply to comment #34)
> > I don't fully agree with bdash's solution. If we generate C++ code and another 
> > object that is linked together, we will do more function calls for no good. If we 
> > have them included as they are right now, the function will effectively go away 
> > with optimizations (as happens now). I guess the extern "C" {} is the best 
> > solution, it will keep performance hit away and should solve the problem.
> 
> The problem is that extern "C" doesn't fix the linking issue.

Don't think so. I have a similar patch that does not work only for
findValue symbol.

I dislike this patch so much too, because it'd be a one-line patch to gperf, simply changing __attribute__(__gnu_inline__) to __attribute__(__always_inline__)
I'm wondering if this would be acceptable by gperf and webkit projects.
Comment 36 Csaba Osztrogonác 2010-05-19 02:53:05 PDT
(In reply to comment #35)
> I dislike this patch so much too, because it'd be a one-line patch to gperf, simply changing __attribute__(__gnu_inline__) to __attribute__(__always_inline__)
> I'm wondering if this would be acceptable by gperf and webkit projects.

I don't think patching gperf source and then build it, isn't acceptable by WebKit project, because it would require extra work from all developers to setup patched gperf and it would be error prone.
Comment 37 Stephan Aßmus 2010-05-19 03:54:43 PDT
In the Haiku build system, we simply post-process the generated files:

# modify the checks around gnu_inline to include NDEBUG so that symbols are
# generated for debug builds
for file in CSSPropertyNames.cpp CSSValueKeywords.c ColorData.c DocTypeStrings.cpp HTMLEntityNames.c
do
	echo "Patching $file: wrap gnu_inline in NDEBUG check"
	sed --in-place=.unpatched -e 's/__GNUC_STDC_INLINE__ || defined __GNUC_GNU_INLINE__/NDEBUG \&\& (__GNUC_STDC_INLINE__ || defined __GNUC_GNU_INLINE__)/g' "$file"
done
Comment 38 Andras Becsi 2010-05-19 08:36:36 PDT

(In reply to comment #35)
> (In reply to comment #34)
> > > I don't fully agree with bdash's solution. If we generate C++ code and another 
> > > object that is linked together, we will do more function calls for no good. If we 
> > > have them included as they are right now, the function will effectively go away 
> > > with optimizations (as happens now). I guess the extern "C" {} is the best 
> > > solution, it will keep performance hit away and should solve the problem.
> > 
> > The problem is that extern "C" doesn't fix the linking issue.
> 
> Don't think so. I have a similar patch that does not work only for
> findValue symbol.

Then please contribute your patch, so we can discuss the two solutions here and resolve this issue which is rotting here for almost 10 months now.

For me the approach of the first patch by Priit didn't fix the linking issues on QtWebKit using gcc 4.5.0.

> I dislike this patch so much too, because it'd be a one-line patch to gperf, simply changing __attribute__(__gnu_inline__) to __attribute__(__always_inline__)
> I'm wondering if this would be acceptable by gperf and webkit projects.

Changing gnu_inline to always_inline in the generated file fixes the linking issue indeed.
But as I see it the problem is not only this linking issue, much more the redundant gperf configuration across five (or so) build systems and their maintainability, not to speak of the multiple source file inclusions across the tree. 
Post-processing the generated files would introduce a much bigger mess than the current one and is a dirty work-around. I'd much better like to fix the current mess of multiple c and cpp source inclusions across WebCore and concentrate the code generation to one script, and not to work around issues and sweep them under the carpet.
The introduced function calls by separating the generated code into it's own compilation unit are by no means significant compared to the time consumed by the function logic itself and the runtime of the parsers which use it, so I think this is not an argument against the approach originally proposed by bdash.
Comment 39 Andras Becsi 2010-05-19 16:04:55 PDT
Created attachment 56528 [details]
refactor gperf usage 1st iteration

Just to check the build on the EWS bots.
Comment 40 WebKit Review Bot 2010-05-19 16:08:32 PDT
Attachment 56528 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/Color.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/html/HTMLTokenizer.cpp:58:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 41 WebKit Review Bot 2010-05-19 17:15:14 PDT
Attachment 56528 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2278348
Comment 42 Andras Becsi 2010-05-20 05:02:27 PDT
Created attachment 56587 [details]
centralize gperf usage

Now with ChangeLog, hopefully this is correct.

This patch is the firs half of the previous approach which removes the redundancy of gperf usage from multiple build systems and centralizes the code generation to make-hash-tools.pl. This doesn't fix the linking issues yet, they have to be addressed in a follow-up patch.
Comment 43 WebKit Review Bot 2010-05-20 05:05:51 PDT
Attachment 56587 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/Color.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/html/HTMLTokenizer.cpp:58:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 44 Andras Becsi 2010-05-21 02:19:32 PDT
(In reply to comment #43)
> Attachment 56587 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/platform/graphics/Color.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
> WebCore/html/HTMLTokenizer.cpp:58:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 2 in 19 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This style error is due to the separate source inclusions and was not introduced by me.
Comment 45 Darin Adler 2010-06-12 18:26:07 PDT
Comment on attachment 56587 [details]
centralize gperf usage

Seems OK. Not obviously a big improvement, but I suppose we can all adopt the build style the Chrome team decided on instead of using the make file. I'm a bit sad, though, because originally DerivedSources.make was going to be shared by all build systems. It seems that dream has died!
Comment 46 Andras Becsi 2010-06-13 07:11:23 PDT
Comment on attachment 56587 [details]
centralize gperf usage

(In reply to comment #45)
> (From update of attachment 56587 [details])
> Seems OK. Not obviously a big improvement, but I suppose we can all adopt the build style the Chrome team decided on instead of using the make file. I'm a bit sad, though, because originally DerivedSources.make was going to be shared by all build systems. It seems that dream has died!

Thanks a lot Darin. First half landed in r61091.

After working on it a bit more I'll submit the second half which should fix the linking.
Comment 47 Andras Becsi 2010-06-22 06:58:56 PDT
Created attachment 59364 [details]
patch

This patch makes the make-hash-tools.pl script generate C++ code for all gperf files now, which are compiled in their own compilation unit, so the linking error with gcc 4.5 in debug mode gets fixed.

Unfortunately this change introduces a potential performance drop of 0.5%-1.3% on our page loading test suite (http://gitorious.org/methanol) with QtWebKit trunk which has 1.5% - 3.0% deviation and thus is inconclusive and insignificant. This drop might be due to some previously inlined functions becoming real function calls but methanol does not convince me that the shown drop is real because some pages show near to 1% performance increase with 0.5-1.0% deviation so this needs further investigation with other PLT suites on other ports of WebKit (Methanol PLT uses locally mirrored popular webpages and measures loading time using a JS framework). 

I tried several other solutions to fix the issue but none of them worked.
(Before somebody mentions it again: extern "C" doesn't fix the linking problem because it is due to gcc 4.5's disabled inlining in debug mode and not because of conflicting C and C++ linkage.)
 
We could work around the issue by patching the generated sources as mentioned earlier, but this hack raises revulsive feeling in me.
If someone has a working (or partially working) solution, please contribute so we can work out a solution which has definitely no performance penalties.

None the less I set r? for this patch because there is a memory usage dropdown due to the smaller code size.
Comment 48 WebKit Review Bot 2010-06-22 07:58:28 PDT
Attachment 59364 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3340601
Comment 49 Andras Becsi 2010-06-22 08:42:02 PDT
Created attachment 59373 [details]
patch

Also apply changes to chromium sources.
Comment 50 WebKit Review Bot 2010-06-22 10:16:02 PDT
Attachment 59373 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3283563
Comment 51 Lucas De Marchi 2010-06-22 13:46:52 PDT
Comment on attachment 59373 [details]
patch

>+    my $htmlEntityNamesGenerated   = "$outdir/HTMLEntityNamesHash.hpp";

Why are you using .hpp? There's only 1 header in the hole webkit tree named .hpp. I think would be better to name it as .h instead.


Also, CMake changes are missing. As we talked previously, I can help with that. Is there anything to be done besides adding the .cpp files?
Comment 52 WebKit Review Bot 2010-06-22 18:59:02 PDT
Attachment 59373 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3330593
Comment 53 Andras Becsi 2010-06-23 11:27:25 PDT
Created attachment 59540 [details]
updated patch

Thanks Lucas you remind me, I've updated the CMake file now. Also updated the chromium gyp file, which I missed previously. I can't find the output of the Windows EWS, and if it is correct on Windows I also need to add the new files to its project file.
This 7 or more different build systems seems hardly maintainable for the long term.
Comment 54 WebKit Review Bot 2010-06-23 13:41:39 PDT
Attachment 59540 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3284625
Comment 55 Andras Becsi 2010-06-23 14:13:58 PDT
Created attachment 59562 [details]
updated patch

Updated patch to work on ToT.
Comment 56 WebKit Review Bot 2010-06-23 15:19:09 PDT
Attachment 59540 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3339619
Comment 57 WebKit Review Bot 2010-06-23 15:21:39 PDT
Attachment 59540 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3321686
Comment 58 WebKit Review Bot 2010-06-23 17:59:20 PDT
Attachment 59562 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3338696
Comment 59 WebKit Review Bot 2010-06-23 21:53:48 PDT
Attachment 59562 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3276696
Comment 60 Andras Becsi 2010-06-24 00:39:03 PDT
(In reply to comment #58)
> Attachment 59562 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/3338696

Seems that the chromium ews has an incremental build problem with my patch, because locally I can build it with a clean build. I might have missed some dependency setting in the gyp file, could someone with more chromium knowledge check it, please?
Also the windows ews is not too helpful, so I'll have to set up a build environment for it.
Comment 61 Evan Martin 2010-06-24 08:43:22 PDT
If you are confident that your patch is reasonable (it seems likely, given the state of the non-chrome bots), perhaps a better approach is to ask on #webkit for a chrome developer to do a clean build of the relevant webkit bots after you check it in.

(We have to do this occasionally for non-webkit bits of chrome, so I don't think it's too much to ask.)
Comment 62 Leandro Pereira 2010-06-24 10:41:49 PDT
Created attachment 59674 [details]
EFL port patch

Attachment #59562 [details] breaks the EFL build; applying this patch after #59562 has been applied fixes the build.
Comment 63 Gustavo Sverzut Barbieri 2010-07-08 19:26:35 PDT
anyone reviewing abecsi's patch? Looks good to me.

abecsi, could you integrate Leandro's patch in yours so you don't break EFL when it is applied?
Comment 64 Andras Becsi 2010-07-09 02:05:59 PDT
Created attachment 61019 [details]
updated patch with EFL changes

I rebased the patch to ToT and incorporated the cmake changes by Leandro. Thanks for the help.
Comment 65 WebKit Review Bot 2010-07-09 06:59:39 PDT
Attachment 61019 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3499072
Comment 66 Ling Kun 2010-07-21 00:26:00 PDT
This patch can't patched to latest webkit svn r63640. 
git apply --check ~/webkit_gperf.patch
error: patch failed: ChangeLog:1
error: ChangeLog: patch does not apply
error: patch failed: WebCore/CMakeLists.txt:525
error: WebCore/CMakeLists.txt: patch does not apply
error: patch failed: WebCore/ChangeLog:1
error: WebCore/ChangeLog: patch does not apply
error: patch failed: WebCore/WebCore.gyp/WebCore.gyp:692
error: WebCore/WebCore.gyp/WebCore.gyp: patch does not apply
error: patch failed: WebCore/WebCore.xcodeproj/project.pbxproj:22118
error: WebCore/WebCore.xcodeproj/project.pbxproj: patch does not apply
error: patch failed: WebKit/chromium/ChangeLog:1
error: WebKit/chromium/ChangeLog: patch does not apply
error: patch failed: WebKit/qt/ChangeLog:1
error: WebKit/qt/ChangeLog: patch does not apply
Comment 67 Andras Becsi 2010-07-23 04:54:10 PDT
Created attachment 62415 [details]
updated patch

Updated patch to ToT.
Comment 68 Andras Becsi 2010-07-23 05:01:18 PDT
Could someone familiar with the Windows build system check the patch, or kick the EWS to force a clean build, I suspect a clean build should fix it.
I'm struggling to configure a build environment capable of building the Windows port, but am unsuccessful yet to build the trunk.

Thanks in advance.
Comment 69 WebKit Review Bot 2010-07-23 09:49:57 PDT
Attachment 62415 [details] did not build on win:
Build output: http://queues.webkit.org/results/3399761
Comment 70 Andras Becsi 2010-08-09 01:37:48 PDT
Created attachment 63867 [details]
updated patch

Updated patch to ToT and hopefully fixed Windows build.
Comment 71 Andras Becsi 2010-08-09 02:03:04 PDT
Created attachment 63870 [details]
updated patch

Update to the right ToT.
Comment 72 Andras Becsi 2010-08-09 05:23:45 PDT
Created attachment 63889 [details]
updated patch

Somehow the Windows file endings got lost from the vcproj file. Now it should be up-to-date.
Comment 73 Andras Becsi 2010-08-23 09:06:59 PDT
Created attachment 65127 [details]
updated to ToT

Updated the patch to ToT after recent changes on the HTML Parser. Nevertheless of https://bugs.webkit.org/show_bug.cgi?id=43981 on Windows svn-apply still cannot apply a this, so if the EWS also fails I will reopen the above bug.
Could someone please review this?
Comment 74 Andras Becsi 2010-08-30 09:40:39 PDT
Created attachment 65924 [details]
updated patch

Updated to ToT
Comment 75 Jerry Caldicot 2010-09-01 10:33:47 PDT
(In reply to comment #74)
> Created an attachment (id=65924) [details]
> updated patch
> 
> Updated to ToT

tried to apply both the patch from 08-30 and 08-23 resulting in the same error:


ERROR: net-libs/webkit-gtk-1.2.3 failed:
 *   Failed Patch: gperf.patch!
 *
 * Call stack:
 *     ebuild.sh, line   54:  Called src_prepare
 *   environment, line 3181:  Called epatch '/usr/local/portage/net-libs/webkit-gtk/files/gperf.patch'
 *   environment, line 1838:  Called die
 * The specific snippet of code:
 *               die "Failed Patch: ${patchname}!";
Comment 76 Andras Becsi 2010-09-03 08:22:52 PDT
Created attachment 66501 [details]
updated to ToT

Updated patch because of CMakeLists.txt conflicts.
This change (and the previous ones) locally works for me on Windows, svn-apply just can't apply it because of some cygwin patch bug, but this issue shouldn't prevent the review, since the Windows change only touches the vcproj file.
Seems that reviewers are scared away by this bug, 25 people are CCed including me, but no one even dares to comment on the patches. I slowly lose my motivation in updating a patch no reviewer cares about, but this issue blocks our migration to gcc 4.5.
Comment 77 Csaba Osztrogonác 2010-09-28 08:58:31 PDT
Comment on attachment 66501 [details]
updated to ToT

This solution is based on suggestions of bdash (https://bugs.webkit.org/show_bug.cgi?id=29244#c5).
There wasn't any objection, so I can't see any reason why not to give r+ and block migrating to gcc 4.5 any further.
Comment 78 Andras Becsi 2010-09-28 09:01:56 PDT
Comment on attachment 66501 [details]
updated to ToT

Clearing flags on attachment: 66501

Committed r68521: <http://trac.webkit.org/changeset/68521>
Comment 79 Andras Becsi 2010-09-28 09:02:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 80 Stephen White 2010-09-28 12:31:04 PDT
After rolling past this CL, we're getting errors in the downstream chromium/win release builder:

http://build.chromium.org/buildbot/waterfall/builders/Chromium%20Builder/builds/35014/steps/compile/logs/stdio

I'm not certain this CL is to blame -- it's definitely something in the range 68504:68527.  I thought I'd mention it in case someone had any insight.
Comment 81 Stephen White 2010-09-28 13:24:10 PDT
> I'm not certain this CL is to blame -- it's definitely something in the range 68504:68527.  I thought I'd mention it in case someone had any insight.

Confirmed:  r68520 is fine, r68521 fails.  It seems to be related to the #include of system headers inside a namespace, which VS2008 doesn't like.  ColorData.cpp has:

namespace WebCore {
#include "ColorDataHash.h"

...

and ColorDataHash.h has #include <string.h>.

Same for DocTypeStringsHash.h.
Comment 82 Stephen White 2010-09-28 13:44:40 PDT
I've opened https://bugs.webkit.org/show_bug.cgi?id=46751 for this error, and I have a fix that I'll try out on that bug to get EWS bot results.
Comment 84 Patrick R. Gansterer 2010-10-03 12:13:58 PDT
Is there a reason why we put the code of _whole_ files (HashTools.h, ColorData.cpp and DocTypeStrings.cpp) into the perl script? I can't find any generated code in these files.
Why are they not simply in the SVN tree?

I don't know the exact problem now, but the different CR/CRLF line endings of gperf and perl make problems on Windows without cygwin since this patch.
An other problem I have with this patch is, that gperf needs to be in the path now and I can't provide it via an argument.
Comment 85 Andras Becsi 2010-10-03 15:44:25 PDT
(In reply to comment #84)
> Is there a reason why we put the code of _whole_ files (HashTools.h, ColorData.cpp and DocTypeStrings.cpp) into the perl script? I can't find any generated code in these files.
> Why are they not simply in the SVN tree?

This is a remainder of a previous design, there is no actual cause for the mentioned code to be in the perl script, except for being in a central place and not to increase the number of source files unnecessarily.
At least HashTools.h could be in it's own file.

> 
> I don't know the exact problem now, but the different CR/CRLF line endings of gperf and perl make problems on Windows without cygwin since this patch.

Moving the mentioned code to separate files wouldn't fix this issue either, would it? If it would, I'll move those to separate files as you suggested.

> An other problem I have with this patch is, that gperf needs to be in the path now and I can't provide it via an argument.

I could expand the scripts to accept the gperf command via an argument if this is a problem but the centralization of the gperf commands was introduced in http://trac.webkit.org/changeset/61091 a long time ago, and there was no option to provide the gperf command as argument before that neither (except for a hacked port specific project file).
Comment 86 Patrick R. Gansterer 2010-10-04 01:52:15 PDT
(In reply to comment #85)
> This is a remainder of a previous design, there is no actual cause for the mentioned code to be in the perl script, except for being in a central place and not to increase the number of source files unnecessarily.
If it's not a big taks for you I'd like to see 3 "real" files.

> Moving the mentioned code to separate files wouldn't fix this issue either, would it? If it would, I'll move those to separate files as you suggested.
I'm not sure, but maybe. :-/

> I could expand the scripts to accept the gperf command via an argument if this is a problem but the centralization of the gperf commands was introduced in http://trac.webkit.org/changeset/61091 a long time ago, and there was no option to provide the gperf command as argument before that neither (except for a hacked port specific project file).
You didn't changed the CMake files in r61091 ;-).

Sorry for the following question: Is there much value in the make-hash-tools.pl? After my requested changes it only consist out of two gperf calls and IMHO it's more clear to call gperf from the "native" buildsystem, because it can provide better error handling than a wrapper scipt.

If we like the centralized definition of gperf (=good idea) I think that we should call it like:
"make-gperf.pl path/to/ColorData.gperf > output/dir/ColorDataHash.h" and
"make-gperf.pl path/to/DocTypeStrings.gperf > output/dir/DocTypeStringsHash.h"
Comment 87 Andras Becsi 2010-10-04 02:36:51 PDT
(In reply to comment #86)
> You didn't changed the CMake files in r61091 ;-).
> 
Right. Sorry.

> If we like the centralized definition of gperf (=good idea) I think that we should call it like:
> "make-gperf.pl path/to/ColorData.gperf > output/dir/ColorDataHash.h" and
> "make-gperf.pl path/to/DocTypeStrings.gperf > output/dir/DocTypeStringsHash.h"

I would like this aproach, because the centralization was one of the main motives of the patch. If you need extra options this would be a way to handle it.