[patch/idea] Add register scrambling to testsuite

[patch/idea] Add register scrambling to testsuite

[DJ Delorie]

[Note: I tried to add a special case for the bug noted below, but ran
out of time while trying to learn enough ppc64/vsx opcodery]

Allow for target-specific register "scrambling" - loading arbitrary
values into all registers that need not be call-saved.  These values
should be non-zero and invalid addresses, to help catch inadvertent
uses of otherwise uninitialized registers.

Intended to help prevent bugs such as those fixed by
0218463dd8265ed937622f88ac68c7d984fe0cfc

diff --git a/support/Makefile b/support/Makefile
index 9b50eac117..91b940c379 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -76,6 +76,7 @@ libsupport-routines = \
   support_quote_string \
   support_record_failure \
   support_run_diff \
+  support_scramble_registers \
   support_select_modifies_timeout \
   support_select_normalizes_timeout \
   support_set_small_thread_stack_size \
diff --git a/support/support.h b/support/support.h
index d20051da4d..3d049575d0 100644
--- a/support/support.h
+++ b/support/support.h
@@ -233,6 +233,11 @@ void support_stack_free (struct support_stack *stack);
    The returned value is the lowest file descriptor number.  */
 int support_open_dev_null_range (int num, int flags, mode_t mode);
 
+/* Write arbitrary values to all registers that can be written do, to
+   avoid assumptions about initial register contents in test
+   cases.  */
+void support_scramble_registers (void);
+
 __END_DECLS
 
 #endif /* SUPPORT_H */
diff --git a/support/support_scramble_registers.c b/support/support_scramble_registers.c
new file mode 100644
index 0000000000..d5e2d3fd6d
--- /dev/null
+++ b/support/support_scramble_registers.c
@@ -0,0 +1,29 @@
+/* scramble any call-not-preserved registers
+   Copyright (C) 2022 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <support/support.h>
+
+#include "scramble-regs.h"
+
+void
+support_scramble_registers(void)
+{
+#ifdef SCRAMBLE_REGS
+  SCRAMBLE_REGS;
+#endif
+}
diff --git a/support/support_test_main.c b/support/support_test_main.c
index 60307fd68e..0ccb182791 100644
--- a/support/support_test_main.c
+++ b/support/support_test_main.c
@@ -269,6 +269,8 @@ adjust_exit_status (int status)
 int
 support_test_main (int argc, char **argv, const struct test_config *config)
 {
+  support_scramble_registers();
+
   if (test_main_called)
     {
       printf ("error: test_main called for a second time\n");
diff --git a/sysdeps/generic/scramble-regs.h b/sysdeps/generic/scramble-regs.h
new file mode 100644
index 0000000000..7ac55d1bfc
--- /dev/null
+++ b/sysdeps/generic/scramble-regs.h
@@ -0,0 +1,36 @@
+/* scramble any call-not-preserved registers, target portion.
+   Copyright (C) 2022 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+/* Example target-specific usage:
+
+   #define SCRAMBLE_REGS \
+     asm volatile ("movl %0, %%eax" :: "i" (1234)); \
+     asm volatile ("movl %0, %%edx" :: "i" (5678));
+
+   Targets are encouraged to create their own target-specific sub-definitions, like
+   
+   #ifndef SCRAMBLE_REGS_FPU
+   #define SCRAMBLE_REGS_FPU
+   #endif
+   #define SCRAMBLE_REGS \
+     SCRAMBLE_REGS_FPU \
+     asm volatile ("..."); \
+
+*/
+
+/* #define SCRAMBLE_REGS */
diff --git a/sysdeps/powerpc/scramble-regs.h b/sysdeps/powerpc/scramble-regs.h
new file mode 100644
index 0000000000..9400b2ed6b
--- /dev/null
+++ b/sysdeps/powerpc/scramble-regs.h
@@ -0,0 +1,20 @@
+/* scramble any call-not-preserved registers, powerpc version
+   Copyright (C) 2022 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#define SCRAMBLE_REGS		       \
+  asm volatile ("li 0, %0" :: "i" (0x1235));
diff --git a/sysdeps/x86_64/scramble-regs.h b/sysdeps/x86_64/scramble-regs.h
new file mode 100644
index 0000000000..66ffab3c8b
--- /dev/null
+++ b/sysdeps/x86_64/scramble-regs.h
@@ -0,0 +1,31 @@
+/* scramble any call-not-preserved registers, x86_64 version
+   Copyright (C) 2022 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+/* SysV ABI: preserve EBX, ESP, EBP and R12-R15.  */
+
+#define SCRAMBLE_REGS		       \
+  asm volatile ("movl %0, %%eax" :: "i" (0x12345679));	\
+  asm volatile ("movl %0, %%ecx" :: "i" (0x12345679));	\
+  asm volatile ("movl %0, %%edx" :: "i" (0x12345679));	\
+  asm volatile ("movl %0, %%esi" :: "i" (0x12345679));	\
+  asm volatile ("movl %0, %%edi" :: "i" (0x12345679));	\
+  asm volatile ("mov %0, %%r8"  :: "i" (0x12345679));	\
+  asm volatile ("mov %0, %%r9"  :: "i" (0x12345679));	\
+  asm volatile ("mov %0, %%r10" :: "i" (0x12345679));	\
+  asm volatile ("mov %0, %%r11" :: "i" (0x12345679));	\
+

Re: [patch/idea] Add register scrambling to testsuite

[Noah Goldstein]

On Fri, Jun 10, 2022 at 8:53 PM DJ Delorie via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
>
> [Note: I tried to add a special case for the bug noted below, but ran
> out of time while trying to learn enough ppc64/vsx opcodery]
>
> Allow for target-specific register "scrambling" - loading arbitrary
> values into all registers that need not be call-saved.  These values
> should be non-zero and invalid addresses, to help catch inadvertent
> uses of otherwise uninitialized registers.
>
> Intended to help prevent bugs such as those fixed by
> 0218463dd8265ed937622f88ac68c7d984fe0cfc
>
> diff --git a/support/Makefile b/support/Makefile
> index 9b50eac117..91b940c379 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -76,6 +76,7 @@ libsupport-routines = \
>    support_quote_string \
>    support_record_failure \
>    support_run_diff \
> +  support_scramble_registers \
>    support_select_modifies_timeout \
>    support_select_normalizes_timeout \
>    support_set_small_thread_stack_size \
> diff --git a/support/support.h b/support/support.h
> index d20051da4d..3d049575d0 100644
> --- a/support/support.h
> +++ b/support/support.h
> @@ -233,6 +233,11 @@ void support_stack_free (struct support_stack *stack);
>     The returned value is the lowest file descriptor number.  */
>  int support_open_dev_null_range (int num, int flags, mode_t mode);
>
> +/* Write arbitrary values to all registers that can be written do, to
> +   avoid assumptions about initial register contents in test
> +   cases.  */
> +void support_scramble_registers (void);
> +
>  __END_DECLS
>
>  #endif /* SUPPORT_H */
> diff --git a/support/support_scramble_registers.c b/support/support_scramble_registers.c
> new file mode 100644
> index 0000000000..d5e2d3fd6d
> --- /dev/null
> +++ b/support/support_scramble_registers.c
> @@ -0,0 +1,29 @@
> +/* scramble any call-not-preserved registers
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <support/support.h>
> +
> +#include "scramble-regs.h"
> +
> +void
> +support_scramble_registers(void)
> +{
> +#ifdef SCRAMBLE_REGS
> +  SCRAMBLE_REGS;
> +#endif
> +}
> diff --git a/support/support_test_main.c b/support/support_test_main.c
> index 60307fd68e..0ccb182791 100644
> --- a/support/support_test_main.c
> +++ b/support/support_test_main.c
> @@ -269,6 +269,8 @@ adjust_exit_status (int status)
>  int
>  support_test_main (int argc, char **argv, const struct test_config *config)
>  {
> +  support_scramble_registers();
> +
>    if (test_main_called)
>      {
>        printf ("error: test_main called for a second time\n");
> diff --git a/sysdeps/generic/scramble-regs.h b/sysdeps/generic/scramble-regs.h
> new file mode 100644
> index 0000000000..7ac55d1bfc
> --- /dev/null
> +++ b/sysdeps/generic/scramble-regs.h
> @@ -0,0 +1,36 @@
> +/* scramble any call-not-preserved registers, target portion.
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +/* Example target-specific usage:
> +
> +   #define SCRAMBLE_REGS \
> +     asm volatile ("movl %0, %%eax" :: "i" (1234)); \
> +     asm volatile ("movl %0, %%edx" :: "i" (5678));
> +
> +   Targets are encouraged to create their own target-specific sub-definitions, like
> +
> +   #ifndef SCRAMBLE_REGS_FPU
> +   #define SCRAMBLE_REGS_FPU
> +   #endif
> +   #define SCRAMBLE_REGS \
> +     SCRAMBLE_REGS_FPU \
> +     asm volatile ("..."); \
> +
> +*/
> +
> +/* #define SCRAMBLE_REGS */
> diff --git a/sysdeps/powerpc/scramble-regs.h b/sysdeps/powerpc/scramble-regs.h
> new file mode 100644
> index 0000000000..9400b2ed6b
> --- /dev/null
> +++ b/sysdeps/powerpc/scramble-regs.h
> @@ -0,0 +1,20 @@
> +/* scramble any call-not-preserved registers, powerpc version
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#define SCRAMBLE_REGS                 \
> +  asm volatile ("li 0, %0" :: "i" (0x1235));
> diff --git a/sysdeps/x86_64/scramble-regs.h b/sysdeps/x86_64/scramble-regs.h
> new file mode 100644
> index 0000000000..66ffab3c8b
> --- /dev/null
> +++ b/sysdeps/x86_64/scramble-regs.h
> @@ -0,0 +1,31 @@
> +/* scramble any call-not-preserved registers, x86_64 version
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +/* SysV ABI: preserve EBX, ESP, EBP and R12-R15.  */
> +
> +#define SCRAMBLE_REGS                 \
> +  asm volatile ("movl %0, %%eax" :: "i" (0x12345679)); \
> +  asm volatile ("movl %0, %%ecx" :: "i" (0x12345679)); \
> +  asm volatile ("movl %0, %%edx" :: "i" (0x12345679)); \
> +  asm volatile ("movl %0, %%esi" :: "i" (0x12345679)); \
> +  asm volatile ("movl %0, %%edi" :: "i" (0x12345679)); \
> +  asm volatile ("mov %0, %%r8"  :: "i" (0x12345679));  \
> +  asm volatile ("mov %0, %%r9"  :: "i" (0x12345679));  \
> +  asm volatile ("mov %0, %%r10" :: "i" (0x12345679));  \
> +  asm volatile ("mov %0, %%r11" :: "i" (0x12345679));  \
Do these statements need to specify the register they are clobbering?
> +
>

Re: [patch/idea] Add register scrambling to testsuite

[Florian Weimer]

* Noah Goldstein via Libc-alpha:

> Do these statements need to specify the register they are clobbering?

Thet do.  This funcationality is likely more easily written as an
assembler routine that expects a function pointer and closure argument,
performs the register clobbers, and then calls the function with the
closure argument.

Thanks,
Florian

Re: [patch/idea] Add register scrambling to testsuite

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> This funcationality is likely more easily written as an assembler
> routine that expects a function pointer and closure argument, performs
> the register clobbers, and then calls the function with the closure
> argument.

I thought of putting the scramble just before the do_test calls, but
there were more than one of those, and for the cases I'm trying to
avoid, it doesn't matter when the regs are clobbered.

Calling scramble once is enough to more accurately simulate a "busy app"
where gcc would have eventually filled all the registers with something,
which is what happened in the bug I referenced.  A closure-based asm
routine would make the work much more complicated, for no real benefit.

I've seen testsuites where each call-under-test was wrapped in a routine
that called it multiple times, permuting the "untouched" registers each
time.  I don't think we need to go that far.

Re: [patch/idea] Add register scrambling to testsuite

[Matheus Castanho]

Hi DJ,

DJ Delorie via Libc-alpha <libc-alpha@sourceware.org> writes:

> Florian Weimer <fweimer@redhat.com> writes:
>> This funcationality is likely more easily written as an assembler
>> routine that expects a function pointer and closure argument, performs
>> the register clobbers, and then calls the function with the closure
>> argument.
>
> I thought of putting the scramble just before the do_test calls, but
> there were more than one of those, and for the cases I'm trying to
> avoid, it doesn't matter when the regs are clobbered.
>

I was actually working on a patch similar to this one, but looks like
you beat me to it =). And you version seems cleaner.

You could embed your call to support_scramble_registers() in the CALL
macro on test-string.h:

#define CALL(impl, ...) \
  ({ support_scramble_registers (); (* (proto_t) (impl)->fn) (__VA_ARGS__); })

This way it would apply for all optimized function calls without
requiring many more changes. It also wouldn't affect the string
benchtests, since they use a separate CALL definition from
bench-string.h

> Calling scramble once is enough to more accurately simulate a "busy app"
> where gcc would have eventually filled all the registers with something,
> which is what happened in the bug I referenced.  A closure-based asm
> routine would make the work much more complicated, for no real benefit.
>
> I've seen testsuites where each call-under-test was wrapped in a routine
> that called it multiple times, permuting the "untouched" registers each
> time.  I don't think we need to go that far.

--
Matheus Castanho

Re: [patch/idea] Add register scrambling to testsuite

[DJ Delorie]

Matheus Castanho <msc@linux.ibm.com> writes:
> You could embed your call to support_scramble_registers() in the CALL
> macro on test-string.h:

Yeah, there's probably lots of places we could tuck this, I was trying
to get the easy wins :-)

Re: [patch/idea] Add register scrambling to testsuite

[DJ Delorie]

Noah Goldstein <goldstein.w.n@gmail.com> writes:
>> +  asm volatile ("mov %0, %%r11" :: "i" (0x12345679));  \
> Do these statements need to specify the register they are clobbering?

Er, yes.  Despite being in an isolated function and volatile, I keep
forgetting that LTO invalidates those considerations.

I'm also hoping that the platform maintainers will step up and provide
the appropriate clobber lists anyway ;-)

Re: [patch/idea] Add register scrambling to testsuite

[Jeff Law]

On 6/10/2022 9:52 PM, DJ Delorie via Libc-alpha wrote:
> [Note: I tried to add a special case for the bug noted below, but ran
> out of time while trying to learn enough ppc64/vsx opcodery]
>
> Allow for target-specific register "scrambling" - loading arbitrary
> values into all registers that need not be call-saved.  These values
> should be non-zero and invalid addresses, to help catch inadvertent
> uses of otherwise uninitialized registers.
>
> Intended to help prevent bugs such as those fixed by
> 0218463dd8265ed937622f88ac68c7d984fe0cfc
If you wanted to get nasty you could intercept all calls going through 
the PLT and do something similar there as well.

Jeff

Re: [patch/idea] Add register scrambling to testsuite

[DJ Delorie]

Jeff Law via Libc-alpha <libc-alpha@sourceware.org> writes:
> If you wanted to get nasty you could intercept all calls going through 
> the PLT and do something similar there as well.

The LD_AUDIT interface may or may not be able to do that, if the backend
doesn't preserve all registers "for our convenience" :-)

Would it be too much to ask for a "gcc -fscramble-regs" that writes
something to every call-clobbered reg in every prologue?  It wouldn't
have to be random, it would just have to be something that's obviously
wrong for most uses.

Re: [patch/idea] Add register scrambling to testsuite

[Fangrui Song]

On 2022-06-13, DJ Delorie via Libc-alpha wrote:
>Jeff Law via Libc-alpha <libc-alpha@sourceware.org> writes:
>> If you wanted to get nasty you could intercept all calls going through
>> the PLT and do something similar there as well.
>
>The LD_AUDIT interface may or may not be able to do that, if the backend
>doesn't preserve all registers "for our convenience" :-)
>
>Would it be too much to ask for a "gcc -fscramble-regs" that writes
>something to every call-clobbered reg in every prologue?  It wouldn't
>have to be random, it would just have to be something that's obviously
>wrong for most uses.
>

(2020-10) gcc x86, (recent) clang x86, and (recent) clang aarch64
support -fzero-call-used-regs which is similar.  The option uses zero
though, not what we desire here :)

Re: [patch/idea] Add register scrambling to testsuite

[Siddhesh Poyarekar]

On 11/06/2022 09:22, DJ Delorie via Libc-alpha wrote:
> 
> [Note: I tried to add a special case for the bug noted below, but ran
> out of time while trying to learn enough ppc64/vsx opcodery]
> 
> Allow for target-specific register "scrambling" - loading arbitrary
> values into all registers that need not be call-saved.  These values
> should be non-zero and invalid addresses, to help catch inadvertent
> uses of otherwise uninitialized registers.
> 
> Intended to help prevent bugs such as those fixed by
> 0218463dd8265ed937622f88ac68c7d984fe0cfc

+1 to the idea, although as Jeff pointed out, PLT boundaries may be a 
more effective place, otherwise it relies on dummy values surviving 
multiple calls.  If that's the route we take, we may also want to 
measure the execution time overhead on the testsuite and if it's too 
high, put this behind a build option, e.g. make SCRAMBLE_REGS=1 check.

Also I wonder if in the longer term (i.e. when glibc can be built with 
some sanitizers enabled) this would be better suited as a 
compiler/sanitizer flag that clobbers regs at function entry instead of 
glibc trying to do the interception.  ISTM that may have broader impact 
than simply catching the ppc64le oddball case.  The performance impact 
may be insane though...

Sid

Re: [patch/idea] Add register scrambling to testsuite

[DJ Delorie]

Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
> otherwise it relies on dummy values surviving multiple calls.

Most of our test cases are short enough that the more likely case is
that a zero value in a register remains throughout, and zeros tend to
hide bugs.  If our dummy value doesn't survive multiple calls, it's
likely to be replaced by some other dummy value, so... win ;-)

> The performance impact may be insane though...

I've written checkers like that before, where the thing to do was
"bracket the bug with these calls, invoke bug, go to lunch, get results
when you return."  An hour of computing time to find a bug is better
than days of engineer time, but yeah, we don't want to make every test
do this every time.

Re: [patch/idea] Add register scrambling to testsuite

[Siddhesh Poyarekar]

On 13/06/2022 22:22, DJ Delorie wrote:
> Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
>> otherwise it relies on dummy values surviving multiple calls.
> 
> Most of our test cases are short enough that the more likely case is
> that a zero value in a register remains throughout, and zeros tend to
> hide bugs.  If our dummy value doesn't survive multiple calls, it's
> likely to be replaced by some other dummy value, so... win ;-)

Trouble is that it could be a zero dummy value, which ends up masking an 
issue.  In any case it's something we can iterate on over time.  This is 
a good start.

>> The performance impact may be insane though...
> 
> I've written checkers like that before, where the thing to do was
> "bracket the bug with these calls, invoke bug, go to lunch, get results
> when you return."  An hour of computing time to find a bug is better
> than days of engineer time, but yeah, we don't want to make every test
> do this every time.

Yeah, the performance issue only comes into play if we decide to do this 
for every test every time with, e.g. a -fscramble-regs or sanitizer.

Sid

Re: [patch/idea] Add register scrambling to testsuite

[DJ Delorie]

Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
>>> The performance impact may be insane though...
>> 
>> I've written checkers like that before, where the thing to do was
>> "bracket the bug with these calls, invoke bug, go to lunch, get results
>> when you return."  An hour of computing time to find a bug is better
>> than days of engineer time, but yeah, we don't want to make every test
>> do this every time.
>
> Yeah, the performance issue only comes into play if we decide to do this 
> for every test every time with, e.g. a -fscramble-regs or sanitizer.

To be clear, I think we're talking about two different cases...

In the case of "scramble registers at every call prolog in our
testsuite" (i.e. -fscramble-regs), I think the cost is insignificant and
the value is high, and would argue that we should encourage such
adoption.  This first patch is a subset of that, scrambling once per
test.

The checker I wrote in the past was, literally, "do a full heap
validation analysis on entry and exit of each malloc ABI function" and
caused real applications to run 100 to 1000 times slower.  While this is
a useful feature to have, and certainly faster than a human debugger,
it's not something that should ever be enabled by default.

Finding the fine line between these two is, of course, the interesting
discussion :-)

Re: [patch/idea] Add register scrambling to testsuite

[H.J. Lu]

On Fri, Jun 10, 2022 at 8:53 PM DJ Delorie via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
>
> [Note: I tried to add a special case for the bug noted below, but ran
> out of time while trying to learn enough ppc64/vsx opcodery]
>
> Allow for target-specific register "scrambling" - loading arbitrary
> values into all registers that need not be call-saved.  These values
> should be non-zero and invalid addresses, to help catch inadvertent
> uses of otherwise uninitialized registers.
>
> Intended to help prevent bugs such as those fixed by
> 0218463dd8265ed937622f88ac68c7d984fe0cfc
>
> diff --git a/support/Makefile b/support/Makefile
> index 9b50eac117..91b940c379 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -76,6 +76,7 @@ libsupport-routines = \
>    support_quote_string \
>    support_record_failure \
>    support_run_diff \
> +  support_scramble_registers \
>    support_select_modifies_timeout \
>    support_select_normalizes_timeout \
>    support_set_small_thread_stack_size \
> diff --git a/support/support.h b/support/support.h
> index d20051da4d..3d049575d0 100644
> --- a/support/support.h
> +++ b/support/support.h
> @@ -233,6 +233,11 @@ void support_stack_free (struct support_stack *stack);
>     The returned value is the lowest file descriptor number.  */
>  int support_open_dev_null_range (int num, int flags, mode_t mode);
>
> +/* Write arbitrary values to all registers that can be written do, to
> +   avoid assumptions about initial register contents in test
> +   cases.  */
> +void support_scramble_registers (void);
> +
>  __END_DECLS
>
>  #endif /* SUPPORT_H */
> diff --git a/support/support_scramble_registers.c b/support/support_scramble_registers.c
> new file mode 100644
> index 0000000000..d5e2d3fd6d
> --- /dev/null
> +++ b/support/support_scramble_registers.c
> @@ -0,0 +1,29 @@
> +/* scramble any call-not-preserved registers
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <support/support.h>
> +
> +#include "scramble-regs.h"
> +
> +void
> +support_scramble_registers(void)
> +{
> +#ifdef SCRAMBLE_REGS
> +  SCRAMBLE_REGS;
> +#endif
> +}
> diff --git a/support/support_test_main.c b/support/support_test_main.c
> index 60307fd68e..0ccb182791 100644
> --- a/support/support_test_main.c
> +++ b/support/support_test_main.c
> @@ -269,6 +269,8 @@ adjust_exit_status (int status)
>  int
>  support_test_main (int argc, char **argv, const struct test_config *config)
>  {
> +  support_scramble_registers();
> +
>    if (test_main_called)
>      {
>        printf ("error: test_main called for a second time\n");
> diff --git a/sysdeps/generic/scramble-regs.h b/sysdeps/generic/scramble-regs.h
> new file mode 100644
> index 0000000000..7ac55d1bfc
> --- /dev/null
> +++ b/sysdeps/generic/scramble-regs.h
> @@ -0,0 +1,36 @@
> +/* scramble any call-not-preserved registers, target portion.
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +/* Example target-specific usage:
> +
> +   #define SCRAMBLE_REGS \
> +     asm volatile ("movl %0, %%eax" :: "i" (1234)); \
> +     asm volatile ("movl %0, %%edx" :: "i" (5678));
> +
> +   Targets are encouraged to create their own target-specific sub-definitions, like
> +
> +   #ifndef SCRAMBLE_REGS_FPU
> +   #define SCRAMBLE_REGS_FPU
> +   #endif
> +   #define SCRAMBLE_REGS \
> +     SCRAMBLE_REGS_FPU \
> +     asm volatile ("..."); \
> +
> +*/
> +
> +/* #define SCRAMBLE_REGS */
> diff --git a/sysdeps/powerpc/scramble-regs.h b/sysdeps/powerpc/scramble-regs.h
> new file mode 100644
> index 0000000000..9400b2ed6b
> --- /dev/null
> +++ b/sysdeps/powerpc/scramble-regs.h
> @@ -0,0 +1,20 @@
> +/* scramble any call-not-preserved registers, powerpc version
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#define SCRAMBLE_REGS                 \
> +  asm volatile ("li 0, %0" :: "i" (0x1235));
> diff --git a/sysdeps/x86_64/scramble-regs.h b/sysdeps/x86_64/scramble-regs.h
> new file mode 100644
> index 0000000000..66ffab3c8b
> --- /dev/null
> +++ b/sysdeps/x86_64/scramble-regs.h
> @@ -0,0 +1,31 @@
> +/* scramble any call-not-preserved registers, x86_64 version
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +/* SysV ABI: preserve EBX, ESP, EBP and R12-R15.  */
> +
> +#define SCRAMBLE_REGS                 \
> +  asm volatile ("movl %0, %%eax" :: "i" (0x12345679)); \
> +  asm volatile ("movl %0, %%ecx" :: "i" (0x12345679)); \
> +  asm volatile ("movl %0, %%edx" :: "i" (0x12345679)); \
> +  asm volatile ("movl %0, %%esi" :: "i" (0x12345679)); \
> +  asm volatile ("movl %0, %%edi" :: "i" (0x12345679)); \
> +  asm volatile ("mov %0, %%r8"  :: "i" (0x12345679));  \
> +  asm volatile ("mov %0, %%r9"  :: "i" (0x12345679));  \
> +  asm volatile ("mov %0, %%r10" :: "i" (0x12345679));  \
> +  asm volatile ("mov %0, %%r11" :: "i" (0x12345679));  \
> +
>

Should we also scramble XMM/YMM/ZMM registers?

-- 
H.J.

Re: [patch/idea] Add register scrambling to testsuite

[Noah Goldstein]

On Mon, Jun 13, 2022 at 8:42 PM H.J. Lu via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On Fri, Jun 10, 2022 at 8:53 PM DJ Delorie via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> >
> > [Note: I tried to add a special case for the bug noted below, but ran
> > out of time while trying to learn enough ppc64/vsx opcodery]
> >
> > Allow for target-specific register "scrambling" - loading arbitrary
> > values into all registers that need not be call-saved.  These values
> > should be non-zero and invalid addresses, to help catch inadvertent
> > uses of otherwise uninitialized registers.
> >
> > Intended to help prevent bugs such as those fixed by
> > 0218463dd8265ed937622f88ac68c7d984fe0cfc
> >
> > diff --git a/support/Makefile b/support/Makefile
> > index 9b50eac117..91b940c379 100644
> > --- a/support/Makefile
> > +++ b/support/Makefile
> > @@ -76,6 +76,7 @@ libsupport-routines = \
> >    support_quote_string \
> >    support_record_failure \
> >    support_run_diff \
> > +  support_scramble_registers \
> >    support_select_modifies_timeout \
> >    support_select_normalizes_timeout \
> >    support_set_small_thread_stack_size \
> > diff --git a/support/support.h b/support/support.h
> > index d20051da4d..3d049575d0 100644
> > --- a/support/support.h
> > +++ b/support/support.h
> > @@ -233,6 +233,11 @@ void support_stack_free (struct support_stack *stack);
> >     The returned value is the lowest file descriptor number.  */
> >  int support_open_dev_null_range (int num, int flags, mode_t mode);
> >
> > +/* Write arbitrary values to all registers that can be written do, to
> > +   avoid assumptions about initial register contents in test
> > +   cases.  */
> > +void support_scramble_registers (void);
> > +
> >  __END_DECLS
> >
> >  #endif /* SUPPORT_H */
> > diff --git a/support/support_scramble_registers.c b/support/support_scramble_registers.c
> > new file mode 100644
> > index 0000000000..d5e2d3fd6d
> > --- /dev/null
> > +++ b/support/support_scramble_registers.c
> > @@ -0,0 +1,29 @@
> > +/* scramble any call-not-preserved registers
> > +   Copyright (C) 2022 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +#include <support/support.h>
> > +
> > +#include "scramble-regs.h"
> > +
> > +void
> > +support_scramble_registers(void)
> > +{
> > +#ifdef SCRAMBLE_REGS
> > +  SCRAMBLE_REGS;
> > +#endif
> > +}
> > diff --git a/support/support_test_main.c b/support/support_test_main.c
> > index 60307fd68e..0ccb182791 100644
> > --- a/support/support_test_main.c
> > +++ b/support/support_test_main.c
> > @@ -269,6 +269,8 @@ adjust_exit_status (int status)
> >  int
> >  support_test_main (int argc, char **argv, const struct test_config *config)
> >  {
> > +  support_scramble_registers();
> > +
> >    if (test_main_called)
> >      {
> >        printf ("error: test_main called for a second time\n");
> > diff --git a/sysdeps/generic/scramble-regs.h b/sysdeps/generic/scramble-regs.h
> > new file mode 100644
> > index 0000000000..7ac55d1bfc
> > --- /dev/null
> > +++ b/sysdeps/generic/scramble-regs.h
> > @@ -0,0 +1,36 @@
> > +/* scramble any call-not-preserved registers, target portion.
> > +   Copyright (C) 2022 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +/* Example target-specific usage:
> > +
> > +   #define SCRAMBLE_REGS \
> > +     asm volatile ("movl %0, %%eax" :: "i" (1234)); \
> > +     asm volatile ("movl %0, %%edx" :: "i" (5678));
> > +
> > +   Targets are encouraged to create their own target-specific sub-definitions, like
> > +
> > +   #ifndef SCRAMBLE_REGS_FPU
> > +   #define SCRAMBLE_REGS_FPU
> > +   #endif
> > +   #define SCRAMBLE_REGS \
> > +     SCRAMBLE_REGS_FPU \
> > +     asm volatile ("..."); \
> > +
> > +*/
> > +
> > +/* #define SCRAMBLE_REGS */
> > diff --git a/sysdeps/powerpc/scramble-regs.h b/sysdeps/powerpc/scramble-regs.h
> > new file mode 100644
> > index 0000000000..9400b2ed6b
> > --- /dev/null
> > +++ b/sysdeps/powerpc/scramble-regs.h
> > @@ -0,0 +1,20 @@
> > +/* scramble any call-not-preserved registers, powerpc version
> > +   Copyright (C) 2022 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +#define SCRAMBLE_REGS                 \
> > +  asm volatile ("li 0, %0" :: "i" (0x1235));
> > diff --git a/sysdeps/x86_64/scramble-regs.h b/sysdeps/x86_64/scramble-regs.h
> > new file mode 100644
> > index 0000000000..66ffab3c8b
> > --- /dev/null
> > +++ b/sysdeps/x86_64/scramble-regs.h
> > @@ -0,0 +1,31 @@
> > +/* scramble any call-not-preserved registers, x86_64 version
> > +   Copyright (C) 2022 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +/* SysV ABI: preserve EBX, ESP, EBP and R12-R15.  */
> > +
> > +#define SCRAMBLE_REGS                 \
> > +  asm volatile ("movl %0, %%eax" :: "i" (0x12345679)); \
> > +  asm volatile ("movl %0, %%ecx" :: "i" (0x12345679)); \
> > +  asm volatile ("movl %0, %%edx" :: "i" (0x12345679)); \
> > +  asm volatile ("movl %0, %%esi" :: "i" (0x12345679)); \
> > +  asm volatile ("movl %0, %%edi" :: "i" (0x12345679)); \
> > +  asm volatile ("mov %0, %%r8"  :: "i" (0x12345679));  \
> > +  asm volatile ("mov %0, %%r9"  :: "i" (0x12345679));  \
> > +  asm volatile ("mov %0, %%r10" :: "i" (0x12345679));  \
> > +  asm volatile ("mov %0, %%r11" :: "i" (0x12345679));  \
> > +
> >
>
> Should we also scramble XMM/YMM/ZMM registers?

Yes. But I think the idea is the commit is just to introduce the concept and
let the arch maintainers fill in all the holes.

Once this is in we can just throw in a `vzeroall`.
>
> --
> H.J.

Re: [patch/idea] Add register scrambling to testsuite

[DJ Delorie]

Noah Goldstein <goldstein.w.n@gmail.com> writes:
> Once this is in we can just throw in a `vzeroall`.

As long as it doesn't zero all the registers, which is what blinded the
testcase that triggered this.  Oh wait...

Ideally we'd want to fill all registers with a large-ish constant that
isn't a valid pointer, like 0x13579bdf.  "obviously wrong" is the goal.

Re: [patch/idea] Add register scrambling to testsuite

[DJ Delorie]

"H.J. Lu" <hjl.tools@gmail.com> writes:
> Should we also scramble XMM/YMM/ZMM registers?

If we can, yes!  The ppc64 bug was caused by accidentally using the
wrong register in a vector-optimized routine, so including all the
vector registers would be good.

I'm hoping the platform maintainers will, like you did, continue to
provide this information ;-)

[patch/idea] Add register scrambling to testsuite

[Wilco Dijkstra]

Hi DJ,

> Allow for target-specific register "scrambling" - loading arbitrary
> values into all registers that need not be call-saved.  These values
> should be non-zero and invalid addresses, to help catch inadvertent
> uses of otherwise uninitialized registers.

If the purpose is to debug assembler implementations, this won't work
since the compiler will use many of these registers, so you could still use
uninitialized registers that are defined by the caller. So this is hard to do
in a generic way and should really be done inside each assembler function.

Note that testing assembler functions already requires specific settings that
enable testing, for example defining a tiny pagesize is the only way to properly
test page cross handling in string functions.

Also this is the first time I've heard about an uninitialized read - the
majority of bugs in string functions are related to page cross handling,
reading before/after the input, overflow errors of the 'n' parameter, basic
logic errors or selecting wrong ifunc. Why should we do something about
uninitialized variables when the testsuite could be improved to better test
all these corner cases?

Cheers,
Wilco

Re: [patch/idea] Add register scrambling to testsuite

[Florian Weimer]

* Wilco Dijkstra via Libc-alpha:

> Also this is the first time I've heard about an uninitialized read - the
> majority of bugs in string functions are related to page cross handling,
> reading before/after the input, overflow errors of the 'n' parameter, basic
> logic errors or selecting wrong ifunc. Why should we do something about
> uninitialized variables when the testsuite could be improved to better test
> all these corner cases?

It seems to have a favorable results/effort ratio.  Perturbing the
registers during test startup (instead of passing through the zeros that
come from the kernel) shouldn't be that difficult.  Crucially, it also
does not drive up the subvariants to test, increasing testing time.

Thanks,
Florian

Re: [patch/idea] Add register scrambling to testsuite

[Wilco Dijkstra]

Hi Florian,

> It seems to have a favorable results/effort ratio.  Perturbing the
> registers during test startup (instead of passing through the zeros that
> come from the kernel) shouldn't be that difficult.  Crucially, it also
> does not drive up the subvariants to test, increasing testing time.

Sure, it is not a huge effort, but my concern is that such a feature adds
even more clutter without significant benefits. GLIBC's goal isn't to make
debugging badly written assembler easier - in fact we should discourage
assembler given that people often use it without significant performance
gains or without doing the thorough testing that it requires.

So what I'd much rather see is people helping removing old rubbish (inline)
assembler rather than adding even more...

Cheers,
Wilco

Re: [patch/idea] Add register scrambling to testsuite

[DJ Delorie]

Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
> If the purpose is to debug assembler implementations, this won't work
> since the compiler will use many of these registers,

Er, yay?  If the compiler uses the register, it will likely be non-zero,
and either way, it will be more like a "real" application where ALL the
registers have been used at some point.

The problem happens if the registers are unused from program start to
the function, and remain with the EABI initial 'zero' value.  That is
common in our trivial test cases.

> Also this is the first time I've heard about an uninitialized read

Each bug is an opportunity for something to happen the first time.

> Why should we do something about uninitialized variables when the
> testsuite could be improved to better test all these corner cases?

Well, I'm willing to add this part.  It's something that makes the
testsuite overall better, is fairly simple, and within my scope.  Fixing
all the string tests to be more robust is a worthy goal but not
something I can do.

So the question becomes, why should I do something simple, when I could
do nothing at all?  That's a silly question; it's better to do
something.