Re: [PATCH 0/6] build: various updates

From: Christopher Li <sparse@chrisli.org>
To: "Uwe Kleine-König" <uwe@kleine-koenig.org>
Cc: Linux-Sparse <linux-sparse@vger.kernel.org>
Subject: Re: [PATCH 0/6] build: various updates
Date: Fri, 6 Oct 2017 14:30:31 -0700	[thread overview]
Message-ID: <CANeU7Q=v5rXzxdtm7_ppW7q=G6qRiymdTQKRzQimXijCsgpR9Q@mail.gmail.com> (raw)
In-Reply-To: <e3d57acc-e699-f51d-6687-e1535db4cd46@kleine-koenig.org>

On Fri, Oct 6, 2017 at 1:10 PM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> Note I don't feel strong for most of my changes. There is little user
> visible change intended by them, just making the build system more
> standard by using expected variable names, which might also be subjective?!

Exactly. I also CC sparse mailing list.

>
>> Can you give the debug-target-v3 a review?
>
> I don't like
>         Makefile: Adding OPT variable for optmize level

Thanks for the review.

>
> I'd expect a Makefile to do something like that:
>
>         CFLAGS ?= -O2 ...
>
> such that I can overrule it by doing
>
>         make CFLAGS="-O0 -g"

I can't see how that will work. CFLAGS has a lot of other options in it.
" -finline-functions -fno-strict-aliasing" just to name a few.
Overwrite the CFLAG
will drop those options as well, which is not desirable.

That is why the OPT variable was needed. Overwrite OPT will just change
the "-O2" an nothing else. Currently over write CFLAGS does not have the same
effect.

In general, overwrite CFLAGS in command line is a bad idea. Most of
the open source project I saw will not able to handle that properly.

>
> Commit
>
>         Makefile: introduce minimal target
>
> has a typo, s/minial/minimal/. And I wonder why it removes debug from

Thanks for catching that. I will update the V4 soon.

> the dependencies of check. This was added in "Makefile: release and
> debug target" needlessly?

Because this change make "all" depend on "debug" as well.
And "check" depend on "all". That makes "check" implicit depend on
"debug" already.

Before this change, "all" target actually did not build all possible target.
That is why I want to change "all" explicit build "release" and "debug" at the
same time.

>
> Other than that I don't see the motivation for this series and I'd
> always compile everything and wonder if
>
>         make CFLAGS="-DSPARSE_DEBUG"
>
> would be good enough.

The motivation is speed. For most user that use sparse to run
the kernel checking. We want sparse run as fast as possible.
However, we also want a debug mode of sparse can run more
validation and error diagnostic at the cost of speed.

One patch series I want to use this debug mode is the ptrlist
reference count patch. The ptrlist reference count has running
over head. However, it can catch the "nest loop modify" bug.
If that code path was triggered, it means a possible list
iteration corruption.

I kind of want sparse can have option  that run the debug
version at run time request. In other words, distributios
will ship both the release and debug version of sparse.
The release version of sparse will invoke the debug
options if there is debug option is specified in command line.

Chris