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-faa53e803...@gmail.com/
Signed-off-by: Sean Anderson <sean...@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
-------
