[PATCH] doc: sandbox: Add additional valgrind documentation

[Sean Anderson]

On 5/27/22 3:14 AM, Heinrich Schuchardt wrote:
> 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 believe this is correct. I will adapt your above description for v2.

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

I was not aware of this. The Kconfig should probably be updated.

> 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
> 

I will merge this with the below options.

> 
> 
>>
>> +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.

Sorry, this is a bit unclear, the wording should be something like

	Currently, dlmalloc's bookkeeping information is marked as a "red zone"

> 
>> +zone." This means that all reads to it are marked as illegal by
> 
> "it" has no clear reference point.

s/it/that area/

> 
>> +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)//
> 

Will update.

Thanks for the feedback.

--Sean

> 
>> +
>> +* 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
>>   -------
>>
>