[RFC] scripts/tracetool: don't barf validating TCG types

Message ID 20210406165307.5993-1-alex.bennee@linaro.org
State New
Headers show
Series
  • [RFC] scripts/tracetool: don't barf validating TCG types
Related show

Commit Message

Alex Bennée April 6, 2021, 4:53 p.m.
TCG types will be transformed into the appropriate host types later on
in the tool. Try and work around this by detecting those cases and
pressing on.

[AJB: this seems a bit too hacky - but the problem is validate_type is
buried a few layers deep. Maybe we should just drop TCGv from
ALLOWED_TYPES and manually do if bit.startswith("TCGv_") in validate_type?]

Fixes: 73ff061032 ("trace: only permit standard C types and fixed size integer types")
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Cc: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 scripts/tracetool/__init__.py | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

-- 
2.20.1

Comments

Stefan Hajnoczi April 26, 2021, 2:03 p.m. | #1
On Tue, Apr 06, 2021 at 05:53:07PM +0100, Alex Bennée wrote:
> TCG types will be transformed into the appropriate host types later on

> in the tool. Try and work around this by detecting those cases and

> pressing on.

> 

> [AJB: this seems a bit too hacky - but the problem is validate_type is

> buried a few layers deep. Maybe we should just drop TCGv from

> ALLOWED_TYPES and manually do if bit.startswith("TCGv_") in validate_type?]


Please include a line from a trace-events file that triggers the issue.
It's unclear to me what the problem is although I guess it's related to
TCGv_* types being rejected by validate_type. The
bit.startswith("TCGv_") change you mentioned sounds fine or a slightly
more general ALLOWED_TYPE_PREFIXES = ['TCGv_'].

> Fixes: 73ff061032 ("trace: only permit standard C types and fixed size integer types")

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> Cc: Matheus Ferst <matheus.ferst@eldorado.org.br>

> ---

>  scripts/tracetool/__init__.py | 13 +++++++++----

>  1 file changed, 9 insertions(+), 4 deletions(-)

> 

> diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py

> index 5bc94d95cf..ea078e34b4 100644

> --- a/scripts/tracetool/__init__.py

> +++ b/scripts/tracetool/__init__.py

> @@ -87,10 +87,11 @@ def out(*lines, **kwargs):

>      "ssize_t",

>      "uintptr_t",

>      "ptrdiff_t",

> -    # Magic substitution is done by tracetool

> +    # Magic substitution is done by tracetool TCG_2_HOST


This makes it clearer that "TCG_2_HOST" is a code reference:

  # Magic substitution is done using tracetool.transform.TCG_2_HOST
Alex Bennée May 4, 2021, 3:02 p.m. | #2
Stefan Hajnoczi <stefanha@redhat.com> writes:

> [[PGP Signed Part:Undecided]]

> On Tue, Apr 06, 2021 at 05:53:07PM +0100, Alex Bennée wrote:

>> TCG types will be transformed into the appropriate host types later on

>> in the tool. Try and work around this by detecting those cases and

>> pressing on.

>> 

>> [AJB: this seems a bit too hacky - but the problem is validate_type is

>> buried a few layers deep. Maybe we should just drop TCGv from

>> ALLOWED_TYPES and manually do if bit.startswith("TCGv_") in validate_type?]

>

> Please include a line from a trace-events file that triggers the issue.

> It's unclear to me what the problem is although I guess it's related to

> TCGv_* types being rejected by validate_type. The

> bit.startswith("TCGv_") change you mentioned sounds fine or a slightly

> more general ALLOWED_TYPE_PREFIXES = ['TCGv_'].


I tried:

modified   tcg/tcg-op.c
@@ -2715,6 +2715,9 @@ void tcg_gen_lookup_and_goto_ptr(void)
         plugin_gen_disable_mem_helpers();
         ptr = tcg_temp_new_ptr();
         gen_helper_lookup_tb_ptr(ptr, cpu_env);
+
+        trace_goto_tb_tcg(0, ptr);
+
         tcg_gen_op1i(INDEX_op_goto_ptr, tcgv_ptr_arg(ptr));
         tcg_temp_free_ptr(ptr);
     } else {
modified   trace-events
@@ -136,6 +136,10 @@ vcpu guest_cpu_reset(void)
 # Targets: TCG(all)
 vcpu tcg guest_mem_before(TCGv vaddr, uint16_t info) "info=%d", "vaddr=0x%016"PRIx64" info=%d"
 
+# Mode: user, softmmu
+# Targets: TCG(all)
+tcg goto_ptr(int ignore, TCGv_ptr ptr) "goto_ptr_trans=%d", "goto_ptr=%p (%d)"
+

Which fails with:

FAILED: trace/trace-root.c 
/usr/bin/python3 ../../scripts/tracetool.py --backend=log --group=root --format=c /home/alex/lsrc/qemu.git/./trace-events trace/trace-root.c
Traceback (most recent call last):
  File "../../scripts/tracetool.py", line 154, in <module>
    main(sys.argv)
  File "../../scripts/tracetool.py", line 143, in main
    events.extend(tracetool.read_events(fh, arg))
  File "/home/alex/lsrc/qemu.git/scripts/tracetool/__init__.py", line 406, in read_events
    event = Event.build(line, lineno, fname)
  File "/home/alex/lsrc/qemu.git/scripts/tracetool/__init__.py", line 322, in build
    args = Arguments.build(groups["args"])
  File "/home/alex/lsrc/qemu.git/scripts/tracetool/__init__.py", line 154, in build
    validate_type(arg_type)
  File "/home/alex/lsrc/qemu.git/scripts/tracetool/__init__.py", line 107, in validate_type
    "declared as 'void *'" % name)
ValueError: Error at /home/alex/lsrc/qemu.git/./trace-events:141: Argument type 'TCGv_ptr' is not allowed. Only standard C types and fixed size integer types should be used. struct, union, and other complex pointer types should be declared as 'void *'

>

>> Fixes: 73ff061032 ("trace: only permit standard C types and fixed size integer types")

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> Cc: Matheus Ferst <matheus.ferst@eldorado.org.br>

>> ---

>>  scripts/tracetool/__init__.py | 13 +++++++++----

>>  1 file changed, 9 insertions(+), 4 deletions(-)

>> 

>> diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py

>> index 5bc94d95cf..ea078e34b4 100644

>> --- a/scripts/tracetool/__init__.py

>> +++ b/scripts/tracetool/__init__.py

>> @@ -87,10 +87,11 @@ def out(*lines, **kwargs):

>>      "ssize_t",

>>      "uintptr_t",

>>      "ptrdiff_t",

>> -    # Magic substitution is done by tracetool

>> +    # Magic substitution is done by tracetool TCG_2_HOST

>

> This makes it clearer that "TCG_2_HOST" is a code reference:

>

>   # Magic substitution is done using tracetool.transform.TCG_2_HOST

>

> [[End of PGP Signed Part]]



-- 
Alex Bennée

Patch

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 5bc94d95cf..ea078e34b4 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -87,10 +87,11 @@  def out(*lines, **kwargs):
     "ssize_t",
     "uintptr_t",
     "ptrdiff_t",
-    # Magic substitution is done by tracetool
+    # Magic substitution is done by tracetool TCG_2_HOST
     "TCGv",
 ]
 
+
 def validate_type(name):
     bits = name.split(" ")
     for bit in bits:
@@ -405,9 +406,13 @@  def read_events(fobj, fname):
         try:
             event = Event.build(line, lineno, fname)
         except ValueError as e:
-            arg0 = 'Error at %s:%d: %s' % (fname, lineno, e.args[0])
-            e.args = (arg0,) + e.args[1:]
-            raise
+            # these get translated by TCG_2_HOST later
+            if "TCGv_" not in e.args[0]:
+                arg0 = 'Error at %s:%d: %s' % (fname, lineno, e.args[0])
+                e.args = (arg0,) + e.args[1:]
+                raise
+            else:
+                pass
 
         # transform TCG-enabled events
         if "tcg" not in event.properties: