| Message ID | 20200827142915.108730-5-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | tracetool: show trace-events filename/lineno in fmt string errors | expand |
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
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
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
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)
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(+)