Summary: | WebCore: auto-initialize stack variables | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | JF Bastien <jfbastien> | ||||||
Component: | WebCore Misc. | Assignee: | Keith Rollin <krollin> | ||||||
Status: | ASSIGNED --- | ||||||||
Severity: | Normal | CC: | ap, bfulgham, dean_johnson, fpizlo, jfbastien, krollin, mjs, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
JF Bastien
2019-04-18 09:28:09 PDT
Created attachment 367730 [details]
patch
Comment on attachment 367730 [details] 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. Created attachment 367734 [details]
patch
Add changelog
(In reply to Alexey Proskuryakov from comment #6) > Comment on attachment 367730 [details] > patch > > Please add a ChangeLog. r- on this technicality. Also some questions and > suggestions. Done. > 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. It generates more code in debug builds, but we didn't measure the performance impact. If debug performance matters I suggest not just using -O0 because it's quite terrible. There's a handful of optimizations that you could turn on that would compile *faster* or slightly slower (depends which) while still generating totally debuggable code. There's also ongoing work to create a better low-optimization pipeline in LLVM. I think this discussion should be held separately from this patch. > 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. It doesn't interact with UBSan. UBSan also doesn't give you diagnostics for uninitialized variables. Only msan does this for stack values. > 3. Similarly, how does this interact with static analysis, will clang static > analyzer now think that all these variables are initialized? It doesn't interact with static analysis (nor the warnings). The diagnostics remain unchanged. > 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? Yes, I've sent detailed information to the folks I've CC'ed to the review. Some is caused by large stack buffers, some is new code (when memset / memcpy gets inlined), and some is rodata globals that get memcpy'd to the stack (because that particular stack variable is big and isn't a repeated byte pattern). This in turn influences inlining decisions. If you want all the details please look at the related radars, I've done extensive measurements and optimizations. Keep in mind that there's still ongoing work to optimize this further (by myself and others in the LLVM project). > 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. I don't follow webkit-dev, but I suggest reading the radar and related ones for the discussions that have happened. Comment on attachment 367734 [details] patch > I don't follow webkit-dev, but I suggest reading the radar and related ones for the discussions that have happened. What I'm saying is that we shouldn't make such policy changes without a webkit-dev discussion. > It doesn't interact with UBSan. UBSan also doesn't give you diagnostics for uninitialized variables. Only msan does this for stack values.
OK. Does it break MSan uninitialized variable diagnostics? If it doesn't explicitly interact with it, it probably does break it, as variables are now initialized.
(In reply to Alexey Proskuryakov from comment #10) > > It doesn't interact with UBSan. UBSan also doesn't give you diagnostics for uninitialized variables. Only msan does this for stack values. > > OK. Does it break MSan uninitialized variable diagnostics? If it doesn't > explicitly interact with it, it probably does break it, as variables are now > initialized. This patch turns on auto-init for platforms that build with Xcode. Which of those do you run msan on? Currently none, but we are very interested in enabling diagnostics of this kind. |