Re: [PATCH v4 1/3] posix: add (failing) test for REG_STARTEND

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 07/05/23 19:56, наб via Libc-alpha wrote:
> This test passes on NetBSD, the illumos gate, and musl
> with https://www.openwall.com/lists/musl/2023/04/20/2;
> it's nothing revolutionary and the behaviour it tests
> is largely guaranteed by the 4.4BSD-Lite manual;
> nevertheless, it currently fails with
>   tst-reg-startend.c: ^a: a^@c: no match$
>   tst-reg-startend.c: ^a: a^@c: wanted {1, 2}, got {1, 4}$
>   tst-reg-startend.c: ^a: abc: no match$
>   tst-reg-startend.c: ^a: abc: wanted {1, 2}, got {1, 4}$
>   tst-reg-startend.c: ^a.c$: a^@c: no match$
>   tst-reg-startend.c: ^a.c$: abc: no match$
>   tst-reg-startend.c: ^a.*c$: a^@c: no match$
>   tst-reg-startend.c: ^a.*c$: abc: no match$
>   tst-reg-startend.c: ^a[^c]c$: a^@c: no match$
>   tst-reg-startend.c: ^a[^c]c$: abc: no match$
>   tst-reg-startend.c: ^a..: a^@c: no match$
>   tst-reg-startend.c: ^a..: abc: no match$
>   tst-reg-startend.c: ..c: a^@c: no match$
> 
> The test may also be compiled stand-alone (-DSTANDALONE)
> and on all platforms that have the interface
> (hence the macro to initialise regmatch_ts,
>  which start with pointer fields on the illumos gate),
> for ease of testing and inclusion in other test suites.

Tests that should triggers newer regressions should be either marks as XFAIL,
or in this case, move after the patch that actually fixes it. 

> 
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> ---
> Resending after a week; clean rebase.
> 
>  posix/Makefile           |   1 +
>  posix/tst-reg-startend.c | 124 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 125 insertions(+)
>  create mode 100644 posix/tst-reg-startend.c
> 
> diff --git a/posix/Makefile b/posix/Makefile
> index cc77e939ad..24aeb781ca 100644
> --- a/posix/Makefile
> +++ b/posix/Makefile
> @@ -295,6 +295,7 @@ tests := \
>    tst-posix_spawn-setsid \
>    tst-preadwrite \
>    tst-preadwrite64 \
> +  tst-reg-startend \
>    tst-regcomp-truncated \
>    tst-regex \
>    tst-regex2 \
> diff --git a/posix/tst-reg-startend.c b/posix/tst-reg-startend.c
> new file mode 100644
> index 0000000000..c3bfac0359
> --- /dev/null
> +++ b/posix/tst-reg-startend.c
> @@ -0,0 +1,124 @@
> +/* Permission to use, copy, modify, and/or distribute this software for any
> +   purpose with or without fee is hereby granted.
> +
> +   THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> +   WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> +   MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> +   ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> +   WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> +   ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> +   OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.  */

I am not sure if we can accept such license. It not the current one used for
newer submission, including tests (LGPL 2.1).

> +
> +#include <assert.h>
> +#include <locale.h>
> +#include <string.h>
> +#include <regex.h>
> +#include <stdio.h>
> +#include <stdbool.h>
> +
> +
> +#define M(s, e) (regmatch_t) {.rm_so = s, .rm_eo = e}
> +#define MEQ(l, r) ((l).rm_so == (r).rm_so && (l).rm_eo == (r).rm_eo)
> +
> +static const regmatch_t bound = M(1, 4);
> +
> +static const char *const regex_ac[] =
> +  {"^a", "c$", "^a.c$", "^a.*c$", "^a[^c]c$", "^a..", "..c", "[^z]c", NULL};
> +static const char *const regex_aa[] =
> +  {"^", "^a", "a$", "^\\(a\\).\\1$", "^a[^a]*", NULL};
> +static const char *const data_ac[] = {"_a\0cdef", "_abcdef"};
> +static const char *const data_aa[] = {"_a\0adef", "_abadef"};
> +static const regmatch_t results_ac[] =
> +  {M(1, 2), M(3, 4), M(1, 4), M(1, 4), M(1, 4), M(1, 4), M(1, 4), M(2, 4)};
> +static const regmatch_t results_aa[] =
> +  {M(1, 1), M(1, 2), M(3, 4), M(1, 4), M(1, 3)};
> +static_assert(sizeof(regex_ac) / sizeof(*regex_ac) - 1 ==
> +              sizeof(results_ac) / sizeof(*results_ac), "");
> +static_assert(sizeof(regex_aa) / sizeof(*regex_aa) - 1 ==
> +              sizeof(results_aa) / sizeof(*results_aa), "");


Instead of the static_assert, why not add the input arguments and the
expect result on same struct?

> +
> +
> +static bool
> +testbunch (const char *const *regexes, const char *const data[static 2],
> +           const regmatch_t *results)
> +{
> +#define BASEERR(data)                              \
> +  err = true,                                      \
> +    fprintf (stdout, __FILE__ ": %s: ", *regexes), \
> +    fwrite (data[i] + bound.rm_so, 1, bound.rm_eo - bound.rm_so, stdout)


We have macros that already log and handle the required boilerplate to
report tests issues on support/check.h.  Newer tests should use it.

> +
> +  bool err = false;
> +  for (; *regexes; ++regexes, ++results)
> +    {
> +      regex_t rgx;
> +      assert (!regcomp (&rgx, *regexes, 0));
> +
> +      for (size_t i = 0; i < 2; ++i)
> +        {
> +          regmatch_t match = bound;
> +          if (regexec (&rgx, data[i], 1, &match, REG_STARTEND))
> +            BASEERR(data), fputs (": no match\n", stdout);
> +
> +          if (!MEQ(match, *results))
> +            BASEERR(data), fprintf (stdout, ": wanted {%d, %d}, got {%d, %d}\n",
> +                                    (int)results->rm_so, (int)results->rm_eo,
> +                                    (int)match.rm_so, (int)match.rm_eo);
> +        }
> +
> +      regfree(&rgx);
> +    }
> +
> +  return err;
> +}
> +
> +
> +static const char *const mb_data[2] = {"_aaćdef", "_aćdef"};
> +static const bool mb_exp[] = {false, true};
> +
> +static bool
> +testmb (void)
> +{
> +  bool err = false;
> +  regex_t rgx;
> +  const char *const regexes[] = {"ać"};
> +  assert (!regcomp (&rgx, *regexes, 0));
> +
> +  for (size_t i = 0; i < 2; ++i)

We have array_length macro to avoid putting array sizes everywhere (and they
work better if we want to extend the tests).

> +    {
> +      regmatch_t match = bound;
> +      if (regexec (&rgx, mb_data[i], 1, &match, REG_STARTEND) == mb_exp[i])
> +        BASEERR(mb_data), fprintf (stdout, ": %s match\n",
> +                                   mb_exp[i] ? "no" : "yes");
> +
> +      if (!MEQ(match, bound))
> +        BASEERR(mb_data), fprintf (stdout, ": wanted {%d, %d}, got {%d, %d}\n",
> +                                   (int)bound.rm_so, (int)bound.rm_eo,
> +                                   (int)match.rm_so, (int)match.rm_eo);
> +    }
> +
> +  regfree(&rgx);
> +  return err;
> +}
> +
> +
> +static int
> +do_test (int argc, char **argv)
> +{
> +  (void) argc, (void) argv;

Not really need here.

> +  assert (setlocale (LC_ALL, "C.UTF-8"));
> +
> +  return testbunch (regex_ac, data_ac, results_ac) ||
> +         testbunch (regex_aa, data_aa, results_aa) ||
> +         testmb ();
> +}
> +
> +
> +#ifndef STANDALONE
> +#include "../test-skeleton.c"

Use #include <support/test-driver.c> instead.

> +#else
> +int
> +main(int argc, char **argv)
> +{
> +  return do_test(argc, argv);
> +}
> +#endif