Bug 55221 – Build fix for Intel ICC Compiler

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Alexis Menard (darktears)]

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.

[Alexis Menard (darktears)]

Created attachment 83817 [details]
Proposed patch to fix the build.

[Darin Adler]

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

Created attachment 83832 [details]
V2 with a comment.

[Alexey Proskuryakov]

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

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

Created attachment 84070 [details]
New patch V2 with Alexey's comments

[Alexey Proskuryakov]

Comment on attachment 84070 [details]
New patch V2 with Alexey's comments

Thank you!

[WebKit Commit Bot]

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 on attachment 84070 [details]
New patch V2 with Alexey's comments

Trying again...

[Eric Seidel (no email)]

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]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.