On Tue, Jul 06, 2010 at 01:34:17PM +0530, Prerna Saxena wrote: > On Thu, 1 Jul 2010 10:18:41 +0100 > Stefan Hajnoczi <stefa...@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 <pre...@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
