Make dynamic annotations weak symbols and prevent identical code folding by the linker
Created attachment 96716 [details] Patch
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
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.
What happens w/o this change?
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.
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?)
> 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).
Ping
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.
Should we fix that define? If not, what should we do to land it?
(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+.
Created attachment 98327 [details] Proposed patch
Comment on attachment 96716 [details] Patch Please obsolete the old patch when uploading a new one.
Comment on attachment 98327 [details] Proposed patch Clearing flags on attachment: 98327 Committed r89547: <http://trac.webkit.org/changeset/89547>
All reviewed patches have been landed. Closing bug.
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
Created attachment 98490 [details] Proposed patch
Another take that does not use weak symbols. PTAL
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.
Created attachment 98712 [details] Proposed patch
PTAL
Comment on attachment 98712 [details] Proposed patch Clearing flags on attachment: 98712 Committed r90477: <http://trac.webkit.org/changeset/90477>