[PATCH] selftests/nolibc: always rebuild the sysroot when running a test

[PATCH] selftests/nolibc: always rebuild the sysroot when running a test

[Willy Tarreau]

Paul and myself got trapped a few times by not seeing the effects of
applying a patch to the nolibc source code until a "make clean" was
issued in the nolibc directory. It's particularly annoying when trying
to confirm that a proposed patch really solves a problem (or that
reverting it reintroduces the problem).

The reason for the sysroot not being rebuilt was that it can be quite
slow. But in fact it's only slow after a "make clean" issued at the
kernel's topdir, because it's the main "make headers" that can take a
tens of seconds; as long as "usr/include" still contains headers, the
"headers_install" phase is only a quick "rsync", and rebuilding the
whole nolibc sysroot takes a bit less than one second, which is perfectly
acceptable for a test, even more once the time lost caused by misleading
results if factored in.

This patch marks the sysroot target as phony and starts by clearing
the previous sysroot for the current architecture before reinstalling
it. Thanks to this, applying a patch to nolibc makes the effect
immediately visible to "make nolibc-test":

  $ time make -j -C tools/testing/selftests/nolibc nolibc-test
  make: Entering directory '/k/tools/testing/selftests/nolibc'
    MKDIR   sysroot/x86/include
  make[1]: Entering directory '/k/tools/include/nolibc'
  make[2]: Entering directory '/k'
  make[2]: Leaving directory '/k'
  make[2]: Entering directory '/k'
    INSTALL /k/tools/testing/selftests/nolibc/sysroot/sysroot/include
  make[2]: Leaving directory '/k'
  make[1]: Leaving directory '/k/tools/include/nolibc'
    CC      nolibc-test
  make: Leaving directory '/k/tools/testing/selftests/nolibc'

  real    0m0.869s
  user    0m0.716s
  sys     0m0.149s

Cc: "Paul E. McKenney" <paulmck@kernel.org>
Link: https://lore.kernel.org/all/20221021155645.GK5600@paulmck-ThinkPad-P17-Gen-1/
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
 tools/testing/selftests/nolibc/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index 69ea659caca9..22f1e1d73fa8 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -95,6 +95,7 @@ all: run
 sysroot: sysroot/$(ARCH)/include
 
 sysroot/$(ARCH)/include:
+	$(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
 	$(QUIET_MKDIR)mkdir -p sysroot
 	$(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
 	$(Q)mv sysroot/sysroot sysroot/$(ARCH)
@@ -133,3 +134,5 @@ clean:
 	$(Q)rm -rf initramfs
 	$(call QUIET_CLEAN, run.out)
 	$(Q)rm -rf run.out
+
+.PHONY: sysroot/$(ARCH)/include
-- 
2.35.3

Re: [PATCH] selftests/nolibc: always rebuild the sysroot when running a test

[Paul E. McKenney]

On Wed, Oct 26, 2022 at 07:45:08AM +0200, Willy Tarreau wrote:
> Paul and myself got trapped a few times by not seeing the effects of
> applying a patch to the nolibc source code until a "make clean" was
> issued in the nolibc directory. It's particularly annoying when trying
> to confirm that a proposed patch really solves a problem (or that
> reverting it reintroduces the problem).
> 
> The reason for the sysroot not being rebuilt was that it can be quite
> slow. But in fact it's only slow after a "make clean" issued at the
> kernel's topdir, because it's the main "make headers" that can take a
> tens of seconds; as long as "usr/include" still contains headers, the
> "headers_install" phase is only a quick "rsync", and rebuilding the
> whole nolibc sysroot takes a bit less than one second, which is perfectly
> acceptable for a test, even more once the time lost caused by misleading
> results if factored in.
> 
> This patch marks the sysroot target as phony and starts by clearing
> the previous sysroot for the current architecture before reinstalling
> it. Thanks to this, applying a patch to nolibc makes the effect
> immediately visible to "make nolibc-test":
> 
>   $ time make -j -C tools/testing/selftests/nolibc nolibc-test
>   make: Entering directory '/k/tools/testing/selftests/nolibc'
>     MKDIR   sysroot/x86/include
>   make[1]: Entering directory '/k/tools/include/nolibc'
>   make[2]: Entering directory '/k'
>   make[2]: Leaving directory '/k'
>   make[2]: Entering directory '/k'
>     INSTALL /k/tools/testing/selftests/nolibc/sysroot/sysroot/include
>   make[2]: Leaving directory '/k'
>   make[1]: Leaving directory '/k/tools/include/nolibc'
>     CC      nolibc-test
>   make: Leaving directory '/k/tools/testing/selftests/nolibc'
> 
>   real    0m0.869s
>   user    0m0.716s
>   sys     0m0.149s
> 
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Link: https://lore.kernel.org/all/20221021155645.GK5600@paulmck-ThinkPad-P17-Gen-1/
> Signed-off-by: Willy Tarreau <w@1wt.eu>

Works like a champ with reverting and unreverting c9388e0c1c6c
("tools/nolibc/string: Fix memcmp() implementation"), thank you!!!

I have queued this.  I expect to push this into the next merge window,
thus avoiding the need to document the need for "make clean" in my
pull request.  ;-)

							Thanx, Paul

> ---
>  tools/testing/selftests/nolibc/Makefile | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> index 69ea659caca9..22f1e1d73fa8 100644
> --- a/tools/testing/selftests/nolibc/Makefile
> +++ b/tools/testing/selftests/nolibc/Makefile
> @@ -95,6 +95,7 @@ all: run
>  sysroot: sysroot/$(ARCH)/include
>  
>  sysroot/$(ARCH)/include:
> +	$(Q)rm -rf sysroot/$(ARCH) sysroot/sysroot
>  	$(QUIET_MKDIR)mkdir -p sysroot
>  	$(Q)$(MAKE) -C ../../../include/nolibc ARCH=$(ARCH) OUTPUT=$(CURDIR)/sysroot/ headers_standalone
>  	$(Q)mv sysroot/sysroot sysroot/$(ARCH)
> @@ -133,3 +134,5 @@ clean:
>  	$(Q)rm -rf initramfs
>  	$(call QUIET_CLEAN, run.out)
>  	$(Q)rm -rf run.out
> +
> +.PHONY: sysroot/$(ARCH)/include
> -- 
> 2.35.3
> 

Re: [PATCH] selftests/nolibc: always rebuild the sysroot when running a test

[Willy Tarreau]

On Wed, Oct 26, 2022 at 09:48:25AM -0700, Paul E. McKenney wrote:
> On Wed, Oct 26, 2022 at 07:45:08AM +0200, Willy Tarreau wrote:
> Works like a champ with reverting and unreverting c9388e0c1c6c
> ("tools/nolibc/string: Fix memcmp() implementation"), thank you!!!

Thanks for the feedback, and glad it suits your needs as well. I
hope that it will progressively encourage a few of us to enhance
it with more tests.

> I have queued this.  I expect to push this into the next merge window,
> thus avoiding the need to document the need for "make clean" in my
> pull request.  ;-)

Stupid question, is it really worth postponing it, considering that
we've just introduced this series right now ? I mean, if the current
usage is confusing, it's probably better to propose the fix before
6.1-final ? It's not a new feature here but rather a fix for a recently
introduced one, thus I think it could be part of the next fix series.
Rest assured I don't want to put a mess into your patch workflow, just
asking :-)

Thanks!
Willy

Re: [PATCH] selftests/nolibc: always rebuild the sysroot when running a test

[Paul E. McKenney]

On Wed, Oct 26, 2022 at 09:59:02PM +0200, Willy Tarreau wrote:
> On Wed, Oct 26, 2022 at 09:48:25AM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 26, 2022 at 07:45:08AM +0200, Willy Tarreau wrote:
> > Works like a champ with reverting and unreverting c9388e0c1c6c
> > ("tools/nolibc/string: Fix memcmp() implementation"), thank you!!!
> 
> Thanks for the feedback, and glad it suits your needs as well. I
> hope that it will progressively encourage a few of us to enhance
> it with more tests.

Here is hoping!  ;-)

> > I have queued this.  I expect to push this into the next merge window,
> > thus avoiding the need to document the need for "make clean" in my
> > pull request.  ;-)
> 
> Stupid question, is it really worth postponing it, considering that
> we've just introduced this series right now ? I mean, if the current
> usage is confusing, it's probably better to propose the fix before
> 6.1-final ? It's not a new feature here but rather a fix for a recently
> introduced one, thus I think it could be part of the next fix series.
> Rest assured I don't want to put a mess into your patch workflow, just
> asking :-)

You lost me here.

My intent is to push these nolicb patches into the upcoming v6.2
merge window:

2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12
6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation
e1bbfe393c900 selftests/nolibc: Add 7 tests for memcmp()
3f2c1c45a3a9a selftests/nolibc: Always rebuild the sysroot when running a test

I didn't see the problem until I queued the third patch (e1bbfe393c900),
and it is still in -rcu, not in v6.1.

What am I missing here?

							Thanx, Paul

Re: [PATCH] selftests/nolibc: always rebuild the sysroot when running a test

[Willy Tarreau]

On Wed, Oct 26, 2022 at 01:41:38PM -0700, Paul E. McKenney wrote:
> > > I have queued this.  I expect to push this into the next merge window,
> > > thus avoiding the need to document the need for "make clean" in my
> > > pull request.  ;-)
> > 
> > Stupid question, is it really worth postponing it, considering that
> > we've just introduced this series right now ? I mean, if the current
> > usage is confusing, it's probably better to propose the fix before
> > 6.1-final ? It's not a new feature here but rather a fix for a recently
> > introduced one, thus I think it could be part of the next fix series.
> > Rest assured I don't want to put a mess into your patch workflow, just
> > asking :-)
> 
> You lost me here.

Ah sorry!

> My intent is to push these nolicb patches into the upcoming v6.2
> merge window:
> 
> 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12
> 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation
> e1bbfe393c900 selftests/nolibc: Add 7 tests for memcmp()
> 3f2c1c45a3a9a selftests/nolibc: Always rebuild the sysroot when running a test
> 
> I didn't see the problem until I queued the third patch (e1bbfe393c900),
> and it is still in -rcu, not in v6.1.
> 
> What am I missing here?

I thought that since some of them are fixes, they would be pushed during
6.1-rc so that we don't release 6.1 with known defects. For example Rasmus'
fix for memcmp() or the strlen() fix would IMHO make sense for this
release since we're aware of the bugs and we have the fixes. The 3rd one
is indeed an addition and in no way a fix and it can easily wait for 6.2.
The 4th one is more of a usability fix but I agree that for this last one
it's debatable, I was mostly seeing this as a possiility to avoid causing
needless confusion.

Hoping this clarifies my initial question.

Thanks,
Willy

Re: [PATCH] selftests/nolibc: always rebuild the sysroot when running a test

[Paul E. McKenney]

On Thu, Oct 27, 2022 at 04:34:56AM +0200, Willy Tarreau wrote:
> On Wed, Oct 26, 2022 at 01:41:38PM -0700, Paul E. McKenney wrote:
> > > > I have queued this.  I expect to push this into the next merge window,
> > > > thus avoiding the need to document the need for "make clean" in my
> > > > pull request.  ;-)
> > > 
> > > Stupid question, is it really worth postponing it, considering that
> > > we've just introduced this series right now ? I mean, if the current
> > > usage is confusing, it's probably better to propose the fix before
> > > 6.1-final ? It's not a new feature here but rather a fix for a recently
> > > introduced one, thus I think it could be part of the next fix series.
> > > Rest assured I don't want to put a mess into your patch workflow, just
> > > asking :-)
> > 
> > You lost me here.
> 
> Ah sorry!
> 
> > My intent is to push these nolicb patches into the upcoming v6.2
> > merge window:
> > 
> > 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12
> > 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation
> > e1bbfe393c900 selftests/nolibc: Add 7 tests for memcmp()
> > 3f2c1c45a3a9a selftests/nolibc: Always rebuild the sysroot when running a test
> > 
> > I didn't see the problem until I queued the third patch (e1bbfe393c900),
> > and it is still in -rcu, not in v6.1.
> > 
> > What am I missing here?
> 
> I thought that since some of them are fixes, they would be pushed during
> 6.1-rc so that we don't release 6.1 with known defects. For example Rasmus'
> fix for memcmp() or the strlen() fix would IMHO make sense for this
> release since we're aware of the bugs and we have the fixes. The 3rd one
> is indeed an addition and in no way a fix and it can easily wait for 6.2.
> The 4th one is more of a usability fix but I agree that for this last one
> it's debatable, I was mostly seeing this as a possiility to avoid causing
> needless confusion.
> 
> Hoping this clarifies my initial question.

Very much so, thank you!

I was not considering the bug fixed by the first two patches to be
serious, my mistake, apologies for my misclassification.

Given that background, I would rebase these two, test them, and send
off a pull request, probably early next week.

2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12
6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation

I would push the other two commits into the upcoming merge window.

Or might the discussion between you and Rasmus result in changes to
either of those first two commits?  If so, I should of course wait for
that discussion to resolve.

							Thanx, Paul

Re: [PATCH] selftests/nolibc: always rebuild the sysroot when running a test

[Willy Tarreau]

On Thu, Oct 27, 2022 at 10:04:53AM -0700, Paul E. McKenney wrote:
> > > My intent is to push these nolicb patches into the upcoming v6.2
> > > merge window:
> > > 
> > > 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12
> > > 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation
> > > e1bbfe393c900 selftests/nolibc: Add 7 tests for memcmp()
> > > 3f2c1c45a3a9a selftests/nolibc: Always rebuild the sysroot when running a test
> > > 
> > > I didn't see the problem until I queued the third patch (e1bbfe393c900),
> > > and it is still in -rcu, not in v6.1.
> > > 
> > > What am I missing here?
> > 
> > I thought that since some of them are fixes, they would be pushed during
> > 6.1-rc so that we don't release 6.1 with known defects. For example Rasmus'
> > fix for memcmp() or the strlen() fix would IMHO make sense for this
> > release since we're aware of the bugs and we have the fixes. The 3rd one
> > is indeed an addition and in no way a fix and it can easily wait for 6.2.
> > The 4th one is more of a usability fix but I agree that for this last one
> > it's debatable, I was mostly seeing this as a possiility to avoid causing
> > needless confusion.
> > 
> > Hoping this clarifies my initial question.
> 
> Very much so, thank you!
> 
> I was not considering the bug fixed by the first two patches to be
> serious, my mistake, apologies for my misclassification.

No worries, I wasn't probably clear upfront about the purpose.

> Given that background, I would rebase these two, test them, and send
> off a pull request, probably early next week.
> 
> 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12
> 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation

Perfect, thank you!

> I would push the other two commits into the upcoming merge window.

OK!

> Or might the discussion between you and Rasmus result in changes to
> either of those first two commits?  If so, I should of course wait for
> that discussion to resolve.

We'll see, but in any case it would just be a minor detail, but I'll
give you a quick response so that you don't have to deal with multiple
versions of the patch, we all know that it's painful.

Thanks!
Willy

Re: [PATCH] selftests/nolibc: always rebuild the sysroot when running a test

[Paul E. McKenney]

On Thu, Oct 27, 2022 at 07:13:08PM +0200, Willy Tarreau wrote:
> On Thu, Oct 27, 2022 at 10:04:53AM -0700, Paul E. McKenney wrote:
> > > > My intent is to push these nolicb patches into the upcoming v6.2
> > > > merge window:
> > > > 
> > > > 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12
> > > > 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation
> > > > e1bbfe393c900 selftests/nolibc: Add 7 tests for memcmp()
> > > > 3f2c1c45a3a9a selftests/nolibc: Always rebuild the sysroot when running a test
> > > > 
> > > > I didn't see the problem until I queued the third patch (e1bbfe393c900),
> > > > and it is still in -rcu, not in v6.1.
> > > > 
> > > > What am I missing here?
> > > 
> > > I thought that since some of them are fixes, they would be pushed during
> > > 6.1-rc so that we don't release 6.1 with known defects. For example Rasmus'
> > > fix for memcmp() or the strlen() fix would IMHO make sense for this
> > > release since we're aware of the bugs and we have the fixes. The 3rd one
> > > is indeed an addition and in no way a fix and it can easily wait for 6.2.
> > > The 4th one is more of a usability fix but I agree that for this last one
> > > it's debatable, I was mostly seeing this as a possiility to avoid causing
> > > needless confusion.
> > > 
> > > Hoping this clarifies my initial question.
> > 
> > Very much so, thank you!
> > 
> > I was not considering the bug fixed by the first two patches to be
> > serious, my mistake, apologies for my misclassification.
> 
> No worries, I wasn't probably clear upfront about the purpose.
> 
> > Given that background, I would rebase these two, test them, and send
> > off a pull request, probably early next week.
> > 
> > 2318a710bffbd tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12
> > 6937b8de8f1c3 tools/nolibc/string: Fix memcmp() implementation
> 
> Perfect, thank you!
> 
> > I would push the other two commits into the upcoming merge window.
> 
> OK!
> 
> > Or might the discussion between you and Rasmus result in changes to
> > either of those first two commits?  If so, I should of course wait for
> > that discussion to resolve.
> 
> We'll see, but in any case it would just be a minor detail, but I'll
> give you a quick response so that you don't have to deal with multiple
> versions of the patch, we all know that it's painful.

If I don't hear otherwise from you by the end of tomorrow (Friday),
Pacific Time, I will rebase those two patches in preparation for sending
a pull request for the regression.  I will of course run the pull-message
text past you before sending the pull request.

							Thanx, Paul

Re: [PATCH] selftests/nolibc: always rebuild the sysroot when running a test

[Willy Tarreau]

Hi Paul,

On Thu, Oct 27, 2022 at 11:26:29AM -0700, Paul E. McKenney wrote:
> > We'll see, but in any case it would just be a minor detail, but I'll
> > give you a quick response so that you don't have to deal with multiple
> > versions of the patch, we all know that it's painful.
> 
> If I don't hear otherwise from you by the end of tomorrow (Friday),
> Pacific Time, I will rebase those two patches in preparation for sending
> a pull request for the regression.  I will of course run the pull-message
> text past you before sending the pull request.

No news on this front, so feel free to pick what you already have.

Thank you!
Willy

Re: [PATCH] selftests/nolibc: always rebuild the sysroot when running a test

[Paul E. McKenney]

On Fri, Oct 28, 2022 at 09:34:05PM +0200, Willy Tarreau wrote:
> Hi Paul,
> 
> On Thu, Oct 27, 2022 at 11:26:29AM -0700, Paul E. McKenney wrote:
> > > We'll see, but in any case it would just be a minor detail, but I'll
> > > give you a quick response so that you don't have to deal with multiple
> > > versions of the patch, we all know that it's painful.
> > 
> > If I don't hear otherwise from you by the end of tomorrow (Friday),
> > Pacific Time, I will rebase those two patches in preparation for sending
> > a pull request for the regression.  I will of course run the pull-message
> > text past you before sending the pull request.
> 
> No news on this front, so feel free to pick what you already have.

Very good, thank you!

I currently expect to send something like the pull request shown
below early next week.

Thoughts?

							Thanx, Paul

------------------------------------------------------------------------

Hello, Linus,

This pull request fixes a couple of nolibc string-function bugs noted
by kernel test robot, Rasmus Villemoes, Willy Tarreau.

The following changes since commit 9abf2313adc1ca1b6180c508c25f22f9395cc780:

  Linux 6.1-rc1 (2022-10-16 15:36:24 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git tags/nolibc-urgent.2022.10.28a

for you to fetch changes up to b3f4f51ea68a495f8a5956064c33dce711a2df91:

  tools/nolibc/string: Fix memcmp() implementation (2022-10-28 15:07:02 -0700)

----------------------------------------------------------------
Urgent nolibc pull request for v6.1

This pull request contains a couple of commits that fix string-function
bugs introduced by:

96980b833a21 ("tools/nolibc/string: do not use __builtin_strlen() at -O0")
66b6f755ad45 ("rcutorture: Import a copy of nolibc")

These appeared in v5.19 and v5.0, respectively, but it would be good
to get these fixes in sooner rather than later.  Commits providing the
corresponding tests are in -rcu and I expect to submit them into the
upcoming v6.2 merge window.

----------------------------------------------------------------
Rasmus Villemoes (1):
      tools/nolibc/string: Fix memcmp() implementation

Willy Tarreau (1):
      tools/nolibc: Fix missing strlen() definition and infinite loop with gcc-12

 tools/include/nolibc/string.h | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Re: [PATCH] selftests/nolibc: always rebuild the sysroot when running a test

[Willy Tarreau]

On Fri, Oct 28, 2022 at 03:22:14PM -0700, Paul E. McKenney wrote:
> I currently expect to send something like the pull request shown
> below early next week.
> 
> Thoughts?

That's perfect, thank you Paul!

Willy