[PATCH 0/4] tracetool: show trace-events filename/lineno in fmt string errors

[PATCH 0/4] tracetool: show trace-events filename/lineno in fmt string errors

[Stefan Hajnoczi]

This patch series improves format string compiler error messages. Instead of
showing the generated file, show the trace-events file where the format string
is defined.

Stefan Hajnoczi (4):
  tracetool: add output filename command-line argument
  tracetool: add out_lineno and out_next_lineno to out()
  tracetool: add input filename and line number to Event
  tracetool: show trace-events filename/lineno in fmt string errors

 docs/devel/tracing.txt              |  3 +-
 meson.build                         |  3 +-
 scripts/tracetool.py                | 12 ++++---
 scripts/tracetool/__init__.py       | 53 +++++++++++++++++++++++++----
 scripts/tracetool/backend/ftrace.py |  4 +++
 scripts/tracetool/backend/log.py    |  4 +++
 scripts/tracetool/backend/syslog.py |  4 +++
 trace/meson.build                   | 23 +++++--------
 8 files changed, 76 insertions(+), 30 deletions(-)

-- 
2.26.2

[PATCH 1/4] tracetool: add output filename command-line argument

[Stefan Hajnoczi]

The tracetool.py script writes to stdout. This means the output filename
is not available to the script. Add the output filename to the
command-line so that the script has access to the filename.

This also simplifies the tracetool.py invocation. It's no longer
necessary to use meson's custom_build(capture : true) to save output.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 docs/devel/tracing.txt        |  3 ++-
 meson.build                   |  3 +--
 scripts/tracetool.py          | 12 +++++++-----
 scripts/tracetool/__init__.py | 18 ++++++++++++++++--
 trace/meson.build             | 23 ++++++++---------------
 5 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/docs/devel/tracing.txt b/docs/devel/tracing.txt
index 6144d9921b..c84d4c00ac 100644
--- a/docs/devel/tracing.txt
+++ b/docs/devel/tracing.txt
@@ -318,7 +318,8 @@ probes:
                          --target-type system \
                          --target-name x86_64 \
                          --group=all \
-                         trace-events-all >qemu.stp
+                         trace-events-all \
+                         qemu.stp
 
 To facilitate simple usage of systemtap where there merely needs to be printf
 logging of certain probes, a helper script "qemu-trace-stap" is provided.
diff --git a/meson.build b/meson.build
index f0fe5f8799..fadeb0c268 100644
--- a/meson.build
+++ b/meson.build
@@ -1037,7 +1037,6 @@ foreach target : target_dirs
         custom_target(exe_name + stp['ext'],
                       input: trace_events_all,
                       output: exe_name + stp['ext'],
-                      capture: true,
                       install: stp['install'],
                       install_dir: config_host['qemu_datadir'] / '../systemtap/tapset',
                       command: [
@@ -1046,7 +1045,7 @@ foreach target : target_dirs
                         '--target-name=' + target_name,
                         '--target-type=' + target_type,
                         '--probe-prefix=qemu.' + target_type + '.' + target_name,
-                        '@INPUT@',
+                        '@INPUT@', '@OUTPUT@'
                       ])
       endforeach
     endif
diff --git a/scripts/tracetool.py b/scripts/tracetool.py
index 31146242b7..ab7653a5ce 100644
--- a/scripts/tracetool.py
+++ b/scripts/tracetool.py
@@ -16,7 +16,7 @@ __email__      = "stefanha@redhat.com"
 import sys
 import getopt
 
-from tracetool import error_write, out
+from tracetool import error_write, out, out_open
 import tracetool.backend
 import tracetool.format
 
@@ -32,7 +32,7 @@ def error_opt(msg = None):
     format_descr = "\n".join([ "    %-15s %s" % (n, d)
                                for n,d in tracetool.format.get_list() ])
     error_write("""\
-Usage: %(script)s --format=<format> --backends=<backends> [<options>]
+Usage: %(script)s --format=<format> --backends=<backends> [<options>] <trace-events> ... <output>
 
 Backends:
 %(backends)s
@@ -135,13 +135,15 @@ def main(args):
         if probe_prefix is None:
             probe_prefix = ".".join(["qemu", target_type, target_name])
 
-    if len(args) < 1:
-        error_opt("missing trace-events filepath")
+    if len(args) < 2:
+        error_opt("missing trace-events and output filepaths")
     events = []
-    for arg in args:
+    for arg in args[:-1]:
         with open(arg, "r") as fh:
             events.extend(tracetool.read_events(fh, arg))
 
+    out_open(args[-1])
+
     try:
         tracetool.generate(events, arg_group, arg_format, arg_backends,
                            binary=binary, probe_prefix=probe_prefix)
diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 3ccfa1e116..98104fa50e 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -31,14 +31,28 @@ def error(*lines):
     sys.exit(1)
 
 
+out_filename = '<none>'
+out_fobj = sys.stdout
+
+def out_open(filename):
+    global out_filename, out_fobj
+    out_filename = filename
+    out_fobj = open(filename, 'wt')
+
 def out(*lines, **kwargs):
     """Write a set of output lines.
 
     You can use kwargs as a shorthand for mapping variables when formating all
     the strings in lines.
+
+    The 'out_filename' kwarg is automatically added with the output filename.
     """
-    lines = [ l % kwargs for l in lines ]
-    sys.stdout.writelines("\n".join(lines) + "\n")
+    output = []
+    for l in lines:
+        kwargs['out_filename'] = out_filename
+        output.append(l % kwargs)
+
+    out_fobj.writelines("\n".join(output) + "\n")
 
 # We only want to allow standard C types or fixed sized
 # integer types. We don't want QEMU specific types
diff --git a/trace/meson.build b/trace/meson.build
index 56e870848e..16eccc0c22 100644
--- a/trace/meson.build
+++ b/trace/meson.build
@@ -11,20 +11,17 @@ foreach dir : [ '.' ] + trace_events_subdirs
   trace_h = custom_target(fmt.format('trace', 'h'),
                           output: fmt.format('trace', 'h'),
                           input: trace_events_file,
-                          command: [ tracetool, group, '--format=h', '@INPUT@' ],
-                          capture: true)
+                          command: [ tracetool, group, '--format=h', '@INPUT@', '@OUTPUT@' ])
   genh += trace_h
   trace_c = custom_target(fmt.format('trace', 'c'),
                           output: fmt.format('trace', 'c'),
                           input: trace_events_file,
-                          command: [ tracetool, group, '--format=c', '@INPUT@' ],
-                          capture: true)
+                          command: [ tracetool, group, '--format=c', '@INPUT@', '@OUTPUT@' ])
   if 'CONFIG_TRACE_UST' in config_host
     trace_ust_h = custom_target(fmt.format('trace-ust', 'h'),
                                 output: fmt.format('trace-ust', 'h'),
                                 input: trace_events_file,
-                                command: [ tracetool, group, '--format=ust-events-h', '@INPUT@' ],
-                                capture: true)
+                                command: [ tracetool, group, '--format=ust-events-h', '@INPUT@', '@OUTPUT@' ])
     trace_ss.add(trace_ust_h, lttng, urcubp)
     genh += trace_ust_h
   endif
@@ -33,8 +30,7 @@ foreach dir : [ '.' ] + trace_events_subdirs
     trace_dtrace = custom_target(fmt.format('trace-dtrace', 'dtrace'),
                                  output: fmt.format('trace-dtrace', 'dtrace'),
                                  input: trace_events_file,
-                                 command: [ tracetool, group, '--format=d', '@INPUT@' ],
-                                 capture: true)
+                                 command: [ tracetool, group, '--format=d', '@INPUT@', '@OUTPUT@' ])
     trace_dtrace_h = custom_target(fmt.format('trace-dtrace', 'h'),
                                    output: fmt.format('trace-dtrace', 'h'),
                                    input: trace_dtrace,
@@ -66,9 +62,8 @@ foreach d : [
   gen = custom_target(d[0],
                 output: d[0],
                 input: meson.source_root() / 'trace-events',
-                command: [ tracetool, '--group=root', '--format=@0@'.format(d[1]), '@INPUT@' ],
-                build_by_default: true, # to be removed when added to a target
-                capture: true)
+                command: [ tracetool, '--group=root', '--format=@0@'.format(d[1]), '@INPUT@', '@OUTPUT@' ],
+                build_by_default: true) # to be removed when added to a target
   specific_ss.add(gen)
 endforeach
 
@@ -76,13 +71,11 @@ if 'CONFIG_TRACE_UST' in config_host
   trace_ust_all_h = custom_target('trace-ust-all.h',
                                   output: 'trace-ust-all.h',
                                   input: trace_events_files,
-                                  command: [ tracetool, '--group=all', '--format=ust-events-h', '@INPUT@' ],
-                                  capture: true)
+                                  command: [ tracetool, '--group=all', '--format=ust-events-h', '@INPUT@', '@OUTPUT@' ])
   trace_ust_all_c = custom_target('trace-ust-all.c',
                                   output: 'trace-ust-all.c',
                                   input: trace_events_files,
-                                  command: [ tracetool, '--group=all', '--format=ust-events-c', '@INPUT@' ],
-                                  capture: true)
+                                  command: [ tracetool, '--group=all', '--format=ust-events-c', '@INPUT@', '@OUTPUT@' ])
   trace_ss.add(trace_ust_all_h, trace_ust_all_c)
   genh += trace_ust_all_h
 endif
-- 
2.26.2

[PATCH 2/4] tracetool: add out_lineno and out_next_lineno to out()

[Stefan Hajnoczi]

Make the output file line number and next line number available to
out().

A later patch will use this to improve error messages.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/tracetool/__init__.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 98104fa50e..e4ee4d5e61 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -31,6 +31,7 @@ def error(*lines):
     sys.exit(1)
 
 
+out_lineno = 1
 out_filename = '<none>'
 out_fobj = sys.stdout
 
@@ -45,12 +46,19 @@ def out(*lines, **kwargs):
     You can use kwargs as a shorthand for mapping variables when formating all
     the strings in lines.
 
-    The 'out_filename' kwarg is automatically added with the output filename.
+    The 'out_lineno' kwarg is automatically added to reflect the current output
+    file line number. The 'out_next_lineno' kwarg is also automatically added
+    with the next output line number. The 'out_filename' kwarg is automatically
+    added with the output filename.
     """
+    global out_lineno
     output = []
     for l in lines:
+        kwargs['out_lineno'] = out_lineno
+        kwargs['out_next_lineno'] = out_lineno + 1
         kwargs['out_filename'] = out_filename
         output.append(l % kwargs)
+        out_lineno += 1
 
     out_fobj.writelines("\n".join(output) + "\n")
 
-- 
2.26.2

[PATCH 3/4] tracetool: add input filename and line number to Event

[Stefan Hajnoczi]

Store the input filename and line number in Event.

A later patch will use this to improve error messages.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/tracetool/__init__.py | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index e4ee4d5e61..1a6e2fa64a 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -218,6 +218,10 @@ class Event(object):
         Properties of the event.
     args : Arguments
         The event arguments.
+    lineno : int
+        The line number in the input file.
+    filename : str
+        The path to the input file.
 
     """
 
@@ -230,7 +234,7 @@ class Event(object):
 
     _VALID_PROPS = set(["disable", "tcg", "tcg-trans", "tcg-exec", "vcpu"])
 
-    def __init__(self, name, props, fmt, args, orig=None,
+    def __init__(self, name, props, fmt, args, lineno, filename, orig=None,
                  event_trans=None, event_exec=None):
         """
         Parameters
@@ -243,6 +247,10 @@ class Event(object):
             Event printing format string(s).
         args : Arguments
             Event arguments.
+        lineno : int
+            The line number in the input file.
+        filename : str
+            The path to the input file.
         orig : Event or None
             Original Event before transformation/generation.
         event_trans : Event or None
@@ -255,6 +263,8 @@ class Event(object):
         self.properties = props
         self.fmt = fmt
         self.args = args
+        self.lineno = int(lineno)
+        self.filename = str(filename)
         self.event_trans = event_trans
         self.event_exec = event_exec
 
@@ -276,16 +286,21 @@ class Event(object):
     def copy(self):
         """Create a new copy."""
         return Event(self.name, list(self.properties), self.fmt,
-                     self.args.copy(), self, self.event_trans, self.event_exec)
+                     self.args.copy(), self.lineno, self.filename,
+                     self, self.event_trans, self.event_exec)
 
     @staticmethod
-    def build(line_str):
+    def build(line_str, lineno, filename):
         """Build an Event instance from a string.
 
         Parameters
         ----------
         line_str : str
             Line describing the event.
+        lineno : int
+            Line number in input file.
+        filename : str
+            Path to input file.
         """
         m = Event._CRE.match(line_str)
         assert m is not None
@@ -315,7 +330,7 @@ class Event(object):
         if "tcg" in props and isinstance(fmt, str):
             raise ValueError("Events with 'tcg' property must have two format strings")
 
-        event = Event(name, props, fmt, args)
+        event = Event(name, props, fmt, args, lineno, filename)
 
         # add implicit arguments when using the 'vcpu' property
         import tracetool.vcpu
@@ -360,6 +375,8 @@ class Event(object):
                      list(self.properties),
                      self.fmt,
                      self.args.transform(*trans),
+                     self.lineno,
+                     self.filename,
                      self)
 
 
@@ -386,7 +403,7 @@ def read_events(fobj, fname):
             continue
 
         try:
-            event = Event.build(line)
+            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:]
-- 
2.26.2

[PATCH 4/4] tracetool: show trace-events filename/lineno in fmt string errors

[Stefan Hajnoczi]

The compiler encounters trace event format strings in generated code.
Format strings are error-prone and therefore clear compiler errors are
important.

Use the #line directive to show the trace-events filename and line
number in format string errors:
https://gcc.gnu.org/onlinedocs/gcc-10.2.0/cpp/Line-Control.html

For example, if the cpu_in trace event's %u is changed to %p the
following error is reported:

  trace-events:29:18: error: format ‘%p’ expects argument of type ‘void *’, but argument 7 has type ‘unsigned int’ [-Werror=format=]

Line 29 in trace-events is where cpu_in is defined. This works for any
trace-events file in the QEMU source tree and the correct path is
displayed.

Unfortunately there does not seem to be a way to set the column, so "18"
is not the right character on that line.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 scripts/tracetool/backend/ftrace.py | 4 ++++
 scripts/tracetool/backend/log.py    | 4 ++++
 scripts/tracetool/backend/syslog.py | 4 ++++
 3 files changed, 12 insertions(+)

diff --git a/scripts/tracetool/backend/ftrace.py b/scripts/tracetool/backend/ftrace.py
index e9844dd335..5fa30ccc08 100644
--- a/scripts/tracetool/backend/ftrace.py
+++ b/scripts/tracetool/backend/ftrace.py
@@ -33,8 +33,10 @@ def generate_h(event, group):
         '        int unused __attribute__ ((unused));',
         '        int trlen;',
         '        if (trace_event_get_state(%(event_id)s)) {',
+        '#line %(event_lineno)d "%(event_filename)s"',
         '            trlen = snprintf(ftrace_buf, MAX_TRACE_STRLEN,',
         '                             "%(name)s " %(fmt)s "\\n" %(argnames)s);',
+        '#line %(out_next_lineno)d "%(out_filename)s"',
         '            trlen = MIN(trlen, MAX_TRACE_STRLEN - 1);',
         '            unused = write(trace_marker_fd, ftrace_buf, trlen);',
         '        }',
@@ -42,6 +44,8 @@ def generate_h(event, group):
         name=event.name,
         args=event.args,
         event_id="TRACE_" + event.name.upper(),
+        event_lineno=event.lineno,
+        event_filename=event.filename,
         fmt=event.fmt.rstrip("\n"),
         argnames=argnames)
 
diff --git a/scripts/tracetool/backend/log.py b/scripts/tracetool/backend/log.py
index 877222bbe9..bc43dbb4f4 100644
--- a/scripts/tracetool/backend/log.py
+++ b/scripts/tracetool/backend/log.py
@@ -37,12 +37,16 @@ def generate_h(event, group):
     out('    if (%(cond)s && qemu_loglevel_mask(LOG_TRACE)) {',
         '        struct timeval _now;',
         '        gettimeofday(&_now, NULL);',
+        '#line %(event_lineno)d "%(event_filename)s"',
         '        qemu_log("%%d@%%zu.%%06zu:%(name)s " %(fmt)s "\\n",',
         '                 qemu_get_thread_id(),',
         '                 (size_t)_now.tv_sec, (size_t)_now.tv_usec',
         '                 %(argnames)s);',
+        '#line %(out_next_lineno)d "%(out_filename)s"',
         '    }',
         cond=cond,
+        event_lineno=event.lineno,
+        event_filename=event.filename,
         name=event.name,
         fmt=event.fmt.rstrip("\n"),
         argnames=argnames)
diff --git a/scripts/tracetool/backend/syslog.py b/scripts/tracetool/backend/syslog.py
index 1373a90192..5a3a00fe31 100644
--- a/scripts/tracetool/backend/syslog.py
+++ b/scripts/tracetool/backend/syslog.py
@@ -35,9 +35,13 @@ def generate_h(event, group):
         cond = "trace_event_get_state(%s)" % ("TRACE_" + event.name.upper())
 
     out('    if (%(cond)s) {',
+        '#line %(event_lineno)d "%(event_filename)s"',
         '        syslog(LOG_INFO, "%(name)s " %(fmt)s %(argnames)s);',
+        '#line %(out_next_lineno)d "%(out_filename)s"',
         '    }',
         cond=cond,
+        event_lineno=event.lineno,
+        event_filename=event.filename,
         name=event.name,
         fmt=event.fmt.rstrip("\n"),
         argnames=argnames)
-- 
2.26.2

Re: [PATCH 4/4] tracetool: show trace-events filename/lineno in fmt string errors

[Peter Maydell]

On Thu, 27 Aug 2020 at 15:29, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> The compiler encounters trace event format strings in generated code.
> Format strings are error-prone and therefore clear compiler errors are
> important.
>
> Use the #line directive to show the trace-events filename and line
> number in format string errors:
> https://gcc.gnu.org/onlinedocs/gcc-10.2.0/cpp/Line-Control.html
>
> For example, if the cpu_in trace event's %u is changed to %p the
> following error is reported:
>
>   trace-events:29:18: error: format ‘%p’ expects argument of type ‘void *’, but argument 7 has type ‘unsigned int’ [-Werror=format=]
>
> Line 29 in trace-events is where cpu_in is defined. This works for any
> trace-events file in the QEMU source tree and the correct path is
> displayed.
>
> Unfortunately there does not seem to be a way to set the column, so "18"
> is not the right character on that line.

It's been pointed out to me that you could do this by
making the generated code have suitable line breaks, padding,
etc, so that the format string in the output ends up starting in
the same column it was in the input trace file. Whether this is
worthwhile I leave up to you :-)

(The argument number (7 in your example) is also of course off,
and that I think we're also stuck with. Getting the file and line
number right is a solid improvement on the current situation.)

thanks
-- PMM

Re: [PATCH 4/4] tracetool: show trace-events filename/lineno in fmt string errors

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]

On Thu, Aug 27, 2020 at 03:59:04PM +0100, Peter Maydell wrote:
> On Thu, 27 Aug 2020 at 15:29, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > The compiler encounters trace event format strings in generated code.
> > Format strings are error-prone and therefore clear compiler errors are
> > important.
> >
> > Use the #line directive to show the trace-events filename and line
> > number in format string errors:
> > https://gcc.gnu.org/onlinedocs/gcc-10.2.0/cpp/Line-Control.html
> >
> > For example, if the cpu_in trace event's %u is changed to %p the
> > following error is reported:
> >
> >   trace-events:29:18: error: format ‘%p’ expects argument of type ‘void *’, but argument 7 has type ‘unsigned int’ [-Werror=format=]
> >
> > Line 29 in trace-events is where cpu_in is defined. This works for any
> > trace-events file in the QEMU source tree and the correct path is
> > displayed.
> >
> > Unfortunately there does not seem to be a way to set the column, so "18"
> > is not the right character on that line.
> 
> It's been pointed out to me that you could do this by
> making the generated code have suitable line breaks, padding,
> etc, so that the format string in the output ends up starting in
> the same column it was in the input trace file. Whether this is
> worthwhile I leave up to you :-)
> 
> (The argument number (7 in your example) is also of course off,
> and that I think we're also stuck with. Getting the file and line
> number right is a solid improvement on the current situation.)

Thanks for mentioning that trick. I will leave the patch series as-is
for now.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] tracetool: add output filename command-line argument

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 9491 bytes --]

Le jeu. 27 août 2020 16:30, Stefan Hajnoczi <stefanha@redhat.com> a écrit :

> The tracetool.py script writes to stdout. This means the output filename
> is not available to the script. Add the output filename to the
> command-line so that the script has access to the filename.
>
> This also simplifies the tracetool.py invocation. It's no longer
> necessary to use meson's custom_build(capture : true) to save output.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/devel/tracing.txt        |  3 ++-
>  meson.build                   |  3 +--
>  scripts/tracetool.py          | 12 +++++++-----
>  scripts/tracetool/__init__.py | 18 ++++++++++++++++--
>  trace/meson.build             | 23 ++++++++---------------
>  5 files changed, 34 insertions(+), 25 deletions(-)
>
> diff --git a/docs/devel/tracing.txt b/docs/devel/tracing.txt
> index 6144d9921b..c84d4c00ac 100644
> --- a/docs/devel/tracing.txt
> +++ b/docs/devel/tracing.txt
> @@ -318,7 +318,8 @@ probes:
>                           --target-type system \
>                           --target-name x86_64 \
>                           --group=all \
> -                         trace-events-all >qemu.stp
> +                         trace-events-all \
> +                         qemu.stp
>
>  To facilitate simple usage of systemtap where there merely needs to be
> printf
>  logging of certain probes, a helper script "qemu-trace-stap" is provided.
> diff --git a/meson.build b/meson.build
> index f0fe5f8799..fadeb0c268 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1037,7 +1037,6 @@ foreach target : target_dirs
>          custom_target(exe_name + stp['ext'],
>                        input: trace_events_all,
>                        output: exe_name + stp['ext'],
> -                      capture: true,
>                        install: stp['install'],
>                        install_dir: config_host['qemu_datadir'] /
> '../systemtap/tapset',
>                        command: [
> @@ -1046,7 +1045,7 @@ foreach target : target_dirs
>                          '--target-name=' + target_name,
>                          '--target-type=' + target_type,
>                          '--probe-prefix=qemu.' + target_type + '.' +
> target_name,
> -                        '@INPUT@',
> +                        '@INPUT@', '@OUTPUT@'
>                        ])
>        endforeach
>      endif
> diff --git a/scripts/tracetool.py b/scripts/tracetool.py
> index 31146242b7..ab7653a5ce 100644
> --- a/scripts/tracetool.py
> +++ b/scripts/tracetool.py
> @@ -16,7 +16,7 @@ __email__      = "stefanha@redhat.com"
>  import sys
>  import getopt
>
> -from tracetool import error_write, out
> +from tracetool import error_write, out, out_open
>  import tracetool.backend
>  import tracetool.format
>
> @@ -32,7 +32,7 @@ def error_opt(msg = None):
>      format_descr = "\n".join([ "    %-15s %s" % (n, d)
>                                 for n,d in tracetool.format.get_list() ])
>      error_write("""\
> -Usage: %(script)s --format=<format> --backends=<backends> [<options>]
> +Usage: %(script)s --format=<format> --backends=<backends> [<options>]
> <trace-events> ... <output>
>
>  Backends:
>  %(backends)s
> @@ -135,13 +135,15 @@ def main(args):
>          if probe_prefix is None:
>              probe_prefix = ".".join(["qemu", target_type, target_name])
>
> -    if len(args) < 1:
> -        error_opt("missing trace-events filepath")
> +    if len(args) < 2:
> +        error_opt("missing trace-events and output filepaths")
>      events = []
> -    for arg in args:
> +    for arg in args[:-1]:
>          with open(arg, "r") as fh:
>              events.extend(tracetool.read_events(fh, arg))
>
> +    out_open(args[-1])
> +
>      try:
>          tracetool.generate(events, arg_group, arg_format, arg_backends,
>                             binary=binary, probe_prefix=probe_prefix)
> diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
> index 3ccfa1e116..98104fa50e 100644
> --- a/scripts/tracetool/__init__.py
> +++ b/scripts/tracetool/__init__.py
> @@ -31,14 +31,28 @@ def error(*lines):
>      sys.exit(1)
>
>
> +out_filename = '<none>'
> +out_fobj = sys.stdout
>

These appear to be always overwritten (is initialization useful?)

Anyway:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

+
> +def out_open(filename):
> +    global out_filename, out_fobj
> +    out_filename = filename
> +    out_fobj = open(filename, 'wt')
> +
>  def out(*lines, **kwargs):
>      """Write a set of output lines.
>
>      You can use kwargs as a shorthand for mapping variables when
> formating all
>      the strings in lines.
> +
> +    The 'out_filename' kwarg is automatically added with the output
> filename.
>      """
> -    lines = [ l % kwargs for l in lines ]
> -    sys.stdout.writelines("\n".join(lines) + "\n")
> +    output = []
> +    for l in lines:
> +        kwargs['out_filename'] = out_filename
> +        output.append(l % kwargs)
> +
> +    out_fobj.writelines("\n".join(output) + "\n")
>
>  # We only want to allow standard C types or fixed sized
>  # integer types. We don't want QEMU specific types
> diff --git a/trace/meson.build b/trace/meson.build
> index 56e870848e..16eccc0c22 100644
> --- a/trace/meson.build
> +++ b/trace/meson.build
> @@ -11,20 +11,17 @@ foreach dir : [ '.' ] + trace_events_subdirs
>    trace_h = custom_target(fmt.format('trace', 'h'),
>                            output: fmt.format('trace', 'h'),
>                            input: trace_events_file,
> -                          command: [ tracetool, group, '--format=h',
> '@INPUT@' ],
> -                          capture: true)
> +                          command: [ tracetool, group, '--format=h',
> '@INPUT@', '@OUTPUT@' ])
>    genh += trace_h
>    trace_c = custom_target(fmt.format('trace', 'c'),
>                            output: fmt.format('trace', 'c'),
>                            input: trace_events_file,
> -                          command: [ tracetool, group, '--format=c',
> '@INPUT@' ],
> -                          capture: true)
> +                          command: [ tracetool, group, '--format=c',
> '@INPUT@', '@OUTPUT@' ])
>    if 'CONFIG_TRACE_UST' in config_host
>      trace_ust_h = custom_target(fmt.format('trace-ust', 'h'),
>                                  output: fmt.format('trace-ust', 'h'),
>                                  input: trace_events_file,
> -                                command: [ tracetool, group,
> '--format=ust-events-h', '@INPUT@' ],
> -                                capture: true)
> +                                command: [ tracetool, group,
> '--format=ust-events-h', '@INPUT@', '@OUTPUT@' ])
>      trace_ss.add(trace_ust_h, lttng, urcubp)
>      genh += trace_ust_h
>    endif
> @@ -33,8 +30,7 @@ foreach dir : [ '.' ] + trace_events_subdirs
>      trace_dtrace = custom_target(fmt.format('trace-dtrace', 'dtrace'),
>                                   output: fmt.format('trace-dtrace',
> 'dtrace'),
>                                   input: trace_events_file,
> -                                 command: [ tracetool, group,
> '--format=d', '@INPUT@' ],
> -                                 capture: true)
> +                                 command: [ tracetool, group,
> '--format=d', '@INPUT@', '@OUTPUT@' ])
>      trace_dtrace_h = custom_target(fmt.format('trace-dtrace', 'h'),
>                                     output: fmt.format('trace-dtrace',
> 'h'),
>                                     input: trace_dtrace,
> @@ -66,9 +62,8 @@ foreach d : [
>    gen = custom_target(d[0],
>                  output: d[0],
>                  input: meson.source_root() / 'trace-events',
> -                command: [ tracetool, '--group=root', '--format=@0@'.format(d[1]),
> '@INPUT@' ],
> -                build_by_default: true, # to be removed when added to a
> target
> -                capture: true)
> +                command: [ tracetool, '--group=root', '--format=@0@'.format(d[1]),
> '@INPUT@', '@OUTPUT@' ],
> +                build_by_default: true) # to be removed when added to a
> target
>    specific_ss.add(gen)
>  endforeach
>
> @@ -76,13 +71,11 @@ if 'CONFIG_TRACE_UST' in config_host
>    trace_ust_all_h = custom_target('trace-ust-all.h',
>                                    output: 'trace-ust-all.h',
>                                    input: trace_events_files,
> -                                  command: [ tracetool, '--group=all',
> '--format=ust-events-h', '@INPUT@' ],
> -                                  capture: true)
> +                                  command: [ tracetool, '--group=all',
> '--format=ust-events-h', '@INPUT@', '@OUTPUT@' ])
>    trace_ust_all_c = custom_target('trace-ust-all.c',
>                                    output: 'trace-ust-all.c',
>                                    input: trace_events_files,
> -                                  command: [ tracetool, '--group=all',
> '--format=ust-events-c', '@INPUT@' ],
> -                                  capture: true)
> +                                  command: [ tracetool, '--group=all',
> '--format=ust-events-c', '@INPUT@', '@OUTPUT@' ])
>    trace_ss.add(trace_ust_all_h, trace_ust_all_c)
>    genh += trace_ust_all_h
>  endif
> --
> 2.26.2
>
>

[-- Attachment #2: Type: text/html, Size: 12845 bytes --]

Re: [PATCH 3/4] tracetool: add input filename and line number to Event

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 4036 bytes --]

Le jeu. 27 août 2020 16:32, Stefan Hajnoczi <stefanha@redhat.com> a écrit :

> Store the input filename and line number in Event.
>
> A later patch will use this to improve error messages.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

---
>  scripts/tracetool/__init__.py | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
> index e4ee4d5e61..1a6e2fa64a 100644
> --- a/scripts/tracetool/__init__.py
> +++ b/scripts/tracetool/__init__.py
> @@ -218,6 +218,10 @@ class Event(object):
>          Properties of the event.
>      args : Arguments
>          The event arguments.
> +    lineno : int
> +        The line number in the input file.
> +    filename : str
> +        The path to the input file.
>
>      """
>
> @@ -230,7 +234,7 @@ class Event(object):
>
>      _VALID_PROPS = set(["disable", "tcg", "tcg-trans", "tcg-exec",
> "vcpu"])
>
> -    def __init__(self, name, props, fmt, args, orig=None,
> +    def __init__(self, name, props, fmt, args, lineno, filename,
> orig=None,
>                   event_trans=None, event_exec=None):
>          """
>          Parameters
> @@ -243,6 +247,10 @@ class Event(object):
>              Event printing format string(s).
>          args : Arguments
>              Event arguments.
> +        lineno : int
> +            The line number in the input file.
> +        filename : str
> +            The path to the input file.
>          orig : Event or None
>              Original Event before transformation/generation.
>          event_trans : Event or None
> @@ -255,6 +263,8 @@ class Event(object):
>          self.properties = props
>          self.fmt = fmt
>          self.args = args
> +        self.lineno = int(lineno)
> +        self.filename = str(filename)
>          self.event_trans = event_trans
>          self.event_exec = event_exec
>
> @@ -276,16 +286,21 @@ class Event(object):
>      def copy(self):
>          """Create a new copy."""
>          return Event(self.name, list(self.properties), self.fmt,
> -                     self.args.copy(), self, self.event_trans,
> self.event_exec)
> +                     self.args.copy(), self.lineno, self.filename,
> +                     self, self.event_trans, self.event_exec)
>
>      @staticmethod
> -    def build(line_str):
> +    def build(line_str, lineno, filename):
>          """Build an Event instance from a string.
>
>          Parameters
>          ----------
>          line_str : str
>              Line describing the event.
> +        lineno : int
> +            Line number in input file.
> +        filename : str
> +            Path to input file.
>          """
>          m = Event._CRE.match(line_str)
>          assert m is not None
> @@ -315,7 +330,7 @@ class Event(object):
>          if "tcg" in props and isinstance(fmt, str):
>              raise ValueError("Events with 'tcg' property must have two
> format strings")
>
> -        event = Event(name, props, fmt, args)
> +        event = Event(name, props, fmt, args, lineno, filename)
>
>          # add implicit arguments when using the 'vcpu' property
>          import tracetool.vcpu
> @@ -360,6 +375,8 @@ class Event(object):
>                       list(self.properties),
>                       self.fmt,
>                       self.args.transform(*trans),
> +                     self.lineno,
> +                     self.filename,
>                       self)
>
>
> @@ -386,7 +403,7 @@ def read_events(fobj, fname):
>              continue
>
>          try:
> -            event = Event.build(line)
> +            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:]
> --
> 2.26.2
>
>

[-- Attachment #2: Type: text/html, Size: 5768 bytes --]

Re: [PATCH 2/4] tracetool: add out_lineno and out_next_lineno to out()

[Peter Maydell]

On Thu, 27 Aug 2020 at 15:29, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> Make the output file line number and next line number available to
> out().
>
> A later patch will use this to improve error messages.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 3/4] tracetool: add input filename and line number to Event

[Peter Maydell]

On Thu, 27 Aug 2020 at 15:29, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> Store the input filename and line number in Event.
>
> A later patch will use this to improve error messages.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 4/4] tracetool: show trace-events filename/lineno in fmt string errors

[Peter Maydell]

On Thu, 27 Aug 2020 at 15:29, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> The compiler encounters trace event format strings in generated code.
> Format strings are error-prone and therefore clear compiler errors are
> important.
>
> Use the #line directive to show the trace-events filename and line
> number in format string errors:
> https://gcc.gnu.org/onlinedocs/gcc-10.2.0/cpp/Line-Control.html
>
> For example, if the cpu_in trace event's %u is changed to %p the
> following error is reported:
>
>   trace-events:29:18: error: format ‘%p’ expects argument of type ‘void *’, but argument 7 has type ‘unsigned int’ [-Werror=format=]
>
> Line 29 in trace-events is where cpu_in is defined. This works for any
> trace-events file in the QEMU source tree and the correct path is
> displayed.
>
> Unfortunately there does not seem to be a way to set the column, so "18"
> is not the right character on that line.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 1/4] tracetool: add output filename command-line argument

[Peter Maydell]

On Thu, 27 Aug 2020 at 15:29, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> The tracetool.py script writes to stdout. This means the output filename
> is not available to the script. Add the output filename to the
> command-line so that the script has access to the filename.
>
> This also simplifies the tracetool.py invocation. It's no longer
> necessary to use meson's custom_build(capture : true) to save output.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 0/4] tracetool: show trace-events filename/lineno in fmt string errors

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

On Thu, Aug 27, 2020 at 03:29:11PM +0100, Stefan Hajnoczi wrote:
> This patch series improves format string compiler error messages. Instead of
> showing the generated file, show the trace-events file where the format string
> is defined.
> 
> Stefan Hajnoczi (4):
>   tracetool: add output filename command-line argument
>   tracetool: add out_lineno and out_next_lineno to out()
>   tracetool: add input filename and line number to Event
>   tracetool: show trace-events filename/lineno in fmt string errors
> 
>  docs/devel/tracing.txt              |  3 +-
>  meson.build                         |  3 +-
>  scripts/tracetool.py                | 12 ++++---
>  scripts/tracetool/__init__.py       | 53 +++++++++++++++++++++++++----
>  scripts/tracetool/backend/ftrace.py |  4 +++
>  scripts/tracetool/backend/log.py    |  4 +++
>  scripts/tracetool/backend/syslog.py |  4 +++
>  trace/meson.build                   | 23 +++++--------
>  8 files changed, 76 insertions(+), 30 deletions(-)
> 
> -- 
> 2.26.2
> 

Thanks, applied to my tracing-next tree:
https://gitlab.com/stefanha/qemu/commits/tracing-next

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]