Bug 62443

Summary: Make dynamic annotations weak symbols and prevent identical code folding by the linker
Product: WebKit Reporter: Timur Iskhodzhanov <timurrrr>
Component: New BugsAssignee: Dmitry Vyukov <dvyukov>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, dvyukov, eric, ggaren, jamesr, levin, oliver, timurrrr, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 63252    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Proposed patch
none
Proposed patch
none
Proposed patch none

Description Timur Iskhodzhanov 2011-06-10 02:16:38 PDT
Make dynamic annotations weak symbols and prevent identical code folding by the linker
Comment 1 Timur Iskhodzhanov 2011-06-10 02:18:00 PDT
Created attachment 96716 [details]
Patch
Comment 2 Timur Iskhodzhanov 2011-06-10 02:23:15 PDT
Hi David, James,

Can you please review this small patch for me?

Or can you recommend someone else to review it for me?

FYI, all the changes done only affect anything when USE_DYNAMIC_ANNOTATIONS is defined => should only affect Chromium and only in the Debug- or Valgrind- build set up.

Thanks,
Timur
Comment 3 Eric Seidel (no email) 2011-06-13 15:01:47 PDT
I CC'd folks who know things about code generation. :)  However, reading the bug I now see this is chromium-only.  I'm still not sure who is particularly good for reviewing this.
Comment 4 Eric Seidel (no email) 2011-06-13 15:01:58 PDT
What happens w/o this change?
Comment 5 Dmitry Vyukov 2011-06-14 01:25:25 PDT
Hi Eric, 

> What happens w/o this change?

As for __attribute__((weak)), our tools define strong symbols with the same names, so if WebKit annotations are not weak symbols we either get linker errors or our symbols do not trump WebKit symbols.

As for Identical Code Folding and strange function bodies, linkers can fold symbols with identical bodies (as you may guess it especially affects empty functions because there are a lot of them). Then some tools (in particular, Valgrind) are unable to intercept such functions. It's possible to suppress such behavior with linker-specific flags, but (1) the problem is hard to debug (so it will make harm before people guess they need additional flags), (2) it can significantly increase binary size (especially crucial for ARM devices), (3) libraries have loose control over linker flags and (4) it's better to provide a solution that works either way anyway.
Comment 6 Eric Seidel (no email) 2011-06-14 07:53:32 PDT
So these functions are specifically added to WTF to provide hook-points for Valgrind, is that correct?

Where are these functions called?  (All over the place, I assume?)
Comment 7 Dmitry Vyukov 2011-06-14 08:01:10 PDT
> So these functions are specifically added to WTF to provide hook-points for Valgrind, is that correct?

Yes. These are annotations for dynamic tools like Valgrind.

> Where are these functions called?  (All over the place, I assume?)

Currently we added them only to thread-safe reference counters (wtf/ThreadSafeRefCounted.h) and strings (wtf/text/StringStatics.cpp, in order to deal with shared string bodies).
Comment 8 Dmitry Vyukov 2011-06-23 01:55:37 PDT
Ping
Comment 9 David Levin 2011-06-23 02:00:08 PDT
Comment on attachment 96716 [details]
Patch

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

Looks ok.

> Source/JavaScriptCore/wtf/DynamicAnnotations.h:80
> +#   define WTF_DYNAMIC_ANNOTATIONS_ATTRIBUTE_WEAK

This
#   define
style isn't used elsewhere in WebKit as far as I know.
Comment 10 Dmitry Vyukov 2011-06-23 02:03:09 PDT
Should we fix that define?
If not, what should we do to land it?
Comment 11 David Levin 2011-06-23 02:05:34 PDT
(In reply to comment #10)
> Should we fix that define?
> If not, what should we do to land it?

Ideally if you'd like me to cq+ it or you can get someone else to land it who has commit permission.

Or you can upload with the fix and put in "Reviewed by David Levin." in place of the oops since I gave you an r+. Then you only need to set cq? (and leave the r blank). Any WK committer can then change that to cq+.
Comment 12 Dmitry Vyukov 2011-06-23 02:14:35 PDT
Created attachment 98327 [details]
Proposed patch
Comment 13 David Levin 2011-06-23 02:16:10 PDT
Comment on attachment 96716 [details]
Patch

Please obsolete the old patch when uploading a new one.
Comment 14 WebKit Review Bot 2011-06-23 02:55:51 PDT
Comment on attachment 98327 [details]
Proposed patch

Clearing flags on attachment: 98327

Committed r89547: <http://trac.webkit.org/changeset/89547>
Comment 15 WebKit Review Bot 2011-06-23 02:55:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Yury Semikhatsky 2011-06-23 07:12:08 PDT
The patch was rolled out in http://trac.webkit.org/changeset/89566 since it caused Chromium Linux Debug to crash during startup. Debug bots also crashed: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20%28dbg%29%281%29/builds/7586/steps/webkit_tests/logs/stdio
Comment 17 Dmitry Vyukov 2011-06-24 06:52:57 PDT
Created attachment 98490 [details]
Proposed patch
Comment 18 Dmitry Vyukov 2011-06-24 06:53:23 PDT
Another take that does not use weak symbols.
PTAL
Comment 19 WebKit Review Bot 2011-06-24 06:54:30 PDT
Attachment 98490 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/DynamicAnnotations.cpp:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/DynamicAnnotations.cpp:38:  More than one command on the same line  [whitespace/newline] [4]
Source/JavaScriptCore/wtf/DynamicAnnotations.cpp:41:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/DynamicAnnotations.cpp:41:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Dmitry Vyukov 2011-06-27 06:27:05 PDT
Created attachment 98712 [details]
Proposed patch
Comment 21 Dmitry Vyukov 2011-06-27 06:27:42 PDT
PTAL
Comment 22 WebKit Review Bot 2011-07-06 11:59:03 PDT
Comment on attachment 98712 [details]
Proposed patch

Clearing flags on attachment: 98712

Committed r90477: <http://trac.webkit.org/changeset/90477>
Comment 23 WebKit Review Bot 2011-07-06 11:59:09 PDT
All reviewed patches have been landed.  Closing bug.