| Message ID | 2b9cb5f725ded34557f9ab2aff9c5d9dd34c608b.1389783548.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
Ping! Do reverts require the usual reviewed-by? Markus are you happy with this mini series to address your error_report() concerns? Regards, Peter On Wed, Jan 15, 2014 at 9:06 PM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > This reverts commit d32934c84c72f57e78d430c22974677b7bcabe5d. > > The original implementation before this patch makes abortive error > messages much more friendly. The underlying bug that required this > change is now fixed. Revert. > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > --- > > util/error.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/util/error.c b/util/error.c > index e5de34f..f11f1d5 100644 > --- a/util/error.c > +++ b/util/error.c > @@ -44,7 +44,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...) > err->err_class = err_class; > > if (errp == &error_abort) { > - fprintf(stderr, "%s\n", error_get_pretty(err)); > + error_report("%s", error_get_pretty(err)); > abort(); > } > > @@ -80,7 +80,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class, > err->err_class = err_class; > > if (errp == &error_abort) { > - fprintf(stderr, "%s\n", error_get_pretty(err)); > + error_report("%s", error_get_pretty(err)); > abort(); > } > > @@ -125,7 +125,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class, > err->err_class = err_class; > > if (errp == &error_abort) { > - fprintf(stderr, "%s\n", error_get_pretty(err)); > + error_report("%s", error_get_pretty(err)); > abort(); > } > > @@ -171,7 +171,7 @@ void error_free(Error *err) > void error_propagate(Error **dst_err, Error *local_err) > { > if (local_err && dst_err == &error_abort) { > - fprintf(stderr, "%s\n", error_get_pretty(local_err)); > + error_report("%s", error_get_pretty(local_err)); > abort(); > } else if (dst_err && !*dst_err) { > *dst_err = local_err; > -- > 1.8.5.3 >
Hi, I committed it last week, forgot to email you. Cheers, Edgar On Tue, Jan 28, 2014 at 12:35:08PM +1000, Peter Crosthwaite wrote: > Ping! > > Do reverts require the usual reviewed-by? Markus are you happy with > this mini series to address your error_report() concerns? > > Regards, > Peter > > On Wed, Jan 15, 2014 at 9:06 PM, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: > > This reverts commit d32934c84c72f57e78d430c22974677b7bcabe5d. > > > > The original implementation before this patch makes abortive error > > messages much more friendly. The underlying bug that required this > > change is now fixed. Revert. > > > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > --- > > > > util/error.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/util/error.c b/util/error.c > > index e5de34f..f11f1d5 100644 > > --- a/util/error.c > > +++ b/util/error.c > > @@ -44,7 +44,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...) > > err->err_class = err_class; > > > > if (errp == &error_abort) { > > - fprintf(stderr, "%s\n", error_get_pretty(err)); > > + error_report("%s", error_get_pretty(err)); > > abort(); > > } > > > > @@ -80,7 +80,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class, > > err->err_class = err_class; > > > > if (errp == &error_abort) { > > - fprintf(stderr, "%s\n", error_get_pretty(err)); > > + error_report("%s", error_get_pretty(err)); > > abort(); > > } > > > > @@ -125,7 +125,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class, > > err->err_class = err_class; > > > > if (errp == &error_abort) { > > - fprintf(stderr, "%s\n", error_get_pretty(err)); > > + error_report("%s", error_get_pretty(err)); > > abort(); > > } > > > > @@ -171,7 +171,7 @@ void error_free(Error *err) > > void error_propagate(Error **dst_err, Error *local_err) > > { > > if (local_err && dst_err == &error_abort) { > > - fprintf(stderr, "%s\n", error_get_pretty(local_err)); > > + error_report("%s", error_get_pretty(local_err)); > > abort(); > > } else if (dst_err && !*dst_err) { > > *dst_err = local_err; > > -- > > 1.8.5.3 > > >
Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > Ping! > > Do reverts require the usual reviewed-by? Markus are you happy with > this mini series to address your error_report() concerns? Yes, I'm happy with it. Thank you!
On 15 January 2014 11:06, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > This reverts commit d32934c84c72f57e78d430c22974677b7bcabe5d. > > The original implementation before this patch makes abortive error > messages much more friendly. The underlying bug that required this > change is now fixed. Revert. Unfortunately 'make check' still doesn't work on MacOS: cc -m64 -DOS_OBJECT_USE_OBJC=0 -arch x86_64 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -Wno-initializer-overrides -Wendif-labels -Wmissing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-definition -Wtype-limits -fstack-protector-all -I/sw/include -I/sw/include/libpng15 -I/sw/include -I/opt/X11/include/pixman-1 -I/Users/pm215/src/qemu/dtc/libfdt -I/sw/include/glib-2.0 -I/sw/lib/glib-2.0/include -I/Users/pm215/src/qemu/tests -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g -m64 -framework CoreFoundation -framework IOKit -arch x86_64 -g -o tests/check-qjson tests/check-qjson.o libqemuutil.a libqemustub.a -L/sw/lib -lgthread-2.0 -lglib-2.0 -lintl -lz -L/sw/lib -lssh2 -L/sw/lib -lcurl Undefined symbols for architecture x86_64: "_cur_mon", referenced from: _error_vprintf in libqemuutil.a(qemu-error.o) _error_printf in libqemuutil.a(qemu-error.o) _error_printf_unless_qmp in libqemuutil.a(qemu-error.o) _error_print_loc in libqemuutil.a(qemu-error.o) _error_report in libqemuutil.a(qemu-error.o) ld: symbol(s) not found for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation) make: *** [tests/check-qjson] Error 1 Curiously, if you edit the cc command line so it says not just "libqemustub.a" but "libqemustub.a stubs/mon-set-error.o" then it links successfully. nm says that that .o file is in the .a: libqemustub.a(mon-set-error.o): 0000000000000a68 s EH_frame0 U ___stack_chk_fail U ___stack_chk_guard 0000000000000008 C _cur_mon 0000000000000000 T _monitor_set_error 0000000000000a80 S _monitor_set_error.eh This appears to be a deficiency in the MacOSX linker and/or executable format: http://stackoverflow.com/questions/19398742/os-x-linker-unable-to-find-symbols-from-a-c-file-which-only-contains-variables To fix this we need to: * make the cur_mon in the stub-file not common (eg by initializing it) * make the cur_mon in monitor.c not common (because otherwise the linker will prefer to pull in the version from the stub file, which then causes a clash between the stub monitor_set_error() function and the real one) Which is kind of ugly. The other approach would be to ensure that the monitor-related stubs are combined into fewer stub .c files, such that any binary that needs cur_mon will pull in the stub .o file because it needed some function from it anyway. Related questions: why does util/qemu-error.c guard its use of cur_mon only by "if (cur_mon)" whereas qobject/qerror.c uses "if (monitor_cur_is_qmp())"? why, given that qerror.c has made a specific decision not to send its output to the monitor, is it ok for error_report and error_vprintf to override it and send the output to the monitor anyway? if error_vprintf is correct should we remove the higher level check? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 15 January 2014 11:06, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> This reverts commit d32934c84c72f57e78d430c22974677b7bcabe5d. >> >> The original implementation before this patch makes abortive error >> messages much more friendly. The underlying bug that required this >> change is now fixed. Revert. > > Unfortunately 'make check' still doesn't work on MacOS: > > cc -m64 -DOS_OBJECT_USE_OBJC=0 -arch x86_64 -D_GNU_SOURCE > -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes > -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes > -fno-strict-aliasing -Wno-initializer-overrides -Wendif-labels > -Wmissing-include-dirs -Wempty-body -Wnested-externs -Wformat-security > -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-definition > -Wtype-limits -fstack-protector-all -I/sw/include > -I/sw/include/libpng15 -I/sw/include -I/opt/X11/include/pixman-1 > -I/Users/pm215/src/qemu/dtc/libfdt -I/sw/include/glib-2.0 > -I/sw/lib/glib-2.0/include -I/Users/pm215/src/qemu/tests -O2 > -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g -m64 -framework > CoreFoundation -framework IOKit -arch x86_64 -g -o tests/check-qjson > tests/check-qjson.o libqemuutil.a libqemustub.a -L/sw/lib > -lgthread-2.0 -lglib-2.0 -lintl -lz -L/sw/lib -lssh2 -L/sw/lib -lcurl > Undefined symbols for architecture x86_64: > "_cur_mon", referenced from: > _error_vprintf in libqemuutil.a(qemu-error.o) > _error_printf in libqemuutil.a(qemu-error.o) > _error_printf_unless_qmp in libqemuutil.a(qemu-error.o) > _error_print_loc in libqemuutil.a(qemu-error.o) > _error_report in libqemuutil.a(qemu-error.o) > ld: symbol(s) not found for architecture x86_64 > clang: error: linker command failed with exit code 1 (use -v to see invocation) > make: *** [tests/check-qjson] Error 1 > > > Curiously, if you edit the cc command line so it says not just > "libqemustub.a" but "libqemustub.a stubs/mon-set-error.o" then > it links successfully. nm says that that .o file is in the .a: > > libqemustub.a(mon-set-error.o): > 0000000000000a68 s EH_frame0 > U ___stack_chk_fail > U ___stack_chk_guard > 0000000000000008 C _cur_mon > 0000000000000000 T _monitor_set_error > 0000000000000a80 S _monitor_set_error.eh > > This appears to be a deficiency in the MacOSX linker and/or > executable format: > > http://stackoverflow.com/questions/19398742/os-x-linker-unable-to-find-symbols-from-a-c-file-which-only-contains-variables > > To fix this we need to: > * make the cur_mon in the stub-file not common (eg by initializing it) > * make the cur_mon in monitor.c not common (because otherwise > the linker will prefer to pull in the version from the stub file, which > then causes a clash between the stub monitor_set_error() function > and the real one) > > Which is kind of ugly. Not ugly, but a sensible move (in my opinion) regardless of this specific issue: compile with -fno-common. Then both become not common. `-fno-common' In C code, controls the placement of uninitialized global variables. Unix C compilers have traditionally permitted multiple definitions of such variables in different compilation units by placing the variables in a common block. This is the behavior specified by `-fcommon', and is the default for GCC on most targets. On the other hand, this behavior is not required by ISO C, and on some targets may carry a speed or code size penalty on variable references. The `-fno-common' option specifies that the compiler should place uninitialized global variables in the data section of the object file, rather than generating them as common blocks. This has the effect that if the same variable is declared (without `extern') in two different compilations, you will get a multiple-definition error when you link them. In this case, you must compile with `-fcommon' instead. Compiling with `-fno-common' is useful on targets for which it provides better performance, or if you wish to verify that the program will work on other systems that always treat uninitialized variable declarations this way. > The other approach would be to ensure that the monitor-related > stubs are combined into fewer stub .c files, such that any binary > that needs cur_mon will pull in the stub .o file because it needed > some function from it anyway. That may make sense regardless. > Related questions: > > why does util/qemu-error.c guard its use of cur_mon only by > "if (cur_mon)" whereas qobject/qerror.c uses "if (monitor_cur_is_qmp())"? > > why, given that qerror.c has made a specific decision not to > send its output to the monitor, is it ok for error_report and > error_vprintf to override it and send the output to the monitor > anyway? if error_vprintf is correct should we remove the > higher level check? Our prolongued thrashings in the error infrastructure quicksand have resulted in a bit of a mess, I'm afraid. We want to print human-readable messages to stderr in non-monitor context, to the current monitor in HMP context, and not at all in QMP context. Why the latter? Isn't an attempt to print to QMP simply a bug? Yes, it is. However, when you call existing code from a QMP command handler, finding every print that might reach is hard. Hard means there will be oversights. Unless we guard against them, they'll kill the QMP connection, so we better do. The guard has evolved over time. See, for instance, commit 10e4f60 and commit 74ee59a. Today, it works like this: at the deepest possible point in the print-to-monitor call chain, namely monitor_vprintf(), we silently do nothing unless we're in HMP contect. Print-to-monitor functions further up the chain can remain oblivious of the need for HMP. Since they can, they should. Print-to-the-right-place functions still need to decide whether to print to the monitor or to stderr. Again, this happens at the deepest possible point in the call chain: error_vprintf(). qerror_report() is a different beast entirely. It's the obsolete error reporting API designed for minimally invasive conversion from existing "just print the error" practice, saving us from having to rework call chains to pass up error objects, with lots of fine opportunities to leak them along the way. Unfortunately, we later had to accept that there is no way around that, and so we got error_set(). qerror_report() is still around, though. It works like this: print the error to the right place right away, *except* in QMP context. There, store an error object in the monitor, for later sending to the client. Clear as mud?
On 30 January 2014 08:00, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: >> [MacOSX doesn't pull in common symbols from .o files in >> .a archives] > Not ugly, but a sensible move (in my opinion) regardless of this > specific issue: compile with -fno-common. Then both become not common. > > `-fno-common' > In C code, controls the placement of uninitialized global > variables. Unix C compilers have traditionally permitted multiple > definitions of such variables in different compilation units by > placing the variables in a common block. This is the behavior > specified by `-fcommon', and is the default for GCC on most > targets. On the other hand, this behavior is not required by ISO > C, and on some targets may carry a speed or code size penalty on > variable references. The `-fno-common' option specifies that the > compiler should place uninitialized global variables in the data > section of the object file, rather than generating them as common > blocks. This has the effect that if the same variable is declared > (without `extern') in two different compilations, you will get a > multiple-definition error when you link them. In this case, you > must compile with `-fcommon' instead. Compiling with > `-fno-common' is useful on targets for which it provides better > performance, or if you wish to verify that the program will work > on other systems that always treat uninitialized variable > declarations this way. This seems to work. Judging by that description, we could build with -fno-common everywhere. That would mean that accidentally declaring the same variable in two compilation units was a compile failure everywhere rather than just on MacOSX, which I think is preferable. Is there any reason we shouldn't just build with -fno-common for all platforms and compilers? thanks -- PMM
diff --git a/util/error.c b/util/error.c index e5de34f..f11f1d5 100644 --- a/util/error.c +++ b/util/error.c @@ -44,7 +44,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...) err->err_class = err_class; if (errp == &error_abort) { - fprintf(stderr, "%s\n", error_get_pretty(err)); + error_report("%s", error_get_pretty(err)); abort(); } @@ -80,7 +80,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class, err->err_class = err_class; if (errp == &error_abort) { - fprintf(stderr, "%s\n", error_get_pretty(err)); + error_report("%s", error_get_pretty(err)); abort(); } @@ -125,7 +125,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class, err->err_class = err_class; if (errp == &error_abort) { - fprintf(stderr, "%s\n", error_get_pretty(err)); + error_report("%s", error_get_pretty(err)); abort(); } @@ -171,7 +171,7 @@ void error_free(Error *err) void error_propagate(Error **dst_err, Error *local_err) { if (local_err && dst_err == &error_abort) { - fprintf(stderr, "%s\n", error_get_pretty(local_err)); + error_report("%s", error_get_pretty(local_err)); abort(); } else if (dst_err && !*dst_err) { *dst_err = local_err;
This reverts commit d32934c84c72f57e78d430c22974677b7bcabe5d. The original implementation before this patch makes abortive error messages much more friendly. The underlying bug that required this change is now fixed. Revert. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- util/error.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)