On Tuesday, May 12, 2020 11:58:49 PM CEST Paul Eggert wrote:
> On 5/12/20 10:49 AM, Kamil Dudka wrote:
> > The problem is that such
> > false positives may easily turn out into true positives, as the code gets
> > changed, and nobody will notice it.
>
> Sounds extremely unlikely here. It's never happened with coreutils as far as
> I know. For this particular case, this sounds more like a theoretical
> argument than a practical one.
I am not sure about coreutils or gnulib but, in general, such things happen
in practice. For example, acl upstream pushed the following commit "just to
silence valgrind":
http://git.savannah.gnu.org/cgit/acl.git/commit?id=33f01b5d
They were lucky that they did not have IF_LINT because they in fact fixed a
real-world bug without actually knowing about it. Two months later Red Hat
released an accelerated fix for a customer-facing issue with backport of the
upstream commit that had itself been documented as a no-op.
> >> I doubt whether it's a good idea to apply downstream patches that mean
> >> you
> >> are running differently from everybody else. This is more maintenance
> >> work
> >> for you,
> >
> > It actually saves me work because it eases review of static analysis
> > results.
> Why isn't it less work to build and analyze with '-Dlint'?
Using it for static analyzers only is not an option. We need to analyze the
code that is going to run. Building RPMs with -Dlint would mean performance
penalty because some programs (e.g. sort) would explicitly release complex
data structures just before exit, which is wasteful.
> Then you don't
> have to change the source code, so you won't have to adapt your patch to
> future releases.
Yes. If there was a define that does the same thing as my patch does, it
would certainly ease the downstream maintenance. However, as -Dlint is
implemented now, it has unwanted side-effects that my patch does not have.
> Is -Dlint avoided because it would break other tools?
Not that I know of.
> If so, how about if we
> change coreutils's '#ifdef lint' to '#ifdef GCC_LINT'? Then you could build
> and analyze with -DGCC_LINT. Emacs already does this, so there's good
> precedent.
Name of the define does not matter as long as there is no conflict. If there
is a way to explicitly initialize scalar variables (in all the cases where
analyzers think it is read before initialization), I will be happy to use it.
> > We can only have subset of the bugs that everyone else has. I cannot
> > image
> > a situation where initialization of otherwise uninitialized scalar
> > variable
> > could introduce a new bug.
>
> I can. Perhaps the uninitialized variable has garbage in it that is
> typically nonzero, and a later bug in the program is triggered only by a
> rare combination of factors, one of which is that the variable must be
> zero.
>
> Of course there's no such bug in coreutils - but there's no bug of the
> flavor that you're imagining either. The point is that there's no proof
> that your bugs are a subset of everyone else's in this area.
This part I do not understand. To put it simple, there are two cases:
(1) The variable is always initialized before it is read. In this case the
explicit initialization is harmless. In some cases, optimizer drops the
redundant assignment anyway. If not, it is kept without having any impact
on the observable behavior of the program.
(2) The variable is somewhere read before it is initialized. In this case
behavior of the original program is undefined. The explicit initialization
makes the behavior at least defined (even though not necessarily matching
author's expectations).
Relying on undefined behavior is not an option.
Kamil
