[PATCH] doc: sandbox: Add additional valgrind documentation

[Heinrich Schuchardt]

On 5/27/22 15:25, Sean Anderson wrote:
> 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?

Hello Sean,

thanks for revising the patch.

Will you have a look at the usefulness of VALGRIND_MEMPOOL* for U-Boot?

Best regards

Heinrich

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