[Webkit-unassigned] [Bug 196855] Fix Covscan uninitialized after ctor

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=196855

--- Comment #14 from Filip Pizlo <fpizlo at apple.com> ---
(In reply to Eike Rathke from comment #13)
> (In reply to Michael Catanzaro from comment #7)
> > We have several existing Coverity bugs: bug #104114, bug #186798, bug
> > #190468. We should be aware of those as well, especially the metabug
> > #104114.
> I can only see bug #186798 and get "You are not authorized to access" for
> the others. However, that one bug and patch was submitted by Tomas (I wasn't
> aware that old bug still existed) and my patch contains the same old fixes
> if they weren't applied, as I tried to apply everything we have to current
> master, plus a set of new ones resulting from differences between 2.20.x and
> 2.22.7
> 
> > I would remove the HTMLMediaElement.cpp change from this patch,
> > since it's clearly not a UNINIT_CTOR issue, and fix that in a new bug marked
> > against bug #104114 instead.
> Do you want me to split the patch?
> 
> > Also, since there are a relatively large number
> > of whitespace fixes in AccessibilityTableColumn.h, those should be landed
> > separately as well; if it was a smaller amount, I wouldn't consider it worth
> > a separate patch, but that's enough to obscure the fix you've made there.
> That's probably due to an editor not saving trailing spaces (which is a good
> thing, IMHO). And also already in the old patch. But well, yes, that's
> easily squeezable to the one changed line.
> 
> > BTW, welcome Eike. :) Darin, Eike is the new maintainer of Red Hat's WebKit
> > and Chromium packages.
> Thanks :-)
> 
> 
> > > Source/JavaScriptCore/dfg/DFGOSRExit.h:135
> > > -    ExtraInitializationLevel extraInitializationLevel;
> > > +    ExtraInitializationLevel extraInitializationLevel { };
> > 
> > You could choose to be explicit here: extraInitializationLevel {
> > ExtraInitializationLevel::None };
> > 
> > But I think this is fine, too.
> > 
> > > Source/JavaScriptCore/runtime/CodeCache.cpp:141
> > > -        JSToken token;
> > > +        JSToken token = { };
> > 
> > I think you should fix this one in ParserTokens.h:
> > 
> > struct JSToken {
> >     JSToken() = default;
> >     // ...
> > };
> > 
> > to match the other tokens defined in that file. That would be much more
> > robust.
> Agreed. That was also discussed in bug #186798. So, what I could do to
> prevent duplicated discussion and work is to split off all the old changes
> and submit a new patch that contains only newer fixes. Does that make sense?
> 
> 
> > > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:154
> > > +        ResultList result { };
> > 
> > But ResultList is:
> > 
> > Vector<Value*, 1>
> > 
> > so this one shouldn't need the brace initialization... correct?
> Looks like. That one is also carried over from bug #186798, maybe it was
> simply "one too much" (three had to be initialized, one not).
> 
> > > Source/WebCore/Modules/webaudio/DelayDSPKernel.h:53
> > >      int m_writeIndex;
> > 
> > Both constructors initialize this to 0, so might as well modernize them by
> > initializing it here instead of in the constructors' initializer lists.
> > 
> > > Source/WebCore/Modules/webaudio/DelayDSPKernel.h:56
> > >      bool m_firstTime;
> > 
> > Ditto.
> Ditto, both from the old patch.
> 
> > > Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:59
> > > +    : m_hasForwardButtonStartPart(false)
> > > +    , m_hasForwardButtonEndPart(false)
> > > +    , m_hasBackButtonStartPart(false)
> > > +    , m_hasBackButtonEndPart(false)
> > 
> > Hm, is it possible to initialize these in the class declaration instead? Or
> > is that not possible, because these are bitfields? If so, this is fine.
> Bitfield initializers are available only since C++20
> 
> 
> (In reply to Alexey Proskuryakov from comment #11)
> > > Although I agree with Darin, I'm also generally of the opinion that we should change WebKit to please as many static analysis tools as we can. Even when the tool reports false positives, as long as it's not too many, then avoiding those will make it easier to see when there are real problems.
> > 
> > It may be obvious to everyone looking at this bug, but adding initialization
> > where it is not needed regresses performance, almost by definition. Other
> > changes needed to please code analysis tools can be introducing other
> > problems, so I think that each one needs to be discussed on its merits.
> If so, then WebKit should be added to the actual Coverity Scan Analysis
> platform, where each "error" can be marked as false positive (which is
> remembered) and other means of telling Coverity already in the source code
> how to interpret things exist. For example, there are dozens of false
> positives about unreachable code or uninitialized where, well, an
> ASSERT_NOTREACHED() or so exists in a switch case. Coverity can be taught
> about such things.

Compiler dude here.  Adding initialization where it is unnecessary is most likely not going to add any overhead.  It won't even add instructions in many cases.

This is because once you get to the compiler's optimizer, it knows very well if an initialization is live at any use. If an initialization was added just to placate a sanitizer or static analysis tool, then it will be very obvious to the compiler that the initialization is dead. The compiler will then remove it.

For example, say you have:

int x = 0;
x = 42;
print(x);

Then the "x = 0" part will disappear in the eventual code. This is true even for complex control flow patterns, including probably many cases where we add " = 0" to spoonfeed tools around a switch statement.

Generally, I only like to spoonfeed tools if those tools are useful.  I think that in the case of adding initializations of fields, it's not just useful to do it but it's also generally perf-neutral and it makes the code more clear.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190412/05d4f85b/attachment-0001.html>