[PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Stefan Beller]

On reviewing [1] I wondered why there are so many asserts and wondered
if these asserts could have been prevented by a better functionality around
bug reporting in our code.

Introduce a BUG_ON macro, which is superior to assert() by
 * being always there, even when compiled with NDEBUG and
 * providind an additional human readable error message, like BUG()
 
Opinions?

Thanks,
Stefan

[1] https://public-inbox.org/git/20171121205852.15731-5-git@jeffhostetler.com/

Stefan Beller (3):
  Documentation/CodingGuidelines: explain why assert is bad
  git-compat: introduce BUG_ON(condition, fmt, ...) macro
  contrib/coccinelle: convert all conditional bugs to bug_on

 Documentation/CodingGuidelines  |  3 +++
 builtin/merge.c                 |  3 +--
 contrib/coccinelle/bug_on.cocci |  8 ++++++++
 environment.c                   | 22 ++++++++++------------
 git-compat-util.h               |  4 ++++
 notes.c                         |  9 +++++----
 refs.c                          |  7 +++----
 refs/files-backend.c            | 14 ++++++--------
 refs/packed-backend.c           | 13 +++++--------
 sha1_file.c                     |  4 ++--
 tempfile.c                      | 34 ++++++++++++++++------------------
 usage.c                         | 12 +++++++++++-
 12 files changed, 74 insertions(+), 59 deletions(-)
 create mode 100644 contrib/coccinelle/bug_on.cocci

-- 
2.15.0.448.gf294e3d99a-goog

[PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/CodingGuidelines | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index c4cb5ff0d4..4f8791895b 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -386,6 +386,9 @@ For C programs:
  - Use Git's gettext wrappers to make the user interface
    translatable. See "Marking strings for translation" in po/README.
 
+ - Prefer the BUG() macro over asserts, as asserts requires that the
+   NDEBUG flag is unset on compilation to work.
+
 For Perl programs:
 
  - Most of the C guidelines above apply.
-- 
2.15.0.448.gf294e3d99a-goog

[PATCH 2/3] git-compat: introduce BUG_ON(condition, fmt, ...) macro

[Stefan Beller]

A lot of BUG() invocations are in the form of
    if (condition)
        BUG()
so moving the condition into the same line as the macro will result in
more readable code.  The conversion to use this MACRO will happen in a
later patch.

This macro in name and spirit is borrowed from linux, which defines it as

    #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)

in include/asm-generic/bug.h, however Git prefers to have a more specific
message in BUG() calls that we include as well.

In case the user doesn't have HAVE_VARIADIC_MACROS, I could not come up
with some MACRO trickery to transport the message down to BUG conditionally
such that BUG_ON is a function just like BUG() as well.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-compat-util.h |  4 ++++
 usage.c           | 12 +++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index cedad4d581..4fec462e30 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1092,9 +1092,13 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
 __attribute__((format (printf, 3, 4))) NORETURN
 void BUG_fl(const char *file, int line, const char *fmt, ...);
 #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define BUG_ON(condition, ...) do { if (condition) BUG(__VA_ARGS__); } while (0)
 #else
 __attribute__((format (printf, 1, 2))) NORETURN
 void BUG(const char *fmt, ...);
+
+__attribute__((format (printf, 2, 3)))
+void BUG_ON(int condition, const char *fmt, ...);
 #endif
 
 /*
diff --git a/usage.c b/usage.c
index cdd534c9df..3aed669181 100644
--- a/usage.c
+++ b/usage.c
@@ -240,7 +240,17 @@ NORETURN void BUG(const char *fmt, ...)
 	BUG_vfl(NULL, 0, fmt, ap);
 	va_end(ap);
 }
-#endif
+
+void BUG_ON(int condition, const char *fmt, ...)
+{
+	if (condition) {
+		va_list ap;
+		va_start(ap, fmt);
+		BUG_vfl(NULL, 0, fmt, ap);
+		va_end(ap);
+	}
+}
+#endif /* HAVE_VARIADIC_MACROS */
 
 #ifdef SUPPRESS_ANNOTATED_LEAKS
 void unleak_memory(const void *ptr, size_t len)
-- 
2.15.0.448.gf294e3d99a-goog

[PATCH 3/3] contrib/coccinelle: convert all conditional bugs to bug_on

[Stefan Beller]

Add a coccinelle patch that detects a conditional BUG and converts it to
BUG_ON.  Also apply that semantic patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/merge.c                 |  3 +--
 contrib/coccinelle/bug_on.cocci |  8 ++++++++
 environment.c                   | 22 ++++++++++------------
 notes.c                         |  9 +++++----
 refs.c                          |  7 +++----
 refs/files-backend.c            | 14 ++++++--------
 refs/packed-backend.c           | 13 +++++--------
 sha1_file.c                     |  4 ++--
 tempfile.c                      | 34 ++++++++++++++++------------------
 9 files changed, 56 insertions(+), 58 deletions(-)
 create mode 100644 contrib/coccinelle/bug_on.cocci

diff --git a/builtin/merge.c b/builtin/merge.c
index 612dd7bfb6..df5884b4c1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -770,8 +770,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	struct strbuf msg = STRBUF_INIT;
 	strbuf_addbuf(&msg, &merge_msg);
 	strbuf_addch(&msg, '\n');
-	if (squash)
-		BUG("the control must not reach here under --squash");
+	BUG_ON(squash, "the control must not reach here under --squash");
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	if (signoff)
diff --git a/contrib/coccinelle/bug_on.cocci b/contrib/coccinelle/bug_on.cocci
new file mode 100644
index 0000000000..e778d19e3c
--- /dev/null
+++ b/contrib/coccinelle/bug_on.cocci
@@ -0,0 +1,8 @@
+@@
+expression E;
+@@
+- if (E)
+-  BUG(
++ BUG_ON(E,
+   ...);
+
diff --git a/environment.c b/environment.c
index 8fa032f307..9ba01a85ec 100644
--- a/environment.c
+++ b/environment.c
@@ -177,22 +177,20 @@ int have_git_dir(void)
 
 const char *get_git_dir(void)
 {
-	if (!the_repository->gitdir)
-		BUG("git environment hasn't been setup");
+	BUG_ON(!the_repository->gitdir, "git environment hasn't been setup");
 	return the_repository->gitdir;
 }
 
 const char *get_git_common_dir(void)
 {
-	if (!the_repository->commondir)
-		BUG("git environment hasn't been setup");
+	BUG_ON(!the_repository->commondir,
+	       "git environment hasn't been setup");
 	return the_repository->commondir;
 }
 
 const char *get_git_namespace(void)
 {
-	if (!namespace)
-		BUG("git environment hasn't been setup");
+	BUG_ON(!namespace, "git environment hasn't been setup");
 	return namespace;
 }
 
@@ -242,8 +240,8 @@ const char *get_git_work_tree(void)
 
 char *get_object_directory(void)
 {
-	if (!the_repository->objectdir)
-		BUG("git environment hasn't been setup");
+	BUG_ON(!the_repository->objectdir,
+	       "git environment hasn't been setup");
 	return the_repository->objectdir;
 }
 
@@ -282,15 +280,15 @@ int odb_pack_keep(const char *name)
 
 char *get_index_file(void)
 {
-	if (!the_repository->index_file)
-		BUG("git environment hasn't been setup");
+	BUG_ON(!the_repository->index_file,
+	       "git environment hasn't been setup");
 	return the_repository->index_file;
 }
 
 char *get_graft_file(void)
 {
-	if (!the_repository->graft_file)
-		BUG("git environment hasn't been setup");
+	BUG_ON(!the_repository->graft_file,
+	       "git environment hasn't been setup");
 	return the_repository->graft_file;
 }
 
diff --git a/notes.c b/notes.c
index c7f21fae44..cae8fa0657 100644
--- a/notes.c
+++ b/notes.c
@@ -400,10 +400,11 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
 		     oid_to_hex(&subtree->val_oid));
 
 	prefix_len = subtree->key_oid.hash[KEY_INDEX];
-	if (prefix_len >= GIT_SHA1_RAWSZ)
-		BUG("prefix_len (%"PRIuMAX") is out of range", (uintmax_t)prefix_len);
-	if (prefix_len * 2 < n)
-		BUG("prefix_len (%"PRIuMAX") is too small", (uintmax_t)prefix_len);
+	BUG_ON(prefix_len >= GIT_SHA1_RAWSZ,
+	       "prefix_len (%"PRIuMAX") is out of range",
+	       (uintmax_t)prefix_len);
+	BUG_ON(prefix_len * 2 < n, "prefix_len (%"PRIuMAX") is too small",
+	       (uintmax_t)prefix_len);
 	memcpy(object_oid.hash, subtree->key_oid.hash, prefix_len);
 	while (tree_entry(&desc, &entry)) {
 		unsigned char type;
diff --git a/refs.c b/refs.c
index 339d4318ee..1a5473fbe3 100644
--- a/refs.c
+++ b/refs.c
@@ -937,8 +937,8 @@ int ref_transaction_update(struct ref_transaction *transaction,
 		return -1;
 	}
 
-	if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
-		BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);
+	BUG_ON(flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS,
+	       "illegal flags 0x%x passed to ref_transaction_update()", flags);
 
 	flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
 
@@ -1279,8 +1279,7 @@ struct ref_iterator *refs_ref_iterator_begin(
 		iter = prefix_ref_iterator_begin(iter, "", trim);
 
 	/* Sanity check for subclasses: */
-	if (!iter->ordered)
-		BUG("reference iterator is not ordered");
+	BUG_ON(!iter->ordered, "reference iterator is not ordered");
 
 	return iter;
 }
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f75d960e19..a00a7d9f78 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2208,8 +2208,8 @@ static int split_head_update(struct ref_update *update,
 	 * size, but it happens at most once per transaction.
 	 * Add new_update->refname instead of a literal "HEAD".
 	 */
-	if (strcmp(new_update->refname, "HEAD"))
-		BUG("%s unexpectedly not 'HEAD'", new_update->refname);
+	BUG_ON(strcmp(new_update->refname, "HEAD"),
+	       "%s unexpectedly not 'HEAD'", new_update->refname);
 	item = string_list_insert(affected_refnames, new_update->refname);
 	item->util = new_update;
 
@@ -2285,9 +2285,8 @@ static int split_symref_update(struct files_ref_store *refs,
 	 * referent, which might soon be freed by our caller.
 	 */
 	item = string_list_insert(affected_refnames, new_update->refname);
-	if (item->util)
-		BUG("%s unexpectedly found in affected_refnames",
-		    new_update->refname);
+	BUG_ON(item->util, "%s unexpectedly found in affected_refnames",
+	       new_update->refname);
 	item->util = new_update;
 
 	return 0;
@@ -2570,9 +2569,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,
 		struct string_list_item *item =
 			string_list_append(&affected_refnames, update->refname);
 
-		if ((update->flags & REF_IS_PRUNING) &&
-		    !(update->flags & REF_NO_DEREF))
-			BUG("REF_IS_PRUNING set without REF_NO_DEREF");
+		BUG_ON((update->flags & REF_IS_PRUNING) && !(update->flags & REF_NO_DEREF),
+		       "REF_IS_PRUNING set without REF_NO_DEREF");
 
 		/*
 		 * We store a pointer to update in item->util, but at
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 023243fd5f..e10c2f94d8 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -334,9 +334,7 @@ static void sort_snapshot(struct snapshot *snapshot)
 
 	while (pos < eof) {
 		eol = memchr(pos, '\n', eof - pos);
-		if (!eol)
-			/* The safety check should prevent this. */
-			BUG("unterminated line found in packed-refs");
+		BUG_ON(!eol, "unterminated line found in packed-refs");
 		if (eol - pos < GIT_SHA1_HEXSZ + 2)
 			die_invalid_line(snapshot->refs->path,
 					 pos, eof - pos);
@@ -349,9 +347,8 @@ static void sort_snapshot(struct snapshot *snapshot)
 			const char *peeled_start = eol;
 
 			eol = memchr(peeled_start, '\n', eof - peeled_start);
-			if (!eol)
-				/* The safety check should prevent this. */
-				BUG("unterminated peeled line found in packed-refs");
+			BUG_ON(!eol,
+			       "unterminated peeled line found in packed-refs");
 			eol++;
 		}
 
@@ -1272,8 +1269,8 @@ int is_packed_transaction_needed(struct ref_store *ref_store,
 	size_t i;
 	int ret;
 
-	if (!is_lock_file_locked(&refs->lock))
-		BUG("is_packed_transaction_needed() called while unlocked");
+	BUG_ON(!is_lock_file_locked(&refs->lock),
+	       "is_packed_transaction_needed() called while unlocked");
 
 	/*
 	 * We're only going to bother returning false for the common,
diff --git a/sha1_file.c b/sha1_file.c
index 8a7c6b7eba..f10886f907 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1885,8 +1885,8 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 	int r = 0;
 	struct object_id oid;
 
-	if (subdir_nr > 0xff)
-		BUG("invalid loose object subdirectory: %x", subdir_nr);
+	BUG_ON(subdir_nr > 0xff, "invalid loose object subdirectory: %x",
+	       subdir_nr);
 
 	origlen = path->len;
 	strbuf_complete(path, '/');
diff --git a/tempfile.c b/tempfile.c
index 5fdafdd2d2..63884b6bdd 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -107,8 +107,8 @@ static void activate_tempfile(struct tempfile *tempfile)
 {
 	static int initialized;
 
-	if (is_tempfile_active(tempfile))
-		BUG("activate_tempfile called for active object");
+	BUG_ON(is_tempfile_active(tempfile),
+	       "activate_tempfile called for active object");
 
 	if (!initialized) {
 		sigchain_push_common(remove_tempfiles_on_signal);
@@ -215,10 +215,9 @@ struct tempfile *xmks_tempfile_m(const char *template, int mode)
 
 FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)
 {
-	if (!is_tempfile_active(tempfile))
-		BUG("fdopen_tempfile() called for inactive object");
-	if (tempfile->fp)
-		BUG("fdopen_tempfile() called for open object");
+	BUG_ON(!is_tempfile_active(tempfile),
+	       "fdopen_tempfile() called for inactive object");
+	BUG_ON(tempfile->fp, "fdopen_tempfile() called for open object");
 
 	tempfile->fp = fdopen(tempfile->fd, mode);
 	return tempfile->fp;
@@ -226,22 +225,22 @@ FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)
 
 const char *get_tempfile_path(struct tempfile *tempfile)
 {
-	if (!is_tempfile_active(tempfile))
-		BUG("get_tempfile_path() called for inactive object");
+	BUG_ON(!is_tempfile_active(tempfile),
+	       "get_tempfile_path() called for inactive object");
 	return tempfile->filename.buf;
 }
 
 int get_tempfile_fd(struct tempfile *tempfile)
 {
-	if (!is_tempfile_active(tempfile))
-		BUG("get_tempfile_fd() called for inactive object");
+	BUG_ON(!is_tempfile_active(tempfile),
+	       "get_tempfile_fd() called for inactive object");
 	return tempfile->fd;
 }
 
 FILE *get_tempfile_fp(struct tempfile *tempfile)
 {
-	if (!is_tempfile_active(tempfile))
-		BUG("get_tempfile_fp() called for inactive object");
+	BUG_ON(!is_tempfile_active(tempfile),
+	       "get_tempfile_fp() called for inactive object");
 	return tempfile->fp;
 }
 
@@ -275,10 +274,9 @@ int close_tempfile_gently(struct tempfile *tempfile)
 
 int reopen_tempfile(struct tempfile *tempfile)
 {
-	if (!is_tempfile_active(tempfile))
-		BUG("reopen_tempfile called for an inactive object");
-	if (0 <= tempfile->fd)
-		BUG("reopen_tempfile called for an open object");
+	BUG_ON(!is_tempfile_active(tempfile),
+	       "reopen_tempfile called for an inactive object");
+	BUG_ON(0 <= tempfile->fd, "reopen_tempfile called for an open object");
 	tempfile->fd = open(tempfile->filename.buf, O_WRONLY);
 	return tempfile->fd;
 }
@@ -287,8 +285,8 @@ int rename_tempfile(struct tempfile **tempfile_p, const char *path)
 {
 	struct tempfile *tempfile = *tempfile_p;
 
-	if (!is_tempfile_active(tempfile))
-		BUG("rename_tempfile called for inactive object");
+	BUG_ON(!is_tempfile_active(tempfile),
+	       "rename_tempfile called for inactive object");
 
 	if (close_tempfile_gently(tempfile)) {
 		delete_tempfile(tempfile_p);
-- 
2.15.0.448.gf294e3d99a-goog

Re: [PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -386,6 +386,9 @@ For C programs:
>   - Use Git's gettext wrappers to make the user interface
>     translatable. See "Marking strings for translation" in po/README.
>  
> + - Prefer the BUG() macro over asserts, as asserts requires that the
> +   NDEBUG flag is unset on compilation to work.

nit: is there some logical place in the list of C guidelines this
should go at, instead of the last item?  Maybe near the top, since
this is one of those straightforward cases and we're just saying that
this is the startegy for asserting invariants that this project
prefers.

Separately: I am not sure we currently universally prefer BUG_ON()
over assert().  In theory, assert() is fine wherever you don't care
whether the assertion is checked at run-time --- in other words, it is
a fancy form of comment.  BUG_ON() is useful for defensive checks that
you *expect* to never trip but want to rely on afterwards.

In a certain ideal world, the preference would be reversed: you'd want
to use assert() wherever you can and require the compiler to check
that all assert()s are verifiable at compile time.  A check that a
static analyzer can verify is more valuable than a run-time check.
When a compile-time check is not possible, you'd have to fall back to
BUG_ON().

But we don't live in that ideal world.  I'm not aware of any widely
available tools for checking assert()s.  So maybe it makes sense to
forbid assert() in our codebase entirely.

That could be enforced using something like check-non-portable-shell.pl,
except

- run on C files instead of sh files
- run on Git's source code instead of t/

What do you think?

Thanks,
Jonathan

Re: [PATCH 2/3] git-compat: introduce BUG_ON(condition, fmt, ...) macro

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1092,9 +1092,13 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
>  __attribute__((format (printf, 3, 4))) NORETURN
>  void BUG_fl(const char *file, int line, const char *fmt, ...);
>  #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
> +#define BUG_ON(condition, ...) do { if (condition) BUG(__VA_ARGS__); } while (0)
>  #else
>  __attribute__((format (printf, 1, 2))) NORETURN
>  void BUG(const char *fmt, ...);
> +
> +__attribute__((format (printf, 2, 3)))
> +void BUG_ON(int condition, const char *fmt, ...);
>  #endif

I worry that these definitions are mildly incompatible: the macro
accepts anything that can go in an 'if', including pointers, and the
function only accepts an int.

Is there a way for the macro to typecheck that its argument is an
integer to avoid that?

Thanks,
Jonathan

Re: [PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad

[Stefan Beller]

On Wed, Nov 22, 2017 at 2:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

> In a certain ideal world, the preference would be reversed: you'd want
> to use assert() wherever you can and require the compiler to check
> that all assert()s are verifiable at compile time.  A check that a
> static analyzer can verify is more valuable than a run-time check.
> When a compile-time check is not possible, you'd have to fall back to
> BUG_ON().

Linux has BUILT_BUG_ON as well, which we may desire?

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Jeff King]

On Wed, Nov 22, 2017 at 02:38:24PM -0800, Stefan Beller wrote:

> On reviewing [1] I wondered why there are so many asserts and wondered
> if these asserts could have been prevented by a better functionality around
> bug reporting in our code.
> 
> Introduce a BUG_ON macro, which is superior to assert() by
>  * being always there, even when compiled with NDEBUG and
>  * providind an additional human readable error message, like BUG()

I'm not sure I agree with the aim of the series.

If people want to compile with NDEBUG, that's their business, I guess.
I don't see much _point_ in it for Git, since most of our assertions do
not respect NDEBUG, and I don't think we tend to assert in expensive
ways anyway.

I do like human readable messages. But sometimes such a message just
makes the code harder to read (and to write). E.g., is there any real
value in:

  BUG_ON(!foo, "called bar() with a foo!");

over:

  assert(foo);

? The error message you'd get from the latter is rather sparse, but the
file and line number information it contains should be enough to find
the original source line. And after all, it's not _supposed_ to happen,
so if it does you're likely going to need to dig into the source anyway.

The human-readable BUG messages I find useful add some context or
summarize the situation. E.g. (pulled from random grepping):

  BUG: color parsing ran out of space

is way better than:

  assert failed: len < 2

Likewise, in this code:

  if (hashmap_put(map, alloc_ref_store_hash_entry(name, refs)))
	die("BUG: %s ref_store '%s' initialized twice", type, name);

we get a lot of extra information:

  - the type is mentioned
  - the name variable is dereferenced
  - the implication of "initialized twice" is made clear by the author,
    which would not be immediately obvious just from seeing the failed
    call

So I _like_ good messages, but I also think a lot of assertions don't
really lend themselves to good messages. And we should shoot for just
making them easy to read and write.

I also find (as your third patch switches):

  if (!foo)
	BUG("foo has not been setup");

more readable than the BUG_ON() version, if only because it uses
traditional control flow. But that may just be because I'm used to it.
I'm sure kernel folks are used to BUG_ON() at this point, and we'd grow
used to it, too.

-Peff

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Wed, Nov 22, 2017 at 02:38:24PM -0800, Stefan Beller wrote:

>> On reviewing [1] I wondered why there are so many asserts and wondered
>> if these asserts could have been prevented by a better functionality around
>> bug reporting in our code.
>>
>> Introduce a BUG_ON macro, which is superior to assert() by
>>  * being always there, even when compiled with NDEBUG and
>>  * providind an additional human readable error message, like BUG()
>
> I'm not sure I agree with the aim of the series.
>
> If people want to compile with NDEBUG, that's their business, I guess.
> I don't see much _point_ in it for Git, since most of our assertions do
> not respect NDEBUG, and I don't think we tend to assert in expensive
> ways anyway.
>
> I do like human readable messages. But sometimes such a message just
> makes the code harder to read (and to write). E.g., is there any real
> value in:
>
>   BUG_ON(!foo, "called bar() with a foo!");
>
> over:
>
>   assert(foo);

I think you're hinting at wanting

	BUG_ON(!foo);

which is something that the Linux kernel has (and which is not done in
this series).

[...]
> I also find (as your third patch switches):
>
>   if (!foo)
> 	BUG("foo has not been setup");
>
> more readable than the BUG_ON() version, if only because it uses
> traditional control flow.

Yes, I think you're right.

Thanks,
Jonathan

Re: [PATCH 2/3] git-compat: introduce BUG_ON(condition, fmt, ...) macro

[Jeff King]

On Wed, Nov 22, 2017 at 03:02:39PM -0800, Jonathan Nieder wrote:

> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -1092,9 +1092,13 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
> >  __attribute__((format (printf, 3, 4))) NORETURN
> >  void BUG_fl(const char *file, int line, const char *fmt, ...);
> >  #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
> > +#define BUG_ON(condition, ...) do { if (condition) BUG(__VA_ARGS__); } while (0)
> >  #else
> >  __attribute__((format (printf, 1, 2))) NORETURN
> >  void BUG(const char *fmt, ...);
> > +
> > +__attribute__((format (printf, 2, 3)))
> > +void BUG_ON(int condition, const char *fmt, ...);
> >  #endif
> 
> I worry that these definitions are mildly incompatible: the macro
> accepts anything that can go in an 'if', including pointers, and the
> function only accepts an int.

I suspect this would cause real latent problems. Doing:

  const char *foo = NULL;

  ...
  BUG_ON(foo, "foo was set twice!");
  foo = xstrdup(bar);

would compile fine on modern systems, but issue a pointer-to-int
conversion warning if you didn't have variadic macros. So most git devs
would be unlikely to see it, until the trap springs on some poor soul
building on ancient AIX or something.

On the other hand, I wouldn't be surprised if our friends on less-abled
compilers see a lot of pointless warnings anyway, and almost certainly
can't deal with the equivalent of "-Werror".

I'm also not sure which compilers that even encompasses these days.
Maybe we should add variadic macros to our list of weather-balloon c99
features to test for.

> Is there a way for the macro to typecheck that its argument is an
> integer to avoid that?

I couldn't think of one.

It would be nice to just "!!condition" in the non-macro case, but of
course we can't do that automatically without a macro.

You're asking for the opposite: force the macro version to require an
int, so that the caller has to remember to do "!!condition". I can't
think of a way to do that, but I'm also not sure I like the direction,
as it pushes the effort into the callsite.

-Peff

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Jeff King]

On Wed, Nov 22, 2017 at 03:28:14PM -0800, Jonathan Nieder wrote:

> > I do like human readable messages. But sometimes such a message just
> > makes the code harder to read (and to write). E.g., is there any real
> > value in:
> >
> >   BUG_ON(!foo, "called bar() with a foo!");
> >
> > over:
> >
> >   assert(foo);
> 
> I think you're hinting at wanting
> 
> 	BUG_ON(!foo);
> 
> which is something that the Linux kernel has (and which is not done in
> this series).

Yes. I'd be fine having a single-argument BUG_ON() like that. But then,
I'm not sure what it's buying us over assert().

I get why the kernel cannot use the default "dump core and exit"
behavior of assert(), but that's basically what our BUG() does.

-Peff

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Jonathan Nieder]

Jeff King wrote:

> Yes. I'd be fine having a single-argument BUG_ON() like that. But then,
> I'm not sure what it's buying us over assert().

It lets you build with NDEBUG.  It also goes through our own die()
handler, which means that e.g. the error message gets propagated over
remote transports.

Please please please, don't rely on side-effects from assert().  They
will cause me to run into pain over time.  This issue alone might be
worth banning use of assert() in the codebase, if we can't trust
reviewers to catch problematic examples (fortunately reviewers have
been pretty good about that so far, but banning it would free up their
attention to focus on other aspects of a patch).

Jonathan

Re: [PATCH 1/3] Documentation/CodingGuidelines: explain why assert is bad

[Jonathan Nieder]

Stefan Beller wrote:
> On Wed, Nov 22, 2017 at 2:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> In a certain ideal world, the preference would be reversed: you'd want
>> to use assert() wherever you can and require the compiler to check
>> that all assert()s are verifiable at compile time.  A check that a
>> static analyzer can verify is more valuable than a run-time check.
>> When a compile-time check is not possible, you'd have to fall back to
>> BUG_ON().
>
> Linux has BUILT_BUG_ON as well, which we may desire?

I thought so until I tried to use it:
https://public-inbox.org/git/20170830065631.GH153983@aiede.mtv.corp.google.com/

So I think we are stuck with run-time checking for now.

Thanks,
Jonathan

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Jeff King]

On Wed, Nov 22, 2017 at 03:45:32PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Yes. I'd be fine having a single-argument BUG_ON() like that. But then,
> > I'm not sure what it's buying us over assert().
> 
> It lets you build with NDEBUG.

But why do you want to build with NDEBUG if nothing uses assert()? ;)

I'm being a little glib, but I think there really is a chicken-and-egg
question here. Why are people building with NDEBUG if they don't want to
disable asserts? Do they do it out of habit? A misguided sense of
performance optimization? Does it do something else I'm not aware of?

> It also goes through our own die()
> handler, which means that e.g. the error message gets propagated over
> remote transports.

BUG() doesn't go through our die handler. It hits vreportf(), which does
do some minor munging, but it always goes to stderr. Error message
propagation over the remote protocol happens either:

  1. From a parent process muxing our stderr onto the sideband.

  2. Via special wrappers like receive-pack's rp_error().

The only program I know that sets a custom die handler to change the
reporting is git-http-backend (and there it only applies to die("BUG"),
not BUG(), so with the latter you'd get a hard hangup instead of an
attempt at an http 500).

> Please please please, don't rely on side-effects from assert().  They
> will cause me to run into pain over time.  This issue alone might be
> worth banning use of assert() in the codebase, if we can't trust
> reviewers to catch problematic examples (fortunately reviewers have
> been pretty good about that so far, but banning it would free up their
> attention to focus on other aspects of a patch).

Yes, obviously side effects in an assert() are horrible. But I haven't
noticed that being a problem in our code base.

For the record, I'm totally fine with banning assert() in favor of a
custom equivalent. I just don't think we've seen any real problems with
assert in our codebase so far.

-Peff

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Jonathan Nieder]

Jeff King wrote:
> On Wed, Nov 22, 2017 at 03:45:32PM -0800, Jonathan Nieder wrote:

>> It lets you build with NDEBUG.
>
> But why do you want to build with NDEBUG if nothing uses assert()? ;)

No idea, but some distros (not Debian) have done it before and I don't
want to be burned again.

> I'm being a little glib, but I think there really is a chicken-and-egg
> question here. Why are people building with NDEBUG if they don't want to
> disable asserts?

It's because they want to disable asserts.

The semantics of assert is that they're allowed to be compiled out.  A
person setting NDEBUG is perfectly within their rights to assume that
the author of a program has followed those semantics.  Of course this
makes assert a terrible API from the point of view of Rusty Russel's
API rating scheme
<http://sweng.the-davies.net/Home/rustys-api-design-manifesto> but
it's what C gives us.

FWIW I think we've done fine at using assert so far.  But if I
understand correctly, the point of this series is to stop having to
worry about it.

[...]
>> It also goes through our own die()
>> handler, which means that e.g. the error message gets propagated over
>> remote transports.
>
> BUG() doesn't go through our die handler. It hits vreportf(), which does
> do some minor munging, but it always goes to stderr. Error message
> propagation over the remote protocol happens either:
>
>   1. From a parent process muxing our stderr onto the sideband.
>
>   2. Via special wrappers like receive-pack's rp_error().
>
> The only program I know that sets a custom die handler to change the
> reporting is git-http-backend (and there it only applies to die("BUG"),
> not BUG(), so with the latter you'd get a hard hangup instead of an
> attempt at an http 500).

Ah, interesting.  That could be worth changing, though I don't think
it's too important.

[...]
> Yes, obviously side effects in an assert() are horrible. But I haven't
> noticed that being a problem in our code base.
>
> For the record, I'm totally fine with banning assert() in favor of a
> custom equivalent. I just don't think we've seen any real problems with
> assert in our codebase so far.

It sounds like we basically agree, then. :)

Jonathan

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Jeff King]

On Wed, Nov 22, 2017 at 04:08:39PM -0800, Jonathan Nieder wrote:

> > For the record, I'm totally fine with banning assert() in favor of a
> > custom equivalent. I just don't think we've seen any real problems with
> > assert in our codebase so far.
> 
> It sounds like we basically agree, then. :)

Yeah, I didn't quite understand your argument at first. If it's that
asserts are potentially dangerous, I buy that. They haven't bit us, but
they could.

That would be a good thing for Stefan to put in his commit message. ;)

-Peff

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> FWIW I think we've done fine at using assert so far.  But if I
> understand correctly, the point of this series is to stop having to
> worry about it.

I recalled that there was at least one, and "log -Sassert" piped to
"less" that looks for "/^[ ^I]*assert\(" caught me one recent one.

    08414938 ("mailinfo.c: move side-effects outside of assert", 2016-12-19)

Even though I do not personally mind

	assert(flags & EXPECTED_BIT);
	assert(remaining_doshes == 0);

left as a reminder primarily for coders, we can do just as well do
so with

	if (remaining_doshes != 0)
		BUG("the gostak did not distim all doshes???");

So I am fine if we want to move to reduce the use of assert()s or
get rid of them.  I personally prefer (like Peff, if I am not
mistaken) an explicit use of the usual control structure, as it is
easy to follow.  BUG_ON() would become another thing readers need to
get used to, if we were to use it, and my gut feeling is that it may
not be worth it.

A few more random things related to this topic that comes to my
mind:

 - If we had a good set of tools to tell us if an expression is free
   of side-effects, then assert(<expression>) would be less
   problematic---we could mechanically check if an assert() that is
   left as a reminder for coders/readers is safe.

 - Even if we had such a check, using the check only on new changes
   when a patch is accepted is not good enough.  An assert(distim())
   may have been safe back when it was added because distim() used
   to be free of side-effects, but a later update to it may add side
   effects to it.

 - The issue that is caused by "this function used to be pure but
   lately it gained side-effects" is not limited to assert().  Using
   it in "if (condition) BUG(...)" or "BUG_ON(condition,...)" will
   not sidestep the fact that such a change will alter behaviour of
   callers of the function.  It's just that assert(condition) is
   conditionally compiled, which makes the issue a worse one.

Re: [PATCH 0/3] Introduce BUG_ON(cond, msg) MACRO

[Jeff King]

On Thu, Nov 23, 2017 at 10:38:07AM +0900, Junio C Hamano wrote:

> > FWIW I think we've done fine at using assert so far.  But if I
> > understand correctly, the point of this series is to stop having to
> > worry about it.
> 
> I recalled that there was at least one, and "log -Sassert" piped to
> "less" that looks for "/^[ ^I]*assert\(" caught me one recent one.
> 
>     08414938 ("mailinfo.c: move side-effects outside of assert", 2016-12-19)

Thanks, I forgot about that one. There's some discussion about NDEBUG in
the surrounding thread if anybody is interested:

  https://public-inbox.org/git/900a55073f78a9f19daca67e468d334@3c843fe6ba8f3c586a21345a2783aa0/

(but it's long and there's really no resolution, so you may want to skip
it).

> Even though I do not personally mind
> 
> 	assert(flags & EXPECTED_BIT);
> 	assert(remaining_doshes == 0);
> 
> left as a reminder primarily for coders, we can do just as well do
> so with
> 
> 	if (remaining_doshes != 0)
> 		BUG("the gostak did not distim all doshes???");

Yeah, agreed. The reason I do not mind the assert() form is that if you
have nothing useful to say in the BUG() sentence, it's a bit more
compact.

> So I am fine if we want to move to reduce the use of assert()s or
> get rid of them.  I personally prefer (like Peff, if I am not
> mistaken) an explicit use of the usual control structure, as it is
> easy to follow.

To clarify my position, I think BUG_ON(cond, msg) from this series
provides basically no value over "if (cond) BUG(msg)". But I could see
value in "BUG_ON(cond)" that allows the compact form but doesn't respect
NDEBUG.

-Peff