[PATCH] doc: sandbox: Add additional valgrind documentation

[Heinrich Schuchardt]

On 5/27/22 05:36, Sean Anderson wrote:
> This document some additional options which can be used with valgrind, as

Thanks for enhancing this document

nits
%s/document/documents/

> well as directions for future work. It also fixes up inline literals to
> actually be inline literals (and not italics). The content of this
> documentation is primarily adapted from [1].
>
> [1] https://lore.kernel.org/u-boot/57cb4b49-fa30-1194-9ac3-faa53e8033bd@gmail.com/
>
> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
> ---
>
>   doc/arch/sandbox.rst | 65 +++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 61 insertions(+), 4 deletions(-)
>
> diff --git a/doc/arch/sandbox.rst b/doc/arch/sandbox.rst
> index e1119492b4..cd5090be71 100644
> --- a/doc/arch/sandbox.rst
> +++ b/doc/arch/sandbox.rst
> @@ -479,19 +479,76 @@ It is possible to run U-Boot under valgrind to check memory allocations::
>
>       valgrind ./u-boot
>
> -For more detailed results, enable `CONFIG_VALGRIND`. There are many false
> -positives due to `malloc` itself. Suppress these with::
> +For more detailed results, enable ``CONFIG_VALGRIND``. There are many false
> +positives due to ``malloc`` itself. Suppress these with::

What do you mean by 'malloc itself'? Is it the internal implementation
of malloc? Or is it the act of calling malloc()?

This paragraph should explain what CONFIG_VALGRIND does:

The sandbox allocates a memory pool via mmap(). U-Boot's internal
malloc() and free() work on this memory pool. Custom allocators and
deallocators are by default invisible to valgrind. It is
CONFIG_VARLGRIND=y that exposes U-Boot's malloc() and free() to valgrind.

If I understand include/valgrind/valgrind.h correctly, it injects
placeholder assembler code that valgrind can replace by library calls
into valgrind itself when loading the U-Boot binary.

I miss a statement indicating that the sandbox on RISC-V has no valgrind
support, yet.

The U-Boot code does not use VALGRIND_MEMPOOL* macros to indicate which
memory pools it manages. Is this the reason for the problems we are facing?

>
>       valgrind --suppressions=scripts/u-boot.supp ./u-boot
>
>   If you are running sandbox SPL or TPL, then valgrind will not by default
>   notice when U-Boot jumps from TPL to SPL, or from SPL to U-Boot proper. To
> -fix this, use `--trace-children=yes`. To show who alloc'd some troublesome
> -memory, use `--track-origins=yes`. To uncover possible errors, try running all
> +fix this, use ``--trace-children=yes``. To show who alloc'd some troublesome
> +memory, use ``--track-origins=yes``. To uncover possible errors, try running all
>   unit tests with::
>
>       valgrind --track-origins=yes --suppressions=scripts/u-boot.supp ./u-boot -Tc 'ut all'

I would prefer a list like:

--suppressions=scripts/u-boot.supp
     Suppress false positives due to the internal implementation
     of malloc

--trace-children=yes
     Let valgrind consider the progression from TPL to SPL to main U-Boot

....


>
> +Additional options
> +^^^^^^^^^^^^^^^^^^
> +
> +The following options are useful in addition to the above examples:
> +
> +* ``--error-limit=no`` will enable printing more than 1000 errors in a
> +  single session.
> +* ``--vgdb=yes --vgdb-error=0`` will let you use gdb to attach like::
> +
> +    gdb -ex "target remote | vgdb" u-boot
> +
> +  This is very helpful for inspecting the program state when there is
> +  an error.
> +* Passing ``-t cooked`` to U-Boot will keep the console in a sane state if you
> +  terminate it early (instead of having to run tset).
> +
> +Future work
> +^^^^^^^^^^^
> +
> +The biggest limitation to the current approach is that
> +supressions don't "un-taint" uninitialized memory accesses. Currently, I
> +have dlmalloc's reads its bookkeeping information marked as a "red

This documentation does not have a single named author. The pronoun "I"
has no reference point in this context.

"its" does not refer to anything.

> +zone." This means that all reads to it are marked as illegal by

"it" has no clear reference point.

> +valgrind. This is fine for regular code, but dlmalloc really does need
> +to access this area, so we suppress its violations. However, if dlmalloc
> +then passes a result calculated from a "tainted" access, that result is
> +still tainted. So the first accessor will raise a warning. This means
> +that every construct like
> +
> +.. code-block::
> +
> +    foo = malloc(sizeof(*foo));
> +    if (!foo)
> +        return -ENOMEM;
> +
> +will raise a warning when we check the result of malloc. Whoops.
> +
> +There are three ways (as I see it) to address this:

%s/(as I see it)//

Best regards

Heinrich

> +
> +* Don't mark dlmalloc bookkeeping information as a red zone. This is the
> +  simplest solution, but reduces the power of valgrind immensely, since
> +  we can no longer determine that (e.g.) access past the end of an array
> +  is undefined.
> +* Implement red zones properly. This would involve growing every
> +  allocation by a fixed amount (16 bytes or so) and then using that
> +  extra space for a real red zone that neither regular code nor dlmalloc
> +  needs to access. Unfortunately, this would probably some fairly
> +  intensive surgery to dlmalloc to add/remove the offset appropriately.
> +* Mark bookkeeping information as valid before we use it in dlmalloc,
> +  and then mark it invalid before returning. This would be the most
> +  correct, but it would be very tricky to implement since there are so
> +  many code paths to mark. I think it would be the most effort out of
> +  the three options here.
> +
> +Until one of the above options are implemented, it will remain difficult
> +to sift through the massive amount of spurious warnings.
> +
>   Testing
>   -------
>