Re: [RFC PATCH 15/15] config.mak.dev: add and use ASSERT_FOR_FANALYZER() macro

[Phillip Wood <phillip.wood@talktalk.net>]

> On 03 June 2022 at 19:37 Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> 
> 
> Add an ASSERT_FOR_FANALYZER() macro to quiet those -fanalyzer issues

If the purpose of the macro is to silence -fanalyzer then it would perhaps be better to name it after that purpose rather than the way that it is implemented. SILENCE_FANALYZER_WARNING() or something like that.

> that haven't been diagnosed yet to either change the code involved, or
> to add an assert() or BUG() to it to placate GCC v12's -fanalyzer
> mode.
> 
> As in the preceding commit we could also use -Wno-error=* here, which
> was the initial implementation in config.mak.dev, i.e. something like:
> 
> 	## -Wno-error=analyzer-null-dereference
> 	$(eval $(call fn_disable_analyzer, \
> 		-Wno-error=analyzer-null-dereference, \
> 		dir \
> 		gpg-interface \
> 		graph \
> 		range-diff \
> 		utf8 \
> 	))
> 	## -Wno-error=analyzer-null-dereference: pre-gcc12
> 	ifeq ($(filter gcc12,$(COMPILER_FEATURES)),)
> 	$(eval $(call fn_disable_analyzer, \
> 		-Wno-error=analyzer-null-dereference, \
> 		merge \
> 		builtin/name-rev \
> 	))
> 	endif
> 
> But any such approach will unfortunately quiet *all* in the relevant
> files, including any new ones. Instead we want to quiet specific
> issues involved with ASSERT_FOR_FANALYZER().

I had a hard time understanding this, maybe

But any such approach will unfortunately miss any new warnings as it silences them for the whole file.

Best Wishes

Phillip

> 
> A drawback of this overall approach is that this only works under
> e.g. CFLAGS=-O0, but not CFLAGS=-O3. The compiler takes the liberty to
> re-arrange our code to get around these assert()s, and other new
> issues will also crop up. All of this is expected (-fanalyzer is
> subject to optimization passes, like most other options), but let's
> focus on -O0 for now.
> 
> Commentary on specific cases:
> 
>  * builtin/name-rev.c: Since 45a14f578e1 (Revert "name-rev: release
>    unused name strings", 2022-04-22) GCC v12's -fanalyzer has complained
>    about this code, per René's analysis in [1] it's incorrect to do
>    so. So let's add an ASSERT_FOR_FANALYZER() to placate it.
> 
>  * graph.c: In 0f0f389f120 (graph: tidy up display of left-skewed
>    merges, 2019-10-15) a previous "assert" was removed, and another
>    unconditional deference of the return value of
>    first_interesting_parent() was added without checking it. Since it
>    can return NULL let's add ASSERT_FOR_FANALYZER()'s here to note for
>    -fanalyzer that we won't be dereferencing these in practice.
> 
> 1. https://lore.kernel.org/git/dece627d-ccf8-2375-0c50-c59637e561d3@web.de/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/name-rev.c |  1 +
>  config.mak.dev     |  4 ++++
>  dir.c              |  1 +
>  git-compat-util.h  | 16 ++++++++++++++++
>  gpg-interface.c    |  2 ++
>  graph.c            |  2 ++
>  line-log.c         |  2 ++
>  unpack-trees.c     |  1 +
>  utf8.c             |  1 +
>  9 files changed, 30 insertions(+)
> 
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index 02ea9d16330..419159961b9 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -223,6 +223,7 @@ static void name_rev(struct commit *start_commit,
>  			if (commit_is_before_cutoff(parent))
>  				continue;
>  
> +			ASSERT_FOR_FANALYZER(name);
>  			if (parent_number > 1) {
>  				generation = 0;
>  				distance = name->distance + MERGE_TRAVERSAL_WEIGHT;
> diff --git a/config.mak.dev b/config.mak.dev
> index d6f5be92297..1d47fc04163 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -83,6 +83,10 @@ endif
>  
>  DEVELOPER_CFLAGS += -fanalyzer
>  
> +ifeq ($(filter no-suppress-analyzer,$(DEVOPTS)),)
> +DEVELOPER_CFLAGS += -DSUPPRESS_FSANITIZER
> +endif
> +
>  ## -fanalyzer exists exists as of gcc10, but versions older than gcc12
>  ## have a lot of false positives.
>  ifeq ($(filter gcc12,$(COMPILER_FEATURES)),)
> diff --git a/dir.c b/dir.c
> index 3633c0852ee..c80b584b4a7 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -3781,6 +3781,7 @@ static void invalidate_one_directory(struct untracked_cache *uc,
>  				     struct untracked_cache_dir *ucd)
>  {
>  	uc->dir_invalidated++;
> +	ASSERT_FOR_FANALYZER(ucd);
>  	ucd->valid = 0;
>  	ucd->untracked_nr = 0;
>  }
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 96293b6c43a..a553884c048 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -984,6 +984,21 @@ static inline unsigned long cast_size_t_to_ulong(size_t a)
>  	return (unsigned long)a;
>  }
>  
> +/**
> + * Transitory helper macro to quiet currently known GCC -fsanitizer
> + * issues.
> + *
> + * We lie to it and say that we're confident that the given "expr" to
> + * ASSERT_FOR_FANALYZER() cannot be NULL (or 0). Those
> + * grandfathered-in issues should be fixed, but at least we're
> + * stemming the tide.
> + */
> +#ifdef SUPPRESS_FSANITIZER
> +#define ASSERT_FOR_FANALYZER(expr) assert((expr))
> +#else
> +#define ASSERT_FOR_FANALYZER(expr) do {} while (0)
> +#endif
> +
>  #ifdef HAVE_ALLOCA_H
>  # include <alloca.h>
>  # define xalloca(size)      (alloca(size))
> @@ -1037,6 +1052,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);
>  	BUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))
>  static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
>  {
> +	ASSERT_FOR_FANALYZER(dst);
>  	if (n)
>  		memcpy(dst, src, st_mult(size, n));
>  }
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 280f1fa1a58..9cba3b86e45 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -242,6 +242,7 @@ static void parse_gpg_output(struct signature_check *sigc)
>  					next = strchrnul(line, ' ');
>  					replace_cstring(&sigc->key, line, next);
>  					/* Do we have signer information? */
> +					ASSERT_FOR_FANALYZER(next);
>  					if (*next && (sigcheck_gpg_status[i].flags & GPG_STATUS_UID)) {
>  						line = next + 1;
>  						next = strchrnul(line, '\n');
> @@ -283,6 +284,7 @@ static void parse_gpg_output(struct signature_check *sigc)
>  					 */
>  					limit = strchrnul(line, '\n');
>  					for (j = 9; j > 0; j--) {
> +						ASSERT_FOR_FANALYZER(next);
>  						if (!*next || limit <= next)
>  							break;
>  						line = next + 1;
> diff --git a/graph.c b/graph.c
> index 568b6e7cd41..39f7e95d4ab 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -1105,6 +1105,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l
>  			seen_this = 1;
>  
>  			for (j = 0; j < graph->num_parents; j++) {
> +				ASSERT_FOR_FANALYZER(parents);
>  				par_column = graph_find_new_column_by_commit(graph, parents->item);
>  				assert(par_column >= 0);
>  
> @@ -1138,6 +1139,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l
>  			}
>  		}
>  
> +		ASSERT_FOR_FANALYZER(first_parent);
>  		if (col_commit == first_parent->item)
>  			parent_col = col;
>  	}
> diff --git a/line-log.c b/line-log.c
> index 51d93310a4d..1295f46deaf 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -154,6 +154,7 @@ static void range_set_union(struct range_set *out,
>  	while (i < a->nr || j < b->nr) {
>  		struct range *new_range;
>  		if (i < a->nr && j < b->nr) {
> +			ASSERT_FOR_FANALYZER(rb);
>  			if (ra[i].start < rb[j].start)
>  				new_range = &ra[i++];
>  			else if (ra[i].start > rb[j].start)
> @@ -166,6 +167,7 @@ static void range_set_union(struct range_set *out,
>  			new_range = &ra[i++];
>  		else                       /* a exhausted */
>  			new_range = &rb[j++];
> +		ASSERT_FOR_FANALYZER(new_range);
>  		if (new_range->start == new_range->end)
>  			; /* empty range */
>  		else if (!out->nr || out->ranges[out->nr-1].end < new_range->start) {
> diff --git a/unpack-trees.c b/unpack-trees.c
> index a1d0ff3a4d3..b6e10b05a00 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2195,6 +2195,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
>  	char *pathbuf;
>  	int cnt = 0;
>  
> +	ASSERT_FOR_FANALYZER(ce);
>  	if (S_ISGITLINK(ce->ce_mode)) {
>  		struct object_id oid;
>  		int sub_head = resolve_gitlink_ref(ce->name, "HEAD", &oid);
> diff --git a/utf8.c b/utf8.c
> index de4ce5c0e68..3ffc0a97f3b 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -130,6 +130,7 @@ static ucs_char_t pick_one_utf8_char(const char **start, size_t *remainder_p)
>  	 */
>  	remainder = (remainder_p ? *remainder_p : 999);
>  
> +	ASSERT_FOR_FANALYZER(remainder < 1 || s);
>  	if (remainder < 1) {
>  		goto invalid;
>  	} else if (*s < 0x80) {
> -- 
> 2.36.1.1124.g577fa9c2ebd
>