[Qemu-devel] Re: [RFC v2][PATCH][Tracing] Fix build errors for target i386-linux-user

[Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>]

On Tue, Jul 06, 2010 at 01:34:17PM +0530, Prerna Saxena wrote:
> On Thu, 1 Jul 2010 10:18:41 +0100
> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
> 
> > On Wed, Jun 30, 2010 at 09:11:45PM +0530, Prerna Saxena wrote:
> > > [PATCH 1/1] Move definitions of monitor command handlers (do_info_trace, 
> > > do_info_all_trace_events) to monitor.c. This removes build errors for 
> > > user targets such as i386-linux-user, which are not linked with monitor.
> > > 
> > > The export of trace_buf and trace_idx is an unfortunate side effect, 
> > > since these are needed by do_info_trace which dumps buffer 
> > > contents.
> > 
> > "git grep monitor_printf" suggests that other source files do their own
> > monitor printing.  Did you look into alternatives that don't expose the
> > trace buffer internals?
> 
> $grep "monitor_printf" would show this function being used in lot of
> places, but all of those include "monitor.h" and are linked to the
> monitor object as a part of the build process. I could not find any other 
> definition of this function apart from the one in "monitor.c", and a 
> dummy definition in qemu_tool.c.
> 
> > 
> > I feel #ifndef CONFIG_USER_ONLY in simpletrace.c would be would be nicer
> > than exposing the internals.  (CONFIG_USER_ONLY is just a guess, I
> > haven't investigated which is the right thing to test for.)
> > 
> > Stefan
> > 
> 
> Yes, I did explore some alternatives.
> 
> The monitor command handlers for tracing need knowledge of buffer internals.
> The top-level trace object gets compiled earlier, and all target objects
> include this later at link time. None of the user targets (linux-user,
> bsd-user, darwin-user) are linked with monitor.o, and consequently the
> trace object when linked with these targets still has dangling
> references to monitor commands. This flags the linker errors.
> 
> Even if I were to enclose the monitor-specific interfaces with a
> #ifdef...#endif , it would allow the trace-object to be built
> homogenously for all targets -- either with or without the code blocks
> enclosed in #ifdef..#endif. I am not being able to see how we can
> utilize CONFIG_USER_ONLY here.
> 
> There are 3 ways I could think of, to fix this :
> 
> a) Do not link the trace object for user targets, that don't link with
> monitor.o
> Advantage : errors disappear.
> Drawback : tracing not available for user targets.
> 
> b) Keep the trace buffer definitions intact. In places where the linker
> doesn't find the monitor references, put a dummy file that has empty
> definitions for these monitor interfaces, and link the target object
> with these to remove the dangling references.
> Advantage : trace buffer internals are safeguarded.
> Drawback : Code & build process gets complicated ; not a clean approach.
> 
> c) Separate the trace buffer functions and the monitor interfaces. In
> user targets, only the trace object is included which allows
> trace-events to be built for these targets as well. Just that, the
> monitor support is not available.
> Advantage : Clean separation of monitor commands and basic trace
> functionality.
> Drawback : Trace buffer internals get exposed.
> 
> My earlier patch took approach 'c', the one below takes approach 'b'.
> This patch flags a compile warning :
> 
> exec.o: warning: multiple common of `logfile'
> qemu-tool.o: warning: previous common is here
> 
> I'm running short of ideas to fix this. Any suggestions would be very 
> helpful.
> 
> [PATCH] User targets dont include monitor.o. Fix to allow compilation of 
> trace object for these.
> 
> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> ---
>  Makefile.target |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Makefile.target b/Makefile.target
> index 493233a..b05e37e 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -86,7 +86,7 @@ $(call set-vpath, $(SRC_PATH)/linux-user:$(SRC_PATH)/linux-user/$(TARGET_ABI_DIR
>  QEMU_CFLAGS+=-I$(SRC_PATH)/linux-user -I$(SRC_PATH)/linux-user/$(TARGET_ABI_DIR)
>  obj-y = main.o syscall.o strace.o mmap.o signal.o thunk.o \
>        elfload.o linuxload.o uaccess.o gdbstub.o cpu-uname.o \
> -      qemu-malloc.o
> +      qemu-malloc.o qemu-tool.o

qemu-tool.o provides stub implementations of monitor_printf() and other
functions which are not available in qemu-io, qemu-image, and qemu-nbd.

logfile is defined in exec.c, which is not linked into qemu-io but is
linked in for QEMU user targets.  This is the cause of the duplicate
symbol.

I think this approach would work if qemu-tool.c was split into tool
stubs and user stubs.

An alternative to modifying the build is to define an interface for
accessing the "simple" backend trace buffer, e.g.:

/**
 * Pretty-print a trace buffer record
 *
 * @param index     Trace buffer index
 * @return str      Record string (heap allocated) or NULL
 *
 * The caller must free the returned string.  If the index is beyond the
 * last valid trace buffer record, NULL is returned.
 */
char *simpletrace_format_record(unsigned int index)

Then the code in monitor.c can just use simpletrace_format_record() in a
loop to extract formatted trace records.  It doesn't need to know about
trace buffer internals.

Perhaps this is the simplest solution?

> 
>  obj-$(TARGET_HAS_BFLT) += flatload.o
> 
> @@ -124,7 +124,7 @@ LDFLAGS+=-Wl,-segaddr,__STD_PROG_ZONE,0x1000 -image_base 0x0e000000
>  LIBS+=-lmx
> 
>  obj-y = main.o commpage.o machload.o mmap.o signal.o syscall.o thunk.o \
> -        gdbstub.o
> +        gdbstub.o qemu-tool.o
> 
>  obj-i386-y += ioport-user.o
> 
> @@ -146,7 +146,7 @@ $(call set-vpath, $(SRC_PATH)/bsd-user)
>  QEMU_CFLAGS+=-I$(SRC_PATH)/bsd-user -I$(SRC_PATH)/bsd-user/$(TARGET_ARCH)
> 
>  obj-y = main.o bsdload.o elfload.o mmap.o signal.o strace.o syscall.o \
> -        gdbstub.o uaccess.o
> +        gdbstub.o uaccess.o qemu-tool.o
> 
>  obj-i386-y += ioport-user.o
> 
> -- 
> 
> 
> -- 
> Prerna Saxena
> 
> Linux Technology Centre,
> IBM Systems and Technology Lab,
> Bangalore, India
> 

Stefan