Bug 55221

Summary: Build fix for Intel ICC Compiler
Product: WebKit Reporter: Alexis Menard (darktears) <menard>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, eric, menard, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Proposed patch to fix the build.
darin: review+, darin: commit-queue+
V2 with a comment.
none
New patch with Alexey's comments
ap: review+
New patch V2 with Alexey's comments none

Alexis Menard (darktears)
Reported 2011-02-25 08:31:10 PST
With Intel ICC compiler for Linux 64 bits, there is some issue to link against a version of WebKit build with ICC : undefined reference to the destructor of SVGTransformable(). I think ICC is confused with the virtual inheritance and virtual methods in the class.
Attachments
Proposed patch to fix the build. (1.24 KB, patch)
2011-02-25 08:31 PST, Alexis Menard (darktears) 		darin: review+
darin: commit-queue+
DetailsFormatted DiffDiff
V2 with a comment. (1.31 KB, patch)
2011-02-25 10:27 PST, Alexis Menard (darktears) 		no flags
DetailsFormatted DiffDiff
New patch with Alexey's comments (2.05 KB, patch)
2011-02-28 05:11 PST, Alexis Menard (darktears) 		ap: review+
DetailsFormatted DiffDiff
New patch V2 with Alexey's comments (2.07 KB, patch)
2011-02-28 09:03 PST, Alexis Menard (darktears) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Alexis Menard (darktears)
Comment 1 2011-02-25 08:31:40 PST
Created attachment 83817 [details] Proposed patch to fix the build.
Darin Adler
Comment 2 2011-02-25 10:11:49 PST
Comment on attachment 83817 [details] Proposed patch to fix the build. Might have been slightly better to add a brief comment as well. Otherwise I see no reason someone wouldn’t just remove this later.
Alexis Menard (darktears)
Comment 3 2011-02-25 10:27:44 PST
Created attachment 83832 [details] V2 with a comment.
Alexey Proskuryakov
Comment 4 2011-02-25 21:39:50 PST
This is surprising. I thought that Intel ICC was supposed to be an extremely compliant compiler, so perhaps there is a real reason why an implementation is needed? I can't think of any though. I suggest adding the implementation in a .cpp file if that works with ICC, because an inline virtual destructor in a base class causes significant code bloat. Please mark a patch for review, so that it doesn't get overlooked.
Alexis Menard (darktears)
Comment 5 2011-02-28 05:11:20 PST
Created attachment 84041 [details] New patch with Alexey's comments @Alexey : Me too, actually ICC is a very good compiler. Though his main problem is that it's not widely used therefore there is still bugs. For instance there is a huge gap from version 11 to version 12 in term of bugfixing (in a good way). I think if I have a bit of time I will try to reproduce in a small C++ example and fill a bug to Intel. Thanks for the review.
Alexey Proskuryakov
Comment 6 2011-02-28 08:53:00 PST
Comment on attachment 84041 [details] New patch with Alexey's comments +{ + +} We don't put a blank line in empty function bodies. + // This destructor is needed in order to link correctly with Intel ICC. The comment is only needed to prevent accidental removal of the destructor - it would be sufficient (and better) to only have it in .cpp file. People looking for something else in the header don't need to know about this.
Alexis Menard (darktears)
Comment 7 2011-02-28 09:03:36 PST
Created attachment 84070 [details] New patch V2 with Alexey's comments
Alexey Proskuryakov
Comment 8 2011-02-28 09:04:55 PST
Comment on attachment 84070 [details] New patch V2 with Alexey's comments Thank you!
WebKit Commit Bot
Comment 9 2011-02-28 10:21:28 PST
Comment on attachment 84070 [details] New patch V2 with Alexey's comments Rejecting attachment 84070 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2 Last 500 characters of output: ccessibility/AccessibilityMenuList.cpp -o /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityMenuList.o ** BUILD FAILED ** The following build commands failed: WebCore: CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AccessibilityMenuListOption.o /Projects/CommitQueue/Source/WebCore/accessibility/AccessibilityMenuListOption.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2 (1 failure) Full output: http://queues.webkit.org/results/8074328
Alexey Proskuryakov
Comment 10 2011-02-28 10:28:41 PST
Comment on attachment 84070 [details] New patch V2 with Alexey's comments Trying again...
Eric Seidel (no email)
Comment 11 2011-02-28 10:37:19 PST
Odd. The commit-queue hasn't historically had flaky builds in the past. I suspect this will fail again, but it's worth the try!
WebKit Commit Bot
Comment 12 2011-02-28 12:23:19 PST
The commit-queue encountered the following flaky tests while processing attachment 84070 [details]: http/tests/websocket/tests/workers/close-in-shared-worker.html bug 55397 (author: abarth@webkit.org) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 13 2011-02-28 12:25:19 PST
Comment on attachment 84070 [details] New patch V2 with Alexey's comments Clearing flags on attachment: 84070 Committed r79896: <http://trac.webkit.org/changeset/79896>
WebKit Commit Bot
Comment 14 2011-02-28 12:25:25 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.