Bug 96514

Summary: [Gtk EWS] EWS for Gtk+ should generate the mangled symbols for unresolved symbols
Product: WebKit Reporter: Vivek Galatage <vivekgalatage>
Component: WebKitGTKAssignee: Gustavo Noronha (kov) <gustavo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dpranke, goatman.py, gtk-ews, gustavo, mrobinson, pnormand, vivekg, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 115323, 115732    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Test patch
gtk-ews: commit-queue-
Patch gtk-ews: owner-

Description Vivek Galatage 2012-09-12 07:51:05 PDT
It would be great if EWS for Gtk+ generate the mangled version of the unresolved symbols. 

Currently the non-mangled symbols are generated thereby the developer must have gtk+ environment to generate these symbols. But considering cross-platform nature of Webkit, it would be really nice if this information can be generated by the EWS so then the developers can directly use these symbols while exporting it.
Comment 1 Philippe Normand 2012-09-13 07:54:30 PDT
+Gustavo,Martin

Any idea how we can accomplish this? :)
Comment 2 Martin Robinson 2012-09-13 09:28:02 PDT
If we actually know the missing symbols we could run c++filt on the symbol table of the library and then create a map to map backward to the mangled symbol.
Comment 3 Zan Dobersek 2013-04-27 23:25:47 PDT
There's the --no-demangle option that can be passed to the linker to stop the missing symbol being written out in the demangled form.
There's also the COLLECT_NO_DEMANGLE env which has the same effect when set.
http://linux.die.net/man/1/ld

I'll see what the ld and gold linkers support.
Comment 4 Zan Dobersek 2013-04-28 03:19:27 PDT
Setting the COLLECT_NO_DEMANGLE in the environment works with both linkers, so that's what I'd propose doing on the EWS systems.

I also think this would make sense on the buildbots, just so someone can see what symbols need exporting when trying to fix the build.

EWS adjustments should be done by their owners while for the buildbots the variable can be listed somewhere in the buildbot configuration files.
Comment 5 Philippe Normand 2013-04-29 00:37:38 PDT
(In reply to comment #4)
> Setting the COLLECT_NO_DEMANGLE in the environment works with both linkers, so that's what I'd propose doing on the EWS systems.
> 
> I also think this would make sense on the buildbots, just so someone can see what symbols need exporting when trying to fix the build.
> 
> EWS adjustments should be done by their owners while for the buildbots the variable can be listed somewhere in the buildbot configuration files.

I've set that variable on my gtk-wk2-ews. Thanks for investigating this issue!
Comment 6 Gustavo Noronha (kov) 2013-04-29 06:19:28 PDT
Created attachment 199998 [details]
Patch
Comment 7 Gustavo Noronha (kov) 2013-04-29 06:20:20 PDT
How about something like this, so there's one less thing we need to remember when running the queue?
Comment 8 Zan Dobersek 2013-04-29 07:01:23 PDT
Comment on attachment 199998 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=199998&action=review

> Tools/Scripts/webkitpy/port/gtk.py:46
> +        os.environ['COLLECT_NO_DEMANGLE'] = '1'

Does this properly translate into the EWS's environment?
Comment 9 Zan Dobersek 2013-04-30 03:20:14 PDT
Hrm, the proposed hack ... is a hack. It probably would work for EWSs.

Another solution that I should have pushed for (and I apologize for not doing so) is to pass the --no-demangle flag to the linker. This can be easily done by listing the flag in the AM_LDFLAGS assignment in the root GNUmakefile.am. This way the linker outputs mangled symbols by default, though if someone needs clarification on what the symbol is representing, he can still run the symbol through c++filt.

This would remove the need for exporting the COLLECT_NO_DEMANGLE env on both EWSs and the builders.

Thoughts?
Comment 10 Gustavo Noronha (kov) 2013-05-02 08:44:55 PDT
I like that solution. The mangled symbols are usually more interesting for developers, and anyone who needs it can demangle the symbol very easily, so I think it's OK to add it to the linker arguments.
Comment 11 Zan Dobersek 2013-05-08 01:33:05 PDT
Created attachment 201043 [details]
Test patch

Just a test patch to see how EWSs now behave.
Comment 12 kov's GTK+ EWS bot 2013-05-08 01:41:56 PDT
Comment on attachment 201043 [details]
Test patch

Attachment 201043 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/436014
Comment 13 Zan Dobersek 2013-05-08 01:43:16 PDT
Success:

./.libs/libWebCoreInternals.a(./.libs/../Source/WebCore/testing/.libs/libWebCoreInternals_la-Internals.o):Internals.cpp:function _ZN7WebCore9Internals23inspectorHighlightRectsEPNS_8DocumentERi: error: undefined reference to '_ZN7WebCore14ClientRectListC1ERKN3WTF6VectorINS_9FloatQuadELm0ENS1_15CrashOnOverflowEEE'
collect2: error: ld returned 1 exit status
make[1]: *** [Programs/DumpRenderTree] Error 1

Closing the bug.
Comment 14 Zan Dobersek 2013-05-08 01:43:48 PDT
Comment on attachment 199998 [details]
Patch

Clearing the r? flag.
Comment 15 Vivek Galatage 2013-05-08 01:46:30 PDT
(In reply to comment #13)
> Success:
> 
> ./.libs/libWebCoreInternals.a(./.libs/../Source/WebCore/testing/.libs/libWebCoreInternals_la-Internals.o):Internals.cpp:function _ZN7WebCore9Internals23inspectorHighlightRectsEPNS_8DocumentERi: error: undefined reference to '_ZN7WebCore14ClientRectListC1ERKN3WTF6VectorINS_9FloatQuadELm0ENS1_15CrashOnOverflowEEE'
> collect2: error: ld returned 1 exit status
> make[1]: *** [Programs/DumpRenderTree] Error 1
> 
> Closing the bug.

Thank you for handling this :) Really appreciated!
Comment 16 Gustavo Noronha (kov) 2013-05-08 08:07:58 PDT
There's a catch! The EWS bots have the environment variable set. I'll take it off so we can get a real test run done, need to ask rego and philln to remove it from theirs too.
Comment 17 Zan Dobersek 2013-05-12 13:55:27 PDT
Yeah, true, didn't cross my mind.

I'll ensure people have the env unset and then retest, IMO it should be just a formality.
Comment 18 Zan Dobersek 2013-05-13 13:23:24 PDT
Created attachment 201624 [details]
Patch

Test #2
Comment 19 kov's GTK+ EWS bot 2013-05-13 14:09:57 PDT
Comment on attachment 201624 [details]
Patch

Attachment 201624 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/468194
Comment 20 Zan Dobersek 2013-05-13 14:11:28 PDT
All EWSs had the COLLECT_NO_DEMANGLE env removed, and the mangled symbols in the output persist:
./.libs/libWebCoreInternals.a(./.libs/../Source/WebCore/testing/.libs/libWebCoreInternals_la-Internals.o):Internals.cpp:function _ZN7WebCore9Internals23inspectorHighlightRectsEPNS_8DocumentERi: error: undefined reference to '_ZN7WebCore14ClientRectListC1ERKN3WTF6VectorINS_9FloatQuadELm0ENS1_15CrashOnOverflowEEE'
collect2: error: ld returned 1 exit status
make[1]: *** [Programs/DumpRenderTree] Error 1

This now works as expected, closing the bug.
Comment 21 Gustavo Noronha (kov) 2013-08-15 14:12:52 PDT
Looks like we need to do something for this to work on the buildbots as well? Or did we regress?

http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/39871/steps/compile-webkit/logs/stdio
Comment 22 Zan Dobersek 2013-08-15 14:25:32 PDT
Everything on the builder seems fine (i.e. --no-demangle is present in LDFLAGS), don't really have an idea where this might be failing.
Comment 23 Zan Dobersek 2013-08-15 14:35:31 PDT
Still works on the 64-bit debug builder:
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20WK1/builds/3611/steps/compile-webkit/logs/stdio