Re: [Qemu-devel] [PATCH v1 2/2] Revert "error: Don't use error_report() for assertion msgs."

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On 15 January 2014 11:06, Peter Crosthwaite
> <address@hidden> 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?