[PATCH] mailinfo.c: move side-effects outside of assert

[PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:

	assert(call_a_function(...))

The function in question, check_header, has side effects.  This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.

Move the function call outside of the assert and assert on
the result of the function call instead so that the code
still works properly in a release build and passes the tests.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---

Notes:
    Please include this PATCH in 2.11.x maint

 mailinfo.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mailinfo.c b/mailinfo.c
index 2fb3877e..47442fb5 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -708,9 +708,12 @@ static int is_scissors_line(const char *line)
 
 static void flush_inbody_header_accum(struct mailinfo *mi)
 {
+	int okay;
+
 	if (!mi->inbody_header_accum.len)
 		return;
-	assert(check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0));
+	okay = check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0);
+	assert(okay);
 	strbuf_reset(&mi->inbody_header_accum);
 }
 
---

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Johannes Schindelin]

Hi,

On Sat, 17 Dec 2016, Kyle J. McKay wrote:

> Since 6b4b013f18 (mailinfo: handle in-body header continuations,
> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an
> assert of the form:
> 
> 	assert(call_a_function(...))
> 
> The function in question, check_header, has side effects.  This
> means that when NDEBUG is defined during a release build the
> function call is omitted entirely, the side effects do not
> take place and tests (fortunately) start failing.
> 
> Move the function call outside of the assert and assert on
> the result of the function call instead so that the code
> still works properly in a release build and passes the tests.
> 
> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>

ACK. I noticed this problem (and fixed it independently as a part of a
huge patch series I did not get around to submit yet) while trying to get
Git to build correctly with Visual C.

Ciao,
Dscho

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Jeff King]

On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:

> Since 6b4b013f18 (mailinfo: handle in-body header continuations,
> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an
> assert of the form:
> 
> 	assert(call_a_function(...))
> 
> The function in question, check_header, has side effects.  This
> means that when NDEBUG is defined during a release build the
> function call is omitted entirely, the side effects do not
> take place and tests (fortunately) start failing.
> 
> Move the function call outside of the assert and assert on
> the result of the function call instead so that the code
> still works properly in a release build and passes the tests.
> 
> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
> ---
> 
> Notes:
>     Please include this PATCH in 2.11.x maint

This is obviously an improvement, but it makes me wonder if we should be
doing:

  if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
	die("BUG: some explanation of why this can never happen");

which perhaps documents the intended assumptions more clearly. A comment
regarding the side effects might also be helpful.

-Peff

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 19, 2016, at 12:03, Jeff King wrote:

> On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:
>
>> Since 6b4b013f18 (mailinfo: handle in-body header continuations,
>> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an
>> assert of the form:
>>
>> 	assert(call_a_function(...))
>>
>> The function in question, check_header, has side effects.  This
>> means that when NDEBUG is defined during a release build the
>> function call is omitted entirely, the side effects do not
>> take place and tests (fortunately) start failing.
>>
>> Move the function call outside of the assert and assert on
>> the result of the function call instead so that the code
>> still works properly in a release build and passes the tests.
>>
>> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
>> ---
>>
>> Notes:
>>    Please include this PATCH in 2.11.x maint
>
> This is obviously an improvement, but it makes me wonder if we  
> should be
> doing:
>
>  if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
> 	die("BUG: some explanation of why this can never happen");
>
> which perhaps documents the intended assumptions more clearly. A  
> comment
> regarding the side effects might also be helpful.

I wondered exactly the same thing myself.  I was hoping Jonathan would  
pipe in here with some analysis about whether this is:

   a) a super paranoid, just-in-case, can't really ever fail because  
by the time we get to this code we've already effectively validated  
everything that could cause check_header to return false in this case

-or-

   b) Yeah, it could fail in the real world and it should "die" (and  
probably have a test added that triggers such death)

-or-

   c) Actually, if check_header does return false we can keep going  
without problem

-or-

   d) Actually, if check_header does return false we can keep going by  
making a minor change that should be in the patch

I assume that since Jonathan added the code he will just know the  
answer as to which one it is and I won't have to rely on the results  
of my imaginary analysis.  ;)

On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:

> ACK. I noticed this problem (and fixed it independently as a part of a
> huge patch series I did not get around to submit yet) while trying  
> to get
> Git to build correctly with Visual C.

Does this mean that Dscho and I are the only ones who add -DNDEBUG for  
release builds?  Or are we just the only ones who actually run the  
test suite on such builds?

--Kyle

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Jonathan Tan]

On 12/19/2016 12:38 PM, Kyle J. McKay wrote:
> On Dec 19, 2016, at 12:03, Jeff King wrote:
>
>> On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:
>>
>>> Since 6b4b013f18 (mailinfo: handle in-body header continuations,
>>> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an
>>> assert of the form:
>>>
>>>     assert(call_a_function(...))

Thanks for spotting this - I'm not sure how I missed that.

>> This is obviously an improvement, but it makes me wonder if we should be
>> doing:
>>
>>  if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
>>     die("BUG: some explanation of why this can never happen");
>>
>> which perhaps documents the intended assumptions more clearly. A comment
>> regarding the side effects might also be helpful.
>
> I wondered exactly the same thing myself.  I was hoping Jonathan would
> pipe in here with some analysis about whether this is:
>
>   a) a super paranoid, just-in-case, can't really ever fail because by
> the time we get to this code we've already effectively validated
> everything that could cause check_header to return false in this case
>
> -or-
>
>   b) Yeah, it could fail in the real world and it should "die" (and
> probably have a test added that triggers such death)
>
> -or-
>
>   c) Actually, if check_header does return false we can keep going
> without problem
>
> -or-
>
>   d) Actually, if check_header does return false we can keep going by
> making a minor change that should be in the patch
>
> I assume that since Jonathan added the code he will just know the answer
> as to which one it is and I won't have to rely on the results of my
> imaginary analysis.  ;)

The answer is "a". The only time that mi->inbody_header_accum is 
appended to is in check_inbody_header, and appending onto a blank 
mi->inbody_header_accum always happens when is_inbody_header is true 
(which guarantees a prefix that causes check_header to always return true).

Peff's suggestion sounds reasonable to me, maybe with an error message 
like "BUG: inbody_header_accum, if not empty, must always contain a 
valid in-body header".

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>>> This is obviously an improvement, but it makes me wonder if we should be
>>> doing:
>>>
>>>  if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
>>>     die("BUG: some explanation of why this can never happen");
>>>
>>> which perhaps documents the intended assumptions more clearly. A comment
>>> regarding the side effects might also be helpful.
>>
>> I wondered exactly the same thing myself.  I was hoping Jonathan would
>> pipe in here with some analysis about whether this is:
>>
>>   a) a super paranoid, just-in-case, can't really ever fail because by
>> the time we get to this code we've already effectively validated
>> everything that could cause check_header to return false in this case
>> ...
> The answer is "a". The only time that mi->inbody_header_accum is
> appended to is in check_inbody_header, and appending onto a blank
> mi->inbody_header_accum always happens when is_inbody_header is true
> (which guarantees a prefix that causes check_header to always return
> true).
>
> Peff's suggestion sounds reasonable to me, maybe with an error message
> like "BUG: inbody_header_accum, if not empty, must always contain a
> valid in-body header".

OK.  So we do not expect it to fail, but we still do want the side
effect of that function (i.e. accmulation into the field).

Somebody care to send a final "agreed-upon" version?

[PATCH v2] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 19, 2016, at 13:01, Junio C Hamano wrote:

> Jonathan Tan <jonathantanmy@google.com> writes:
>
>>>> This is obviously an improvement, but it makes me wonder if we  
>>>> should be
>>>> doing:
>>>>
>>>> if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))
>>>>    die("BUG: some explanation of why this can never happen");
>>>>
>>>> which perhaps documents the intended assumptions more clearly. A  
>>>> comment
>>>> regarding the side effects might also be helpful.
>>>
>>> I wondered exactly the same thing myself.  I was hoping Jonathan  
>>> would
>>> pipe in here with some analysis about whether this is:
>>>
>>>  a) a super paranoid, just-in-case, can't really ever fail because  
>>> by
>>> the time we get to this code we've already effectively validated
>>> everything that could cause check_header to return false in this  
>>> case
>>> ...
>> The answer is "a". The only time that mi->inbody_header_accum is
>> appended to is in check_inbody_header, and appending onto a blank
>> mi->inbody_header_accum always happens when is_inbody_header is true
>> (which guarantees a prefix that causes check_header to always return
>> true).
>>
>> Peff's suggestion sounds reasonable to me, maybe with an error  
>> message
>> like "BUG: inbody_header_accum, if not empty, must always contain a
>> valid in-body header".
>
> OK.  So we do not expect it to fail, but we still do want the side
> effect of that function (i.e. accmulation into the field).
>
> Somebody care to send a final "agreed-upon" version?

Yup, here it is:

-- 8< --

Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:

	assert(call_a_function(...))

The function in question, check_header, has side effects.  This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.

Move the function call outside of the assert and assert on
the result of the function call instead so that the code
still works properly in a release build and passes the tests.

Since the only time that mi->inbody_header_accum is appended to is
in check_inbody_header, and appending onto a blank
mi->inbody_header_accum always happens when is_inbody_header is
true, this guarantees a prefix that causes check_header to always
return true.

Therefore replace the assert with an if !check_header + DIE
combination to reflect this.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Jeff King <peff@peff.net>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---

Notes:
    Please include this PATCH in 2.11.x maint

 mailinfo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mailinfo.c b/mailinfo.c
index 2fb3877e..a489d9d0 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)
 {
 	if (!mi->inbody_header_accum.len)
 		return;
-	assert(check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0));
+	if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0))
+		die("BUG: inbody_header_accum, if not empty, must always contain a valid in-body header");
 	strbuf_reset(&mi->inbody_header_accum);
 }
 
---

Re: [PATCH v2] mailinfo.c: move side-effects outside of assert

[Junio C Hamano]

"Kyle J. McKay" <mackyle@gmail.com> writes:

>> OK.  So we do not expect it to fail, but we still do want the side
>> effect of that function (i.e. accmulation into the field).
>>
>> Somebody care to send a final "agreed-upon" version?
>
> Yup, here it is:

Thanks.

> -- 8< --
>
> Since 6b4b013f18 (mailinfo: handle in-body header continuations,
> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an
> assert of the form:
>
> 	assert(call_a_function(...))
>
> The function in question, check_header, has side effects.  This
> means that when NDEBUG is defined during a release build the
> function call is omitted entirely, the side effects do not
> take place and tests (fortunately) start failing.
>
> Move the function call outside of the assert and assert on
> the result of the function call instead so that the code
> still works properly in a release build and passes the tests.
>
> Since the only time that mi->inbody_header_accum is appended to is
> in check_inbody_header, and appending onto a blank
> mi->inbody_header_accum always happens when is_inbody_header is
> true, this guarantees a prefix that causes check_header to always
> return true.
>
> Therefore replace the assert with an if !check_header + DIE
> combination to reflect this.
>
> Helped-by: Jonathan Tan <jonathantanmy@google.com>
> Helped-by: Jeff King <peff@peff.net>
> Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
> ---
>
> Notes:
>     Please include this PATCH in 2.11.x maint
>
>  mailinfo.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mailinfo.c b/mailinfo.c
> index 2fb3877e..a489d9d0 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)
>  {
>  	if (!mi->inbody_header_accum.len)
>  		return;
> -	assert(check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0));
> +	if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0))
> +		die("BUG: inbody_header_accum, if not empty, must always contain a valid in-body header");
>  	strbuf_reset(&mi->inbody_header_accum);
>  }
>  
> ---

[PATCH v3] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 19, 2016, at 15:26, Junio C Hamano wrote:

> "Kyle J. McKay" <mackyle@gmail.com> writes:
>
>>> OK.  So we do not expect it to fail, but we still do want the side
>>> effect of that function (i.e. accmulation into the field).
>>>
>>> Somebody care to send a final "agreed-upon" version?
>>
>> Yup, here it is:
>
> Thanks.

Whoops. there's an extra paragraph in the commit description that I
meant to remove and, of course, I didn't notice it until I sent the
copy to the list.  :(

I don't think a "fixup" or "squash" can replace a description, right?

So here's a replacement patch with the correct description with the
deleted paragrah:

-- >8 --

Since 6b4b013f18 (mailinfo: handle in-body header continuations,
2016-09-20, v2.11.0) mailinfo.c has contained new code with an
assert of the form:

	assert(call_a_function(...))

The function in question, check_header, has side effects.  This
means that when NDEBUG is defined during a release build the
function call is omitted entirely, the side effects do not
take place and tests (fortunately) start failing.

Since the only time that mi->inbody_header_accum is appended to is
in check_inbody_header, and appending onto a blank
mi->inbody_header_accum always happens when is_inbody_header is
true, this guarantees a prefix that causes check_header to always
return true.

Therefore replace the assert with an if !check_header + DIE
combination to reflect this.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Jeff King <peff@peff.net>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---

Notes:
    Please include this PATCH in 2.11.x maint

 mailinfo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mailinfo.c b/mailinfo.c
index 2fb3877e..a489d9d0 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)
 {
 	if (!mi->inbody_header_accum.len)
 		return;
-	assert(check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0));
+	if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0))
+		die("BUG: inbody_header_accum, if not empty, must always contain a valid in-body header");
 	strbuf_reset(&mi->inbody_header_accum);
 }
 
---

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Johannes Schindelin]

Hi Kyle,

On Mon, 19 Dec 2016, Kyle J. McKay wrote:

> On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:
> 
> >ACK. I noticed this problem (and fixed it independently as a part of a
> >huge patch series I did not get around to submit yet) while trying to
> >get Git to build correctly with Visual C.
> 
> Does this mean that Dscho and I are the only ones who add -DNDEBUG for
> release builds?  Or are we just the only ones who actually run the test
> suite on such builds?

It seems you and I are for the moment the only ones bothering with running
the test suite on release builds.

Ciao,
Johannes

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Jeff King]

On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote:

> > On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:
> > 
> > >ACK. I noticed this problem (and fixed it independently as a part of a
> > >huge patch series I did not get around to submit yet) while trying to
> > >get Git to build correctly with Visual C.
> > 
> > Does this mean that Dscho and I are the only ones who add -DNDEBUG for
> > release builds?  Or are we just the only ones who actually run the test
> > suite on such builds?
> 
> It seems you and I are for the moment the only ones bothering with running
> the test suite on release builds.

I wasn't aware anybody actually built with NDEBUG at all. You'd have to
explicitly ask for it via CFLAGS, so I assume most people don't.
Certainly I never have when deploying to GitHub's cluster (let alone my
personal use), and I note that the Debian package also does not.

So from my perspective it is not so much "do not bother with release
builds" as "are release builds even a thing for git?".  One of the
reasons I suggested switching the assert() to a die("BUG") is that the
latter cannot be disabled. We generally seem to prefer those to assert()
in our code-base (though there is certainly a mix). If the assertions
are not expensive to compute, I think it is better to keep them in for
all builds. I'd much rather get a report from a user that says "I hit
this BUG" than "git segfaulted and I have no idea where" (of course I
prefer a backtrace even more, but that's not always an option).

I do notice that we set NDEBUG for nedmalloc, though if I am reading the
Makefile right, it is just for compiling those files. It looks like
there are a ton of asserts there that _are_ potentially expensive, so
that makes sense.

-Peff

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 20, 2016, at 08:45, Jeff King wrote:

> On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote:
>
>>> On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:
>>>
>>>> ACK. I noticed this problem (and fixed it independently as a part  
>>>> of a
>>>> huge patch series I did not get around to submit yet) while  
>>>> trying to
>>>> get Git to build correctly with Visual C.
>>>
>>> Does this mean that Dscho and I are the only ones who add -DNDEBUG  
>>> for
>>> release builds?  Or are we just the only ones who actually run the  
>>> test
>>> suite on such builds?
>>
>> It seems you and I are for the moment the only ones bothering with  
>> running
>> the test suite on release builds.
>
> I wasn't aware anybody actually built with NDEBUG at all. You'd have  
> to
> explicitly ask for it via CFLAGS, so I assume most people don't.

Not a good assumption.  You know what happens when you assume[1],  
right? ;)

I've been defining NDEBUG whenever I make a release build for quite  
some time (not just for Git) in order to squeeze every last possible  
drop of performance out of it.

> Certainly I never have when deploying to GitHub's cluster (let alone  
> my
> personal use), and I note that the Debian package also does not.

Yeah, I don't do it for my personal use because those are often not  
based on a release tag so I want to see any assertion failures that  
might happen and they're also not performance critical either.

> So from my perspective it is not so much "do not bother with release
> builds" as "are release builds even a thing for git?"

They should be if you're deploying Git in a performance critical  
environment.

> One of the
> reasons I suggested switching the assert() to a die("BUG") is that the
> latter cannot be disabled. We generally seem to prefer those to  
> assert()
> in our code-base (though there is certainly a mix). If the assertions
> are not expensive to compute, I think it is better to keep them in for
> all builds. I'd much rather get a report from a user that says "I hit
> this BUG" than "git segfaulted and I have no idea where" (of course I
> prefer a backtrace even more, but that's not always an option).

Perhaps Git should provide a "verify" macro.  Works like "assert"  
except that it doesn't go away when NDEBUG is defined.  Being Git- 
provided it could also use Git's die function.  Then Git could do a  
global replace of assert with verify and institute a no-assert policy.

> I do notice that we set NDEBUG for nedmalloc, though if I am reading  
> the
> Makefile right, it is just for compiling those files. It looks like
> there are a ton of asserts there that _are_ potentially expensive, so
> that makes sense.

So there's no way to get a non-release build of nedmalloc inside Git  
then without hacking the Makefile?  What if you need those assertions  
enabled?  Maybe NDEBUG shouldn't be defined by default for any files.

--Kyle

[1] https://www.youtube.com/watch?v=KEP1acj29-Y

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Jeff King]

On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:

> > I wasn't aware anybody actually built with NDEBUG at all. You'd have to
> > explicitly ask for it via CFLAGS, so I assume most people don't.
> 
> Not a good assumption.  You know what happens when you assume[1], right? ;)

Kind of. If it's a configuration that nobody[1] in the Git development
community intended to support or test, then isn't the person triggering
it the one making assumptions?

At any rate, I agree that setting NDEBUG should not create a broken
program, and some solution like your patch is a good idea here. I was
mainly speaking to the "do not bother" comment. It is not that I do not
bother to build with NDEBUG, it is that I think it is actively a bad
idea.

[1] Maybe I am alone in my surprise, and everybody working on Git is
    using assert() with the intention that it can be disabled. But if
    that were the case, I'd expect more push-back against "die(BUG)"
    which does not have this feature. I don't recall a single discussion
    to that effect, and searching for NDEBUG in the list archives turns
    up hardly any mentions.

> I've been defining NDEBUG whenever I make a release build for quite some
> time (not just for Git) in order to squeeze every last possible drop of
> performance out of it.

I think here you are getting into superstition. Is there any single
assert() in Git that will actually have an impact on performance?

I'd be more impressed if you could show some operation that is faster
when built with NDEBUG than without. Running all of t/perf does not seem
to show any difference, and looking at the asserts themselves, they're
almost all single-instruction compares in code that isn't performance
critical anyway.

> > So from my perspective it is not so much "do not bother with release
> > builds" as "are release builds even a thing for git?"
> 
> They should be if you're deploying Git in a performance critical
> environment.

I hope my history of patches shows that I do care about deploying Git in
a performance critical environment. But I only care about performance
tradeoffs that have a _measurable_ gain.

> Perhaps Git should provide a "verify" macro.  Works like "assert" except
> that it doesn't go away when NDEBUG is defined.  Being Git-provided it could
> also use Git's die function.  Then Git could do a global replace of assert
> with verify and institute a no-assert policy.

What would be the advantage of that over `if(...) die("BUG: ...")`? It
does not require you to write a reason in the die(), but I am not sure
that is a good thing.

> > I do notice that we set NDEBUG for nedmalloc, though if I am reading the
> > Makefile right, it is just for compiling those files. It looks like
> > there are a ton of asserts there that _are_ potentially expensive, so
> > that makes sense.
> 
> So there's no way to get a non-release build of nedmalloc inside Git then
> without hacking the Makefile?  What if you need those assertions enabled?
> Maybe NDEBUG shouldn't be defined by default for any files.

AFAICT, yes. I'd leave it to people who actually build with nedmalloc to
decide whether it is worth caring about, and whether the asserts there
have a noticeable performance impact.

-Peff

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 21, 2016, at 07:55, Jeff King wrote:

> On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:
>
>>> I wasn't aware anybody actually built with NDEBUG at all. You'd  
>>> have to
>>> explicitly ask for it via CFLAGS, so I assume most people don't.
>>
>> Not a good assumption.  You know what happens when you assume[1],  
>> right? ;)
>
> Kind of. If it's a configuration that nobody[1] in the Git development
> community intended to support or test, then isn't the person  
> triggering
> it the one making assumptions?

No, I don't think so.  NDEBUG is very clearly specified in POSIX [1].

If NDEBUG is defined then "assert(...)" disappears (and in a nice way  
so as not to precipitate "unused variable" warnings).  "N" being "No"  
or "Not" or "Negated" or "bar over the top" + "DEBUG" meaning Not  
DEBUG.  So the code that goes away when NDEBUG is defined is clearly  
debug code.

Considering the wide deployment and use of Git at this point I think  
rather the opposite to be true that "Git does Not require DEBUGging  
code to be enabled for everyday use."  The alternative that it does  
suggests it's not ready for prime time and quite clearly that's not  
the case.

>> I've been defining NDEBUG whenever I make a release build for quite  
>> some
>> time (not just for Git) in order to squeeze every last possible  
>> drop of
>> performance out of it.
>
> I think here you are getting into superstition. Is there any single
> assert() in Git that will actually have an impact on performance?

You have suggested there is and that Git is enabling NDEBUG for  
exactly that reason -- to increase performance:

>> On Dec 20, 2016, at 08:45, Jeff King wrote:
>>
>>> I do notice that we set NDEBUG for nedmalloc, though if I am  
>>> reading the
>>> Makefile right, it is just for compiling those files. It looks like
>>> there are a ton of asserts there that _are_ potentially expensive


>> Perhaps Git should provide a "verify" macro.  Works like "assert"  
>> except
>> that it doesn't go away when NDEBUG is defined.  Being Git-provided  
>> it could
>> also use Git's die function.  Then Git could do a global replace of  
>> assert
>> with verify and institute a no-assert policy.
>
> What would be the advantage of that over `if(...) die("BUG: ...")`? It
> does not require you to write a reason in the die(), but I am not sure
> that is a good thing.

You have stated that you believe the current "assert" calls in Git  
(excluding nedmalloc) should not magically disappear when NDEBUG is  
defined.  So precluding a more labor intensive approach where all  
currently existing "assert(...)" calls are replaced with an "if (!...)  
die(...)" combination, providing a "verify" macro is a quick way to  
make that happen.  Consider this, was the value that Jonathan provided  
for the "die" string immediately obvious to you?  It sure wasn't to  
me.  That means that whoever does the "assert(...)" -> "if(!...)die"  
swap out may need to be intimately familiar with that particular piece  
of code or the result will be no better than using a "verify" macro.

I'm just trying to find a quick and easy way to accommodate your  
wishes without redefining the semantics of NDEBUG. ;)

--Kyle

[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/assert.h.html

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Jeff King]

On Wed, Dec 21, 2016 at 06:21:37PM -0800, Kyle J. McKay wrote:

> > Kind of. If it's a configuration that nobody[1] in the Git development
> > community intended to support or test, then isn't the person triggering
> > it the one making assumptions?
> 
> No, I don't think so.  NDEBUG is very clearly specified in POSIX [1].

I know what NDEBUG is, and I know it is widely used in other projects.
My claim is only that I do not think that the use of NDEBUG was
generally considered when people wrote assert() in git. That is
certainly true of me. I don't know about other developers.

> > I think here you are getting into superstition. Is there any single
> > assert() in Git that will actually have an impact on performance?
> 
> You have suggested there is and that Git is enabling NDEBUG for exactly that
> reason -- to increase performance:

Sorry if I wasn't clear, but I meant in Git itself, not in the nedmalloc
compat code. I think it's worth considering them separately because the
latter is not part of most Git builds in the first place, and certainly
it is written in a different style, and may have different assumptions
about how people might build it.

I do not know the nedmalloc code well at all. I gave a brief read of the
results of "git grep 'assert('" and it looked like some of its
assertions are more involved than simple comparisons. So I guessed that
perhaps the reason that NDEBUG was added there when the code was
imported is that there is a performance difference there. But that's
just a guess. It may or may not be borne out by measurements.

I'd be surprised if any of the assertions in the rest of git have a
noticeable impact (and I could not detect one by running t/perf).

> > What would be the advantage of that over `if(...) die("BUG: ...")`? It
> > does not require you to write a reason in the die(), but I am not sure
> > that is a good thing.
> 
> You have stated that you believe the current "assert" calls in Git
> (excluding nedmalloc) should not magically disappear when NDEBUG is defined.

Well, no, I mostly just said that I do not think there is any point in
defining NDEBUG in the first place, as there is little or no benefit to
removing those asserts from the built product.

> So precluding a more labor intensive approach where all currently existing
> "assert(...)" calls are replaced with an "if (!...) die(...)" combination,
> providing a "verify" macro is a quick way to make that happen.  Consider
> this, was the value that Jonathan provided for the "die" string immediately
> obvious to you?  It sure wasn't to me.  That means that whoever does the
> "assert(...)" -> "if(!...)die" swap out may need to be intimately familiar
> with that particular piece of code or the result will be no better than
> using a "verify" macro.

Sure, if you want to mass-convert them, doing so with a macro similar to
assert is the simplest way. I don't think we are in a huge hurry to do
that conversion though. I'm not complaining that NDEBUG works as
advertised by disabling asserts. I'm just claiming that it's largely
pointless in our code base, and I'd consider die("BUG") to be our
"usual" style. If any change is to be made, it would be to suggest
people prefer die("BUG") as a style guideline for future patches. But I
haven't seen anybody agree (or disagree) with the notion, so I'd
hesitate to impose my style suggestion without more discussion in that
area.

If we _did_ accept that as a style guideline, then we could move over
existing assert() calls over time (either as janitorial projects, or as
people touch the related code). But there's not a pressing need to do it
quickly.

-Peff

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 21, 2016, at 18:21, Kyle J. McKay wrote:

> On Dec 21, 2016, at 07:55, Jeff King wrote:
>
>> On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:
>>
>>>> I wasn't aware anybody actually built with NDEBUG at all. You'd  
>>>> have to
>>>> explicitly ask for it via CFLAGS, so I assume most people don't.
>>>
>>> Not a good assumption.  You know what happens when you assume[1],  
>>> right? ;)
>>
>> Kind of. If it's a configuration that nobody[1] in the Git  
>> development
>> community intended to support or test, then isn't the person  
>> triggering
>> it the one making assumptions?
>
> No, I don't think so.  NDEBUG is very clearly specified in POSIX [1].
>
> If NDEBUG is defined then "assert(...)" disappears (and in a nice  
> way so as not to precipitate "unused variable" warnings).  "N" being  
> "No" or "Not" or "Negated" or "bar over the top" + "DEBUG" meaning  
> Not DEBUG.  So the code that goes away when NDEBUG is defined is  
> clearly debug code.

I think there is a useful distinction here that I make that's worth  
sharing.  Perhaps it's splitting hairs, but I categorize this "extra"  
code that we've been discussing ("assert(...)" or "if (!...) die(...)"  
or "verify(...)" into two groups:


1) DEBUG code

This is code that developers use when creating new features.  Or  
helpful code that's needed when stepping through a program with the  
debugger to debug a problem.  Or even code that's only used by some  
kind of external "test".  It may be expensive, it may do things that  
should never be done in a build for wider consumption (such as write  
information to special log files, write special syslog messages  
etc.).  Often this code is used in combination with a "-g" debug  
symbols build and possibly even a "-O0" or "-O1" option.

Code like this has no place in a release executable meant for general  
use by an end user.

2) DIAGNOSTIC code

This is near zero overhead code that is intended to be left in a  
release build meant for general use and normally sits there not doing  
anything and NOT leaching any performance out of the build either.   
Its sole purpose in life is to provide a trail of "bread crumbs" if  
the executable goes ***BOOM***.  These "bread crumbs" should be just  
enough when combined with feedback from the unfortunate user who  
experienced the meltdown to re-create the issue in a real DEBUG build  
and find and fix the problem.


It seems to me what you are saying is that Git's "assert" calls are  
DIAGNOSTIC and therefore belong in a release build -- well, except for  
the nedmalloc "assert" calls which do not.

What I'm saying is if they are diagnostic and not debug (and I'm not  
arguing one way or the other, but you've already indicated they are  
near zero overhead which suggests they are indeed diagnostic in  
nature), then they do not belong inside an "assert" which can be  
disabled with "NDEBUG".  I'm arguing that "assert" is not intended for  
diagnostic code, but only debug code as used by nedmalloc.  Having Git  
treat "NDEBUG" one way -- "no, no, do NOT define NDEBUG because that  
disables Git diagnostics and I promise you there's no performance  
penalty" -- versus nedmalloc -- "yes, yes please DO define NDEBUG  
unless you really need our slow debugging code to be present for  
debugging purposes" -- just creates needless unnecessary confusion.

--Kyle

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Jeff King]

On Wed, Dec 21, 2016 at 07:53:15PM -0800, Kyle J. McKay wrote:

> It seems to me what you are saying is that Git's "assert" calls are
> DIAGNOSTIC and therefore belong in a release build -- well, except for the
> nedmalloc "assert" calls which do not.

Yes, I think that is a good way of thinking about it (modulo that I
really can't say one way or the other about nedmalloc's uses).

There _are_ some DEBUG-type things in Git that are protected by #ifdefs
that default to "off" (grep for DIFF_DEBUG, for instance). I'm actually
of the opinion that debugging code like that should be in all builds and
triggerable at run-time, provided it carries no significant performance
penalty when the run-time switch is not enabled. But I do agree that's a
totally separate question than from your DEBUG/DIAGNOSTIC distinction.

-Peff

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Well, no, I mostly just said that I do not think there is any point in
> defining NDEBUG in the first place, as there is little or no benefit to
> removing those asserts from the built product.
> ...
> Sure, if you want to mass-convert them, doing so with a macro similar to
> assert is the simplest way. I don't think we are in a huge hurry to do
> that conversion though. I'm not complaining that NDEBUG works as
> advertised by disabling asserts. I'm just claiming that it's largely
> pointless in our code base, and I'd consider die("BUG") to be our
> "usual" style. 

I agree with all of the above. Given the way how our own code uses
assert(), there is little point removing them and turning them over
time into "if (...) die(BUG)" would probably be better.

Borrowed code like nedmalloc may be a different story, but as you
said in a separate message in this thread, I think we are better off
leaving that to those who care about that piece of code.