Re: [PATCH 1/4] test-lib: add XSAN_OPTIONS, inherit [AL]SAN_OPTIONS

[Taylor Blau <me@ttaylorr.com>]

On Fri, Feb 18, 2022 at 03:20:41PM -0800, Junio C Hamano wrote:
> > +# Usage (using ":" as a delimiter):
> > +#
> > +#	prepend_var VAR : $1
> > +prepend_var () {
> > +	eval "$1=$3\${$1:+$2\$$1}"
>
> This one is correct; the above sample, when passed ":" and "VAR" to
> $1 and $2, will specialize into the above example.

> > +}
> > +
> > +# If [AL]SAN is in effect we want to abort so that we notice problems.
> > +prepend_var XSAN_OPTIONS : abort_on_error=1
>
> XSAN_OPTIONS stands for "options that are common to all ?SAN", I
> guess.

I was also unclear on this. Looking around in the google/sanitizers
repository, I don't see something called "XSAN_OPTIONS" mentioned
anywhere (neither in documentation nor in the actual source code).

Is this a convenience variable that we use to store options that are
common to both ASAN_OPTIONS and LSAN_OPTIONS? If so, I am not sure the
extra confusion is worth it, since it only contains abort_on_error=1.

I guess it makes it (along with the prepend_var function introduced by
this patch) possible for a caller to write XSAN_OPTIONS=... into their
environment and then run a test script and have their settings feed into
ASAN_OPTIONS and LSAN_OPTIONS. But I don't know that callers would find
this super useful (or, at least not dramatically more convenient than
setting both variables).

I could be wrong, and I'm obviously biased towards my own usage of the
ASAN/LSAN builds, but to me this patch might be clearer without the
extra variable.

> >  # If we were built with ASAN, it may complain about leaks
> >  # of program-lifetime variables. Disable it by default to lower
> >  # the noise level. This needs to happen at the start of the script,
> >  # before we even do our "did we build git yet" check (since we don't
> >  # want that one to complain to stderr).
> > -: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
> > +prepend_var ASAN_OPTIONS : $XSAN_OPTIONS
> > +prepend_var ASAN_OPTIONS : detect_leaks=0
>
> This makes me wonder if you want to generalize prepend_var even
> further to notice when "$3" is an empty string.  It already avoids
> spending an extra colon (and introducing an empty element in the
> colon delimited list) by paying attention to the current value of
> $1, so it would make sense to do the same for the incoming value.
>
> IOW, the current prepend_var implementation relies on $XSAN_OPTIONS
> and detect_leaks=0 both being non-empty string.

Yes, agreed.

Thanks,
Taylor