[webkit-reviews] review denied: [Bug 197060] WebCore: auto-initialize stack variables : [Attachment 367730] patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied JF Bastien
<jfbastien at apple.com>'s request for review:
Bug 197060: WebCore: auto-initialize stack variables
https://bugs.webkit.org/show_bug.cgi?id=197060

Attachment 367730: patch

https://bugs.webkit.org/attachment.cgi?id=367730&action=review




--- Comment #6 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 367730
  --> https://bugs.webkit.org/attachment.cgi?id=367730
patch

Please add a ChangeLog. r- on this technicality. Also some questions and
suggestions.

1. I can see how this can be somewhat perf neutral on optimized builds, but
what does this do to debug builds? We don't want tests to become slower in
debug mode.

2. How does this interact with UBSan, will we still be getting diagnostics for
uninitialized variables? I think that it's still a net win even if this breaks
this aspect of UBSan functionality, but it would be useful to know how exactly
it works.

3. Similarly, how does this interact with static analysis, will clang static
analyzer now think that all these variables are initialized?

4. For the 1.9% WebCore size regression, I'm assuming that all of that are
additional initializations, so it's quite surprising that so much new code
isn't registering on benchmarks. Is there some explanation for that?

5. There was a discussion about initializing all variables by default recently
in bug 196855, with regards to silencing Coverity warnings. The idea that it's
OK to introduce some perf regressions as long as our current benchmark suite
can't detect those is not universally agreed upon. The right place to discuss
that is the webkit-dev mailing list.