[PATCH] doc: sandbox: Add additional valgrind documentation
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.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::
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 -------
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
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.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 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 -------
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.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 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 -------
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.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 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
participants (2)
-
Heinrich Schuchardt -
Sean Anderson