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.co...
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-bootIf 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