[PATCH] doc: sandbox: Add additional valgrind documentation

[PATCH] doc: sandbox: Add additional valgrind documentation

[Sean Anderson]

This document some additional options which can be used with valgrind, as
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@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::
 
     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'
 
+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
+zone." This means that all reads to it are marked as illegal by
+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:
+
+* 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
 -------
 
-- 
2.35.1

Re: [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@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
>   -------
>

Re: [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@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
>>   -------
>>
> 

Re: [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@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
>>>   -------
>>>
>>
>
>

Re: [PATCH] doc: sandbox: Add additional valgrind documentation

[Sean Anderson]

On 5/28/22 4:20 AM, Heinrich Schuchardt wrote:
> 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@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?

I don't know if this would address anything. There's still the problem that
dlmalloc has to access the red zones.

--Sean