diff mbox

[v2,05/39] fdsets: use weak aliases instead of qemu-tool.c/qemu-user.c

Message ID 50A608C6.1080804@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Nov. 16, 2012, 9:35 a.m. UTC 
Il 15/11/2012 23:18, Stefan Weil ha scritto:
> Am 15.11.2012 21:52, schrieb Paolo Bonzini:
>> Il 15/11/2012 19:01, Stefan Weil ha scritto:
>>> Hi Paolo,
>>>
>>> this patch breaks QEMU on 32 and 64 bit hosts, native and with Wine.
>>> It's easy to reproduce the SIGSEGV crash: just add a -snapshot option.
>>> Obviously the critical code is executed only when this option was used.
>>
>> I cannot reproduce this, so it must be an assembler or linker bug.
>>
>> Can you try the alternative code that is used for Mac OS X?
> 
> The code which is used for Mac OS X also compiles and
> results in the same run-time bug with Wine:

Ok, I reproduced the original binutils bug, and found a typo in the
weakrefs implementation.  Does this work for you?



If it still doesn't work, let's make sure that this reduced testcase works
for you:

g1.c:
#include <stdio.h>
int f() { printf("strong"); return 82; }
int g() { printf("strong"); return 83; }

g2.c:
#include <stdio.h>
static int weak_f() { return 42; }
static int weak_g() { return 43; }
typeof(weak_f) f __attribute__((__weak__, __alias__("weak_f")));
typeof(weak_g) g __attribute__((__weak__, __alias__("weak_g")));
int main() { printf("%d/%d\n", f(), g()); }

g3.c:
#include <stdio.h>
static int default_f() { return 42; }
static int default_g() { return 43; }
static typeof(default_f) weak_f __attribute__((__weakref__("f")));
static typeof(default_g) weak_g __attribute__((__weakref__("g")));
int main() { printf("%d/%d\n", (weak_f?:default_f)(), (weak_g?:default_g)()); }

Output should be:
- 42/43 for "gcc g2.c"
- 42/43 for "gcc g3.c"
- strongstrong82/83 for "gcc g1.c g2.c"
- strongstrong82/83 for "gcc g1.c g3.c"

Output on Windows is:
- 42/42 for "gcc g2.c"
- 42/43 for "gcc g3.c"
- segfault for "gcc g1.c g2.c"
- strongstrong82/83 for "gcc g1.c g3.c"

So, indeed "normal" weak symbols are broken, but weakrefs seem to work.

If the above patch doesn't work, and/or the reduced testcase fails,
please send to me:

- the .exe for "gcc g1.c g3.c"
- the .s file for g3.s
- the .o file for osdep.o
- the linked .exe (I assume both the windows and console versions fail)
- perhaps the .s file for osdep too; add --save-temps to the compiler command line to get it

Paolo

Comments

Stefan Weil Nov. 16, 2012, 5:15 p.m. UTC | #1 
Am 16.11.2012 10:35, schrieb Paolo Bonzini:
> Il 15/11/2012 23:18, Stefan Weil ha scritto:
>> Am 15.11.2012 21:52, schrieb Paolo Bonzini:
>>> Il 15/11/2012 19:01, Stefan Weil ha scritto:
>>>> Hi Paolo,
>>>>
>>>> this patch breaks QEMU on 32 and 64 bit hosts, native and with Wine.
>>>> It's easy to reproduce the SIGSEGV crash: just add a -snapshot option.
>>>> Obviously the critical code is executed only when this option was used.
>>>
>>> I cannot reproduce this, so it must be an assembler or linker bug.
>>>
>>> Can you try the alternative code that is used for Mac OS X?
>>
>> The code which is used for Mac OS X also compiles and
>> results in the same run-time bug with Wine:
>
> Ok, I reproduced the original binutils bug, and found a typo in the
> weakrefs implementation.  Does this work for you?
>
> diff --git a/compiler.h b/compiler.h
> index 55d7d74..d552757 100644
> --- a/compiler.h
> +++ b/compiler.h
> @@ -50,11 +50,12 @@
>   #   define __printf__ __gnu_printf__
>   #  endif
>   # endif
> -# if defined(__APPLE__)
> +# if defined(__APPLE__) || defined(_WIN32)
>   #  define QEMU_WEAK_ALIAS(newname, oldname) \
> -        static typeof(oldname) weak_##newname __attribute__((unused, weakref(#oldname)))
> +        static typeof(oldname) weak_##newname __attribute__((unused, weakref(#newname)))
>   #  define QEMU_WEAK_REF(newname, oldname) (weak_##newname ? weak_##newname : oldname)
>   # else
> +#error
>   #  define QEMU_WEAK_ALIAS(newname, oldname) \
>           typeof(oldname) newname __attribute__((weak, alias (#oldname)))
>   #  define QEMU_WEAK_REF(newname, oldname) newname
>
>
> If it still doesn't work, let's make sure that this reduced testcase works
> for you:

Tested-by: Stefan Weil <sw@weilnetz.de>

Great, the above patch fixes w32/w64 (native and with Wine).
Is this modification needed / does it work with MacOS X, too?

With the reduced testcase, I get the same results as in your test.

Regards
Stefan


>
> g1.c:
> #include<stdio.h>
> int f() { printf("strong"); return 82; }
> int g() { printf("strong"); return 83; }
>
> g2.c:
> #include<stdio.h>
> static int weak_f() { return 42; }
> static int weak_g() { return 43; }
> typeof(weak_f) f __attribute__((__weak__, __alias__("weak_f")));
> typeof(weak_g) g __attribute__((__weak__, __alias__("weak_g")));
> int main() { printf("%d/%d\n", f(), g()); }
>
> g3.c:
> #include<stdio.h>
> static int default_f() { return 42; }
> static int default_g() { return 43; }
> static typeof(default_f) weak_f __attribute__((__weakref__("f")));
> static typeof(default_g) weak_g __attribute__((__weakref__("g")));
> int main() { printf("%d/%d\n", (weak_f?:default_f)(), (weak_g?:default_g)()); }
>
> Output should be:
> - 42/43 for "gcc g2.c"
> - 42/43 for "gcc g3.c"
> - strongstrong82/83 for "gcc g1.c g2.c"
> - strongstrong82/83 for "gcc g1.c g3.c"
>
> Output on Windows is:
> - 42/42 for "gcc g2.c"
> - 42/43 for "gcc g3.c"
> - segfault for "gcc g1.c g2.c"
> - strongstrong82/83 for "gcc g1.c g3.c"
>
> So, indeed "normal" weak symbols are broken, but weakrefs seem to work.
>
> If the above patch doesn't work, and/or the reduced testcase fails,
> please send to me:
>
> - the .exe for "gcc g1.c g3.c"
> - the .s file for g3.s
> - the .o file for osdep.o
> - the linked .exe (I assume both the windows and console versions fail)
> - perhaps the .s file for osdep too; add --save-temps to the compiler command line to get it
>
> Paolo
Paolo Bonzini Nov. 16, 2012, 5:39 p.m. UTC | #2 
Il 16/11/2012 18:15, Stefan Weil ha scritto:
> 
> Tested-by: Stefan Weil <sw@weilnetz.de>
> 
> Great, the above patch fixes w32/w64 (native and with Wine).
> Is this modification needed / does it work with MacOS X, too?

Yes, but I found a way to get rid of weak references completely.  The
testing is still precious---thanks very much.

> With the reduced testcase, I get the same results as in your test.

Good to know, too. :)  llvm-gcc totally botches it on Mac OS X, by the
way (while clang works there).

Paolo
Peter Maydell Nov. 16, 2012, 5:52 p.m. UTC | #3 
On 16 November 2012 09:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Ok, I reproduced the original binutils bug, and found a typo in the
> weakrefs implementation.  Does this work for you?
>
> diff --git a/compiler.h b/compiler.h
> index 55d7d74..d552757 100644
> --- a/compiler.h
> +++ b/compiler.h
> @@ -50,11 +50,12 @@
>  #   define __printf__ __gnu_printf__
>  #  endif
>  # endif
> -# if defined(__APPLE__)
> +# if defined(__APPLE__) || defined(_WIN32)
>  #  define QEMU_WEAK_ALIAS(newname, oldname) \
> -        static typeof(oldname) weak_##newname __attribute__((unused, weakref(#oldname)))
> +        static typeof(oldname) weak_##newname __attribute__((unused, weakref(#newname)))
>  #  define QEMU_WEAK_REF(newname, oldname) (weak_##newname ? weak_##newname : oldname)
>  # else
> +#error
>  #  define QEMU_WEAK_ALIAS(newname, oldname) \
>          typeof(oldname) newname __attribute__((weak, alias (#oldname)))
>  #  define QEMU_WEAK_REF(newname, oldname) newname

This change makes compilation fail with clang:
  LINK  qemu-ga
Undefined symbols for architecture x86_64:
  "_monitor_fdset_dup_fd_add", referenced from:
      _qemu_open in osdep.o
  "_monitor_fdset_dup_fd_remove", referenced from:
      _qemu_close in osdep.o
  "_monitor_fdset_get_fd", referenced from:
      _qemu_open in osdep.o
  "_monitor_get_fd", referenced from:
      _socket_connect in qemu-sockets.o
      _socket_listen in qemu-sockets.o
  "_qemu_set_fd_handler2", referenced from:
      _inet_connect_addr in qemu-sockets.o
      _unix_connect_opts in qemu-sockets.o
      _wait_for_connect in qemu-sockets.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

(clang builds OK with current git master.)

-- PMM
Paolo Bonzini Nov. 16, 2012, 5:55 p.m. UTC | #4 
Il 16/11/2012 18:52, Peter Maydell ha scritto:
> clang: error: linker command failed with exit code 1 (use -v to see invocation)
> 
> (clang builds OK with current git master.)

But I have no idea why, or whether the generated code is correct...

Looks like the safest bet is to go with the good old static library.

Paolo
diff mbox

Patch

diff --git a/compiler.h b/compiler.h
index 55d7d74..d552757 100644
--- a/compiler.h
+++ b/compiler.h
@@ -50,11 +50,12 @@ 
 #   define __printf__ __gnu_printf__
 #  endif
 # endif
-# if defined(__APPLE__)
+# if defined(__APPLE__) || defined(_WIN32)
 #  define QEMU_WEAK_ALIAS(newname, oldname) \
-        static typeof(oldname) weak_##newname __attribute__((unused, weakref(#oldname)))
+        static typeof(oldname) weak_##newname __attribute__((unused, weakref(#newname)))
 #  define QEMU_WEAK_REF(newname, oldname) (weak_##newname ? weak_##newname : oldname)
 # else
+#error
 #  define QEMU_WEAK_ALIAS(newname, oldname) \
         typeof(oldname) newname __attribute__((weak, alias (#oldname)))
 #  define QEMU_WEAK_REF(newname, oldname) newname