undefined reference errors when linking due to gperf and inlining

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Priit Laes (IRC: plaes)]

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.

[Priit Laes (IRC: plaes)]

After reading GCC bug report below, should we fix our gperf-files instead??

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41194

[Priit Laes (IRC: plaes)]

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);
  }

[Jan Alonzo]

*** Bug 29778 has been marked as a duplicate of this bug. ***

[Priit Laes (IRC: plaes)]

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)'"

[Priit Laes (IRC: plaes)]

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)

[Evan Martin]

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.

[Evan Martin]

Does this still happen?  I haven't heard anyone complain about it in a while.

[Priit Laes (IRC: plaes)]

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

[he zhitong]

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

[Stephan Aßmus]

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.

[Ryan Leavengood]

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

[Andras Becsi]

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

[Andras Becsi]

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?

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 54317 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1907028

[Darin Adler]

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?

[Andras Becsi]

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

[Andras Becsi]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 55713 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2225112

[Kenneth Rohde Christiansen]

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.

[Andras Becsi]

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.

[Andras Becsi]

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.

[Andras Becsi]

Created attachment 55757 [details]
Refactor gperf v2 update

Updated patch to ToT.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 55757 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2194134

[Andras Becsi]

Comment on attachment 55757 [details]
Refactor gperf v2 update

The chromium build system doesn't like me.

[Andras Becsi]

Created attachment 55845 [details]
Refactor gperf v3

Multiple build systems are a torture.

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

Attachment 55845 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2173167

[Andras Becsi]

Comment on attachment 55845 [details]
Refactor gperf v3

Ok, then let's try to split this into smaller pieces.

[WebKit Review Bot]

Attachment 55845 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2212168

[Gustavo Sverzut Barbieri]

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

[Andras Becsi]

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

[Lucas De Marchi]

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

[Csaba Osztrogonác]

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

[Stephan Aßmus]

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

[Andras Becsi]

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

[Andras Becsi]

Created attachment 56528 [details]
refactor gperf usage 1st iteration

Just to check the build on the EWS bots.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 56528 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2278348

[Andras Becsi]

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.

[WebKit Review Bot]

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.

[Andras Becsi]

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

[Darin Adler]

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!

[Andras Becsi]

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.

[Andras Becsi]

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.

[WebKit Review Bot]

Attachment 59364 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3340601

[Andras Becsi]

Created attachment 59373 [details]
patch

Also apply changes to chromium sources.

[WebKit Review Bot]

Attachment 59373 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3283563

[Lucas De Marchi]

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?

[WebKit Review Bot]

Attachment 59373 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3330593

[Andras Becsi]

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.

[WebKit Review Bot]

Attachment 59540 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3284625

[Andras Becsi]

Created attachment 59562 [details]
updated patch

Updated patch to work on ToT.

[WebKit Review Bot]

Attachment 59540 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3339619

[WebKit Review Bot]

Attachment 59540 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3321686

[WebKit Review Bot]

Attachment 59562 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3338696

[WebKit Review Bot]

Attachment 59562 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3276696

[Andras Becsi]

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

[Evan Martin]

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

[Leandro Pereira]

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.

[Gustavo Sverzut Barbieri]

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?

[Andras Becsi]

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.

[WebKit Review Bot]

Attachment 61019 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3499072

[Ling Kun]

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

[Andras Becsi]

Created attachment 62415 [details]
updated patch

Updated patch to ToT.

[Andras Becsi]

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.

[WebKit Review Bot]

Attachment 62415 [details] did not build on win:
Build output: http://queues.webkit.org/results/3399761

[Andras Becsi]

Created attachment 63867 [details]
updated patch

Updated patch to ToT and hopefully fixed Windows build.

[Andras Becsi]

Created attachment 63870 [details]
updated patch

Update to the right ToT.

[Andras Becsi]

Created attachment 63889 [details]
updated patch

Somehow the Windows file endings got lost from the vcproj file. Now it should be up-to-date.

[Andras Becsi]

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?

[Andras Becsi]

Created attachment 65924 [details]
updated patch

Updated to ToT

[Jerry Caldicot]

(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}!";

[Andras Becsi]

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.

[Csaba Osztrogonác]

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.

[Andras Becsi]

Comment on attachment 66501 [details]
updated to ToT

Clearing flags on attachment: 66501

Committed r68521: <http://trac.webkit.org/changeset/68521>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.

[Stephen White]

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.

[Stephen White]

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

[Stephen White]

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.

[Csaba Osztrogonác]

GTK buildfixes landed in:
http://trac.webkit.org/changeset/68528
http://trac.webkit.org/changeset/68529
http://trac.webkit.org/changeset/68550

Chromium fix landed it:
http://trac.webkit.org/changeset/68563

[Patrick R. Gansterer]

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.

[Andras Becsi]

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

[Patrick R. Gansterer]

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

[Andras Becsi]

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