Re: [PATCH] trace: use dynamic string buffer in make_flamegraph()

2024-04-10 Thread Tom Rini
On Tue, 02 Apr 2024 13:29:16 +0200, Vincent Stehlé wrote:

> The str[] buffer declared in make_flamegraph() is used to hold strings
> representing the full call-stacks recorded in traces. The size of this
> buffer is currently 500 characters and this works well for the documented
> examples.
> 
> However, it is possible to exhaust this buffer when processing traces
> captured when running the UEFI shell on aarch64 sandbox for example.
> Indeed, the maximum length needed for such traces can reach 780 characters.
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom




[PATCH] trace: use dynamic string buffer in make_flamegraph()

2024-04-02 Thread Vincent Stehlé
The str[] buffer declared in make_flamegraph() is used to hold strings
representing the full call-stacks recorded in traces. The size of this
buffer is currently 500 characters and this works well for the documented
examples.

However, it is possible to exhaust this buffer when processing traces
captured when running the UEFI shell on aarch64 sandbox for example.
Indeed, the maximum length needed for such traces can reach 780 characters.

As it is difficult to evaluate the maximum size that would ever be needed
for all the possible traces, let's use a dynamically allocated `abuf'
instead, which we reallocate when needed.

This fixes the following error:

  String too short (500 chars)

While at it, fix a few typos in strings and comments.

Signed-off-by: Vincent Stehlé 
Cc: Tom Rini 
Cc: Simon Glass 
Cc: Michal Simek 
---


Hi,

This can be verified using the `test_trace.py' pytest as a starting point.

Configure with `sandbox_defconfig' plus the following options:

  CONFIG_TRACE=y
  CONFIG_TRACE_BUFFER_SIZE=0x0200
  CONFIG_TRACE_EARLY=y
  CONFIG_TRACE_EARLY_SIZE=0x0100

Build with:

  make FTRACE=1 NO_LTO=1

Run the test with:

  ./test/py/test.py --bd sandbox --build-dir . -k trace -s

Note that no reallocation happens during the test as the initial size of
500 characters is never exhausted; reduce the size manually if you want to
force re-allocation.

Also, make sure to delete /tmp/test_trace between tests.

Best regards,
Vincent Stehlé


 tools/proftool.c | 58 
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/tools/proftool.c b/tools/proftool.c
index fca45e4a5af..c2e38099354 100644
--- a/tools/proftool.c
+++ b/tools/proftool.c
@@ -290,7 +290,7 @@ static void usage(void)
"Options:\n"
"   -c \tSpecify config file\n"
"   -f \tSpecify output subtype\n"
-   "   -m \tSpecify Systen.map file\n"
+   "   -m \tSpecify System.map file\n"
"   -o \tSpecify output file\n"
"   -t \tSpecify trace data file (from U-Boot 'trace 
calls')\n"
"   -v <0-4>\tSpecify verbosity\n"
@@ -306,7 +306,7 @@ static void usage(void)
 }
 
 /**
- * h_cmp_offset - bsearch() function to compare two functions bny their offset
+ * h_cmp_offset - bsearch() function to compare two functions by their offset
  *
  * @v1: Pointer to first function (struct func_info)
  * @v2: Pointer to second function (struct func_info)
@@ -431,7 +431,7 @@ static struct func_info *find_func_by_offset(uint offset)
 static struct func_info *find_caller_by_offset(uint offset)
 {
int low;/* least function that could be a match */
-   int high;   /* greated function that could be a match */
+   int high;   /* greatest function that could be a match */
struct func_info key;
 
low = 0;
@@ -1352,7 +1352,7 @@ static int write_pages(struct twriter *tw, enum 
out_format_t out_format,
}
 
if (!(func->flags & FUNCF_TRACE)) {
-   debug("Funcion '%s' is excluded from trace\n",
+   debug("Function '%s' is excluded from trace\n",
  func->name);
skip_count++;
continue;
@@ -1781,7 +1781,8 @@ static int make_flame_tree(enum out_format_t out_format,
  *
  * This works by maintaining a string shared across all recursive calls. The
  * function name for this node is added to the existing string, to make up the
- * full call-stack description. For example, on entry, @str might contain:
+ * full call-stack description. For example, on entry, @str_buf->data might
+ * contain:
  *
  *"initf_bootstage;bootstage_mark_name"
  *^ @base
@@ -1795,18 +1796,18 @@ static int make_flame_tree(enum out_format_t out_format,
  * @fout: Output file
  * @out_format: Output format to use
  * @node: Node to output (pass the whole tree at first)
- * @str: String to use to build the output line (e.g. 500 charas long)
- * @maxlen: Maximum length of string
+ * @str_buf: String buffer to use to build the output line
  * @base: Current base position in the string
  * @treep: Returns the resulting flamegraph tree
  * Returns 0 if OK, -1 on error
  */
 static int output_tree(FILE *fout, enum out_format_t out_format,
-  const struct flame_node *node, char *str, int maxlen,
+  const struct flame_node *node, struct abuf *str_buf,
   int base)
 {
const struct flame_node *child;
int pos;
+   char *str = abuf_data(str_buf);
 
if (node->count) {
if (out_format == OUT_FMT_FLAMEGRAPH_CALLS) {
@@ -1832,18 +1833,29 @@ static int output_tree(FILE *fout, enum out_format_t 
out_format,
if (pos)
str[pos++] = ';';
list_for_each_entry(child, >child_head, sibling_node) {
-