Bug 62443 – Make dynamic annotations weak symbols and prevent identical code folding by the linker

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Timur Iskhodzhanov]

Make dynamic annotations weak symbols and prevent identical code folding by the linker

[Timur Iskhodzhanov]

Created attachment 96716 [details]
Patch

[Timur Iskhodzhanov]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

What happens w/o this change?

[Dmitry Vyukov]

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.

[Eric Seidel (no email)]

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

[Dmitry Vyukov]

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

[Dmitry Vyukov]

Ping

[David Levin]

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.

[Dmitry Vyukov]

Should we fix that define?
If not, what should we do to land it?

[David Levin]

(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+.

[Dmitry Vyukov]

Created attachment 98327 [details]
Proposed patch

[David Levin]

Comment on attachment 96716 [details]
Patch

Please obsolete the old patch when uploading a new one.

[WebKit Review Bot]

Comment on attachment 98327 [details]
Proposed patch

Clearing flags on attachment: 98327

Committed r89547: <http://trac.webkit.org/changeset/89547>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

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

[Dmitry Vyukov]

Created attachment 98490 [details]
Proposed patch

[Dmitry Vyukov]

Another take that does not use weak symbols.
PTAL

[WebKit Review Bot]

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.

[Dmitry Vyukov]

Created attachment 98712 [details]
Proposed patch

[Dmitry Vyukov]

PTAL

[WebKit Review Bot]

Comment on attachment 98712 [details]
Proposed patch

Clearing flags on attachment: 98712

Committed r90477: <http://trac.webkit.org/changeset/90477>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.