On Mon, Oct 14, 2019 at 12:57 AM Thomas Huth <thuth@xxxxxxxxxx> wrote:
>
> On 11/10/2019 20.36, Bill Wendling wrote:
> > I apologize for the breakage. I'm not sure how this escaped me. Here's
> > a proposed fix. Thoughts?
> >
> > commit 5fa1940140fd75a97f3ac5ae2e4de9e1bef645d0
> > Author: Bill Wendling <morbo@xxxxxxxxxx>
> > Date: Fri Oct 11 11:26:03 2019 -0700
> >
> > Use a status enum for reporting pass/fail
> >
> > Some values passed into "report" as "pass/fail" are larger than the
> > size of the parameter. Use instead a status enum so that the size of the
> > argument no longer matters.
> >
> > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > index b6635d9..8f80a1c 100644
> > --- a/lib/libcflat.h
> > +++ b/lib/libcflat.h
> > @@ -95,13 +95,22 @@ extern int vsnprintf(char *buf, int size, const
> > char *fmt, va_list va)
> > extern int vprintf(const char *fmt, va_list va)
> > __attribute__((format(printf, 1, 0)));
> >
> > +enum status { PASSED, FAILED };
> > +
> > +#define STATUS(x) ((x) != 0 ? PASSED : FAILED)
> > +
> > +#define report(msg_fmt, status, ...) \
> > + report_status(msg_fmt, STATUS(status) __VA_OPT__(,) __VA_ARGS__)
> > +#define report_xfail(msg_fmt, xfail, status, ...) \
> > + report_xfail_status(msg_fmt, xfail, STATUS(status) __VA_OPT__(,) __VA_ARGS__)
> > +
> > void report_prefix_pushf(const char *prefix_fmt, ...)
> > __attribute__((format(printf, 1, 2)));
> > extern void report_prefix_push(const char *prefix);
> > extern void report_prefix_pop(void);
> > -extern void report(const char *msg_fmt, unsigned pass, ...)
> > +extern void report_status(const char *msg_fmt, unsigned pass, ...)
> > __attribute__((format(printf, 1, 3)));
> > -extern void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...)
> > +extern void report_xfail_status(const char *msg_fmt, bool xfail, enum
> > status status, ...)
> > __attribute__((format(printf, 1, 4)));
> > extern void report_abort(const char *msg_fmt, ...)
> > __attribute__((format(printf, 1, 2)))
> > diff --git a/lib/report.c b/lib/report.c
> > index 2a5f549..4ba2ac0 100644
> > --- a/lib/report.c
> > +++ b/lib/report.c
> > @@ -80,12 +80,12 @@ void report_prefix_pop(void)
> > spin_unlock(&lock);
> > }
> >
> > -static void va_report(const char *msg_fmt,
> > - bool pass, bool xfail, bool skip, va_list va)
> > +static void va_report(const char *msg_fmt, enum status status, bool xfail,
> > + bool skip, va_list va)
> > {
> > const char *prefix = skip ? "SKIP"
> > - : xfail ? (pass ? "XPASS" : "XFAIL")
> > - : (pass ? "PASS" : "FAIL");
> > + : xfail ? (status == PASSED ? "XPASS" : "XFAIL")
> > + : (status == PASSED ? "PASS" : "FAIL");
> >
> > spin_lock(&lock);
> >
> > @@ -96,27 +96,27 @@ static void va_report(const char *msg_fmt,
> > puts("\n");
> > if (skip)
> > skipped++;
> > - else if (xfail && !pass)
> > + else if (xfail && status == FAILED)
> > xfailures++;
> > - else if (xfail || !pass)
> > + else if (xfail || status == FAILED)
> > failures++;
> >
> > spin_unlock(&lock);
> > }
> >
> > -void report(const char *msg_fmt, unsigned pass, ...)
> > +void report_status(const char *msg_fmt, enum status status, ...)
> > {
> > va_list va;
> > - va_start(va, pass);
> > - va_report(msg_fmt, pass, false, false, va);
> > + va_start(va, status);
> > + va_report(msg_fmt, status, false, false, va);
> > va_end(va);
> > }
> >
> > -void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...)
> > +void report_xfail_status(const char *msg_fmt, bool xfail, enum status
> > status, ...)
> > {
> > va_list va;
> > - va_start(va, pass);
> > - va_report(msg_fmt, pass, xfail, false, va);
> > + va_start(va, status);
> > + va_report(msg_fmt, status, xfail, false, va);
> > va_end(va);
> > }
>
> That's certainly a solution... but I wonder whether it might be easier
> to simply fix the failing tests instead, to make sure that they do not
> pass a value > sizeof(int) to report() and report_xfail_status() ?
>
It may be easier, but it won't stop future changes from encountering
the same issue.
> Another idea would be to swap the parameters of report() and
> report_xfail_status() :
>
It's a bit non-standard, but I don't have much of a preference. It
would take changing tons of places in the code base though.
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index b94d0ac..d6d1323 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -99,10 +99,10 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
> __attribute__((format(printf, 1,
> 2)));
> extern void report_prefix_push(const char *prefix);
> extern void report_prefix_pop(void);
> -extern void report(const char *msg_fmt, bool pass, ...)
> - __attribute__((format(printf, 1,
> 3)));
> -extern void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...)
> - __attribute__((format(printf, 1,
> 4)));
> +extern void report(bool pass, const char *msg_fmt, ...)
> + __attribute__((format(printf, 2,
> 3)));
> +extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
> + __attribute__((format(printf, 3,
> 4)));
> extern void report_abort(const char *msg_fmt, ...)
> __attribute__((format(printf, 1,
> 2)))
> __attribute__((noreturn));
> diff --git a/lib/report.c b/lib/report.c
> index ca9b4fd..2255dc3 100644
> --- a/lib/report.c
> +++ b/lib/report.c
> @@ -104,18 +104,18 @@ static void va_report(const char *msg_fmt,
> spin_unlock(&lock);
> }
>
> -void report(const char *msg_fmt, bool pass, ...)
> +void report(bool pass, const char *msg_fmt, ...)
> {
> va_list va;
> - va_start(va, pass);
> + va_start(va, msg_fmt);
> va_report(msg_fmt, pass, false, false, va);
> va_end(va);
> }
>
> -void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...)
> +void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
> {
> va_list va;
> - va_start(va, pass);
> + va_start(va, msg_fmt);
> va_report(msg_fmt, pass, xfail, false, va);
> va_end(va);
> }
>
> ... then we can keep the "bool" - but we have to fix all calling sites, too.
>
> Paolo, any preferences?
>
> Thomas
