[GIT PULL] debug build of sparse v4

[GIT PULL] debug build of sparse v4

[Christopher Li]

Hi,

This is the V4 version of the debug build change.

https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/log/?h=debug-target-v4

This change allow sparse to build debug version of sparse
along side of the release version. The debug build of sparse
can be used by patches like ptrlist ref count to check the nest
loop modify usage. The debug build can add other verification feature
that might slow down sparse.

I plan to merge that to master. If there is other objections, it is
a good time to raise it now.

Chris

Re: [GIT PULL] debug build of sparse v4

[Luc Van Oostenryck]

On Wed, Oct 18, 2017 at 11:26 PM, Christopher Li <sparse@chrisli.org> wrote:
> Hi,
>
> This is the V4 version of the debug build change.
>
> https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/log/?h=debug-target-v4
>
> This change allow sparse to build debug version of sparse
> along side of the release version. The debug build of sparse
> can be used by patches like ptrlist ref count to check the nest
> loop modify usage. The debug build can add other verification feature
> that might slow down sparse.
>
> I plan to merge that to master. If there is other objections, it is
> a good time to raise it now.

No real objections but I have some doubts about all the debug part.
I think that people hacking on sparse and who need to debug know
what they need and want to debug.
For example, the OPT=0 is, IMO, useless as you generally need
others flags too. Also, when you debug, you generally need to rerun
things several times, so using extra options on the command line is
not ideal (a mechanism like local.mk is better suited but local.mk
itself is not).

Also, what's the real need for dbgbuild/ & debug/ ?
IMO, it's a big complexification of all the rules with plenty of
duplicated things for very few, if any, benefits.

-- Luc

Re: [GIT PULL] debug build of sparse v4

[Christopher Li]

On Wed, Oct 18, 2017 at 3:18 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> No real objections but I have some doubts about all the debug part.
> I think that people hacking on sparse and who need to debug know
> what they need and want to debug.

Right, this change is to allow a separate debug binary let say "dbg-sparse",
to be execute from "sparse" when needed.

Take the ptrlist ref count checking for example. I image have
sparse --check-ptrlist, which will invoke dbg-sparse to do the
checking. I expect the distro will include dbg-sparse as part
of the sparse package as well.

The end result is that. The user of sparse, not sparse developers,
can invoke sparse with those extra verification (at cost of slow
down sparse) on the custom input source without modify and
recompile sparse.

> For example, the OPT=0 is, IMO, useless as you generally need
> others flags too. Also, when you debug, you generally need to rerun
> things several times, so using extra options on the command line is
> not ideal (a mechanism like local.mk is better suited but local.mk
> itself is not).

In that case, you can just OPT=0 into local.mk and problem solved?
Even if you do that, you still need to provide OPT variable for overwrite
purpose. Append and reset the whole variable is easy.
Partial modify the content of variable from one to another is harder and
annoying.

Can you clarify "but local.mk itself is not".

> Also, what's the real need for dbgbuild/ & debug/ ?
> IMO, it's a big complexification of all the rules with plenty of
> duplicated things for very few, if any, benefits.

Provide extra verification on user input files. Another candidate
of those verification can be, the sorting of ptrlist.
There is function  verify_seq_sorted () verify the list is indeed sorted.
But that function currently is not turn on in sparse. Presumably
it is used in the development phase of the ptr list sorting.
That verification function can be turn on in the debug build for example.

Chris

Re: [GIT PULL] debug build of sparse v4

[Luc Van Oostenryck]

On Thu, Oct 19, 2017 at 6:39 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Wed, Oct 18, 2017 at 3:18 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote:
>> No real objections but I have some doubts about all the debug part.
>> I think that people hacking on sparse and who need to debug know
>> what they need and want to debug.
>
> Right, this change is to allow a separate debug binary let say "dbg-sparse",
> to be execute from "sparse" when needed.

My point is: if "when needed" means "when sparse developpers
need to debug something" then I really think all this is not needed
as the dev can very well build the executable he needs, with the
options he needs, use his own workflow, ...

> Take the ptrlist ref count checking for example. I image have
> sparse --check-ptrlist, which will invoke dbg-sparse to do the
> checking. I expect the distro will include dbg-sparse as part
> of the sparse package as well.

Really? Are the distros interested in this sort of things? Is there
a real need for it? Who do you really think will use this?

> The end result is that. The user of sparse, not sparse developers,
> can invoke sparse with those extra verification (at cost of slow
> down sparse) on the custom input source without modify and
> recompile sparse.

Yes *can* ... but will they?
Why isn't there a dbg-gcc, dbg-bash, ... ?

>> For example, the OPT=0 is, IMO, useless as you generally need
>> others flags too. Also, when you debug, you generally need to rerun
>> things several times, so using extra options on the command line is
>> not ideal (a mechanism like local.mk is better suited but local.mk
>> itself is not).
>
> In that case, you can just OPT=0 into local.mk and problem solved?
> Even if you do that, you still need to provide OPT variable for overwrite
> purpose. Append and reset the whole variable is easy.
> Partial modify the content of variable from one to another is harder and
> annoying.
>
> Can you clarify "but local.mk itself is not".

If you have a sort of config file, or anything allowing to adapt things
to your needs, you want at least to have a dot file for it.
But for a project using git, you also want that the file doesn't interfer
with git status but also with git clean (including git clean -x).

>> Also, what's the real need for dbgbuild/ & debug/ ?
>> IMO, it's a big complexification of all the rules with plenty of
>> duplicated things for very few, if any, benefits.
>
> Provide extra verification on user input files. Another candidate
> of those verification can be, the sorting of ptrlist.

> There is function  verify_seq_sorted () verify the list is indeed sorted.
> But that function currently is not turn on in sparse. Presumably
> it is used in the development phase of the ptr list sorting.
> That verification function can be turn on in the debug build for example.

Yes *can* ...
Same questions: who will use this and when? Are extra binaries and
duplicated build rules a good answer for this?

More broadly: what problem do you try to solve with these patches?
What values do these patches bring to sparse?
Does sparse need these patches?

-- Luc

Re: [GIT PULL] debug build of sparse v4

[Christopher Li]

On Fri, Oct 20, 2017 at 3:39 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> My point is: if "when needed" means "when sparse developpers
> need to debug something" then I really think all this is not needed
> as the dev can very well build the executable he needs, with the
> options he needs, use his own workflow, ...
>
>> Take the ptrlist ref count checking for example. I image have
>> sparse --check-ptrlist, which will invoke dbg-sparse to do the
>> checking. I expect the distro will include dbg-sparse as part
>> of the sparse package as well.
>
> Really? Are the distros interested in this sort of things? Is there
> a real need for it? Who do you really think will use this?

To get back on this. I think we user report sparse bugs.
We can ask them to run debug version of sparse to report
or catch more bugs.

For example. The default version of sparse does not have
the ptrlist nest modify detect feature. The debug version of
sparse has. It is actually hard to ask user to check out a git
version of sparse, build it with debug options, then run the
test again.

With debug version of sparse shipped as part of sparse, this
can be simplified.

>
>> The end result is that. The user of sparse, not sparse developers,
>> can invoke sparse with those extra verification (at cost of slow
>> down sparse) on the custom input source without modify and
>> recompile sparse.
>
> Yes *can* ... but will they?
> Why isn't there a dbg-gcc, dbg-bash, ... ?

If we install debug version and invoke them properly, I don't see
why distro will not take it.

Debug version of clang exist. It just not everybody use it.
I don't see a need to for debug version of bash. Because sparse
doing optimization and transformation etc. It is very delicate.
As long as the debug version does provide some value, I think
it is fine to include them as part of the binary.

>
> If you have a sort of config file, or anything allowing to adapt things
> to your needs, you want at least to have a dot file for it.
> But for a project using git, you also want that the file doesn't interfer
> with git status but also with git clean (including git clean -x).

I think that is why local.mk was in .gitignore. For my usage
case, there is clearly need that I want some per host
config to plug into the Makefile.


> Yes *can* ...
> Same questions: who will use this and when? Are extra binaries and
> duplicated build rules a good answer for this?

Again, it is for the user who report sparse bugs.

> More broadly: what problem do you try to solve with these patches?

I want to include the ptrlist nest  modify detect patch into sparse.

> What values do these patches bring to sparse?

See above.

> Does sparse need these patches?

I think so.


Chris

Re: [GIT PULL] debug build of sparse v4

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 966 bytes --]

Hello,

On 10/18/2017 11:26 PM, Christopher Li wrote:
> This change allow sparse to build debug version of sparse
> along side of the release version. The debug build of sparse
> can be used by patches like ptrlist ref count to check the nest
> loop modify usage. The debug build can add other verification feature
> that might slow down sparse.

I wonder about the name. From a debug build (in contrast to a release
build) I would expect that it allows to debug sparse itself (Think: add
-g to gcc, or (not) -DNODEBUG for assert(3)), but I understand you use
that term differently here.

Why not do the the extra tests when called as (say)

	sparse --aggressive

. Then there is no need for an extra binary at all keeping the build
system simple and that also makes it it easier to understand for users
(but I might judge others by my own standards here?) For gcc this flag
is -Wall, there isn't an extra binary either.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [GIT PULL] debug build of sparse v4

[Christopher Li]

On Thu, Oct 19, 2017 at 5:42 AM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> I wonder about the name. From a debug build (in contrast to a release
> build) I would expect that it allows to debug sparse itself (Think: add
> -g to gcc, or (not) -DNODEBUG for assert(3)), but I understand you use
> that term differently here.

The name if mechanical. You can suggest better names. I am
more focus on allowing two version of sparse can be compile
at the same time.

>
> Why not do the the extra tests when called as (say)
>
>         sparse --aggressive
>

Yes, actually that is what I have in mind. When "--aggressive"
is turn on, sparse will execute "dbg-sparse" to enable those aggressive
code path. The optional name is subject to change.

> . Then there is no need for an extra binary at all keeping the build
> system simple and that also makes it it easier to understand for users
> (but I might judge others by my own standards here?) For gcc this flag
> is -Wall, there isn't an extra binary either.

There is still need for extra binary because some verification can be
slow sparse and hard to turn off without impact the sparse performance.
For example the ptr list ref count patch. It is execute at every ptrlist
bucket iteration.

If there is no performance impact on when those verification
is turn off. Then yes, there is no need for a separate binary.
I haven't done concrete measurement of the slow down.
I just assume that there will be some.

Chris

Re: [GIT PULL] debug build of sparse v4

[Luc Van Oostenryck]

On Thu, Oct 19, 2017 at 6:50 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Thu, Oct 19, 2017 at 5:42 AM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> Yes, actually that is what I have in mind. When "--aggressive"
> is turn on, sparse will execute "dbg-sparse" to enable those aggressive
> code path. The optional name is subject to change.
>
>> . Then there is no need for an extra binary at all keeping the build
>> system simple and that also makes it it easier to understand for users
>> (but I might judge others by my own standards here?) For gcc this flag
>> is -Wall, there isn't an extra binary either.
>
> There is still need for extra binary because some verification can be
> slow sparse and hard to turn off without impact the sparse performance.
> For example the ptr list ref count patch. It is execute at every ptrlist
> bucket iteration.
>
> If there is no performance impact on when those verification
> is turn off. Then yes, there is no need for a separate binary.

The questions that really matter are:
- who will use these extra binaries?
- in which circonstances?

-- Luc

Re: [GIT PULL] debug build of sparse v4

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2084 bytes --]

On 10/19/2017 06:50 PM, Christopher Li wrote:
> On Thu, Oct 19, 2017 at 5:42 AM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>>
>> I wonder about the name. From a debug build (in contrast to a release
>> build) I would expect that it allows to debug sparse itself (Think: add
>> -g to gcc, or (not) -DNODEBUG for assert(3)), but I understand you use
>> that term differently here.
> 
> The name if mechanical. You can suggest better names. I am
> more focus on allowing two version of sparse can be compile
> at the same time.
> 
>>
>> Why not do the the extra tests when called as (say)
>>
>>         sparse --aggressive
>>
> 
> Yes, actually that is what I have in mind. When "--aggressive"
> is turn on, sparse will execute "dbg-sparse" to enable those aggressive
> code path. The optional name is subject to change.
> 
>> . Then there is no need for an extra binary at all keeping the build
>> system simple and that also makes it it easier to understand for users
>> (but I might judge others by my own standards here?) For gcc this flag
>> is -Wall, there isn't an extra binary either.
> 
> There is still need for extra binary because some verification can be
> slow sparse and hard to turn off without impact the sparse performance.
> For example the ptr list ref count patch. It is execute at every ptrlist
> bucket iteration.
> 
> If there is no performance impact on when those verification
> is turn off. Then yes, there is no need for a separate binary.
> I haven't done concrete measurement of the slow down.
> I just assume that there will be some.

I would first implement doing everything in a single binary and only
think about the complicated split in a "normal" and an "aggressive"
sparse binary if the overhead is too high and people start wailing.

(And even then I'd first check if using the equivalent of unlikely() in
the kernel would help by assuming that passing --aggressive is "unlikely".)

IMHO the cost of a complicated build system is to high to not first try
the alternatives.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [GIT PULL] debug build of sparse v4

[Christopher Li]

On Sun, Oct 22, 2017 at 9:09 PM, Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> I would first implement doing everything in a single binary and only
> think about the complicated split in a "normal" and an "aggressive"
> sparse binary if the overhead is too high and people start wailing.

I think some thing like detect the ptrlist nest modify is on a hot path
does not belong to release version of sparse. I usually will not wait
until it has some performance problem.

Chris