configure | 4 +-- include/hw/elf_ops.h | 48 +++++++++++++------------ include/qemu/selfmap.h | 44 +++++++++++++++++++++++ fpu/softfloat.c | 3 ++ gdbstub.c | 4 +-- hw/core/loader.c | 5 ++- linux-user/elfload.c | 8 ++++- linux-user/syscall.c | 80 ++++++++++++++++++++++-------------------- target/xtensa/translate.c | 5 +++ tcg/i386/tcg-target.inc.c | 2 +- util/selfmap.c | 78 ++++++++++++++++++++++++++++++++++++++++ .github/lockdown.yml | 34 ++++++++++++++++++ MAINTAINERS | 1 + tests/tcg/x86_64/system/boot.S | 5 +-- util/Makefile.objs | 1 + 15 files changed, 250 insertions(+), 72 deletions(-) create mode 100644 include/qemu/selfmap.h create mode 100644 util/selfmap.c create mode 100644 .github/lockdown.yml
The following changes since commit 53ef8a92eb04ee19640f5aad3bff36cd4a36c250: Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20200406' into staging (2020-04-06 12:36:45 +0100) are available in the Git repository at: https://github.com/stsquad/qemu.git tags/pull-misc-fixes-070420-1 for you to fetch changes up to cce743abbf398a324879039cd582349b36da0ea6: tcg/i386: Fix %r12 guest_base initialization (2020-04-07 16:19:49 +0100) ---------------------------------------------------------------- Various fixes: - add .github repo lockdown config - better handle missing symbols in elf-ops - protect fcntl64 with #ifdef - remove unused macros from test - fix handling of /proc/self/maps - avoid BAD_SHIFT in x80 softfloat - properly terminate on .hex EOF - fix configure probe on windows cross build - fix %r12 guest_base initialization ---------------------------------------------------------------- Alex Bennée (8): elf-ops: bail out if we have no function symbols linux-user: protect fcntl64 with an #ifdef tests/tcg: remove extraneous pasting macros linux-user: more debug for init_guest_space target/xtensa: add FIXME for translation memory leak linux-user: factor out reading of /proc/self/maps linux-user: clean-up padding on /proc/self/maps hw/core: properly terminate loading .hex on EOF record Denis Plotnikov (1): gdbstub: fix compiler complaining Philippe Mathieu-Daudé (1): .github: Enable repo-lockdown bot to refuse GitHub pull requests Richard Henderson (3): softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal configure: Add -Werror to PIE probe tcg/i386: Fix %r12 guest_base initialization configure | 4 +-- include/hw/elf_ops.h | 48 +++++++++++++------------ include/qemu/selfmap.h | 44 +++++++++++++++++++++++ fpu/softfloat.c | 3 ++ gdbstub.c | 4 +-- hw/core/loader.c | 5 ++- linux-user/elfload.c | 8 ++++- linux-user/syscall.c | 80 ++++++++++++++++++++++-------------------- target/xtensa/translate.c | 5 +++ tcg/i386/tcg-target.inc.c | 2 +- util/selfmap.c | 78 ++++++++++++++++++++++++++++++++++++++++ .github/lockdown.yml | 34 ++++++++++++++++++ MAINTAINERS | 1 + tests/tcg/x86_64/system/boot.S | 5 +-- util/Makefile.objs | 1 + 15 files changed, 250 insertions(+), 72 deletions(-) create mode 100644 include/qemu/selfmap.h create mode 100644 util/selfmap.c create mode 100644 .github/lockdown.yml -- 2.20.1
On Tue, 7 Apr 2020 at 16:51, Alex Bennée <alex.bennee@linaro.org> wrote: > > The following changes since commit 53ef8a92eb04ee19640f5aad3bff36cd4a36c250: > > Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20200406' into staging (2020-04-06 12:36:45 +0100) > > are available in the Git repository at: > > https://github.com/stsquad/qemu.git tags/pull-misc-fixes-070420-1 > > for you to fetch changes up to cce743abbf398a324879039cd582349b36da0ea6: > > tcg/i386: Fix %r12 guest_base initialization (2020-04-07 16:19:49 +0100) > > ---------------------------------------------------------------- > Various fixes: > > - add .github repo lockdown config > - better handle missing symbols in elf-ops > - protect fcntl64 with #ifdef > - remove unused macros from test > - fix handling of /proc/self/maps > - avoid BAD_SHIFT in x80 softfloat > - properly terminate on .hex EOF > - fix configure probe on windows cross build > - fix %r12 guest_base initialization Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0 for any user-visible changes. -- PMM
It's perfectly possible to have no function symbols in your elf file and if we do the undefined behaviour sanitizer rightly complains about us passing NULL to qsort. Check nsyms before we go ahead. While we are at it lets drop the unchecked return value and cleanup the fail leg by use of g_autoptr. Another fix was proposed 101 weeks ago in: Message-Id: 20180421232120.22208-1-f4bug@amsat.org Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-Id: <20200403191150.863-2-alex.bennee@linaro.org> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h index a1411bfcab6..e0bb47bb678 100644 --- a/include/hw/elf_ops.h +++ b/include/hw/elf_ops.h @@ -104,19 +104,21 @@ static int glue(symcmp, SZ)(const void *s0, const void *s1) : ((sym0->st_value > sym1->st_value) ? 1 : 0); } -static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, - int clear_lsb, symbol_fn_t sym_cb) +static void glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, + int clear_lsb, symbol_fn_t sym_cb) { - struct elf_shdr *symtab, *strtab, *shdr_table = NULL; - struct elf_sym *syms = NULL; + struct elf_shdr *symtab, *strtab; + g_autofree struct elf_shdr *shdr_table = NULL; + g_autofree struct elf_sym *syms = NULL; + g_autofree char *str = NULL; struct syminfo *s; int nsyms, i; - char *str = NULL; shdr_table = load_at(fd, ehdr->e_shoff, sizeof(struct elf_shdr) * ehdr->e_shnum); - if (!shdr_table) - return -1; + if (!shdr_table) { + return ; + } if (must_swab) { for (i = 0; i < ehdr->e_shnum; i++) { @@ -125,23 +127,25 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, } symtab = glue(find_section, SZ)(shdr_table, ehdr->e_shnum, SHT_SYMTAB); - if (!symtab) - goto fail; + if (!symtab) { + return; + } syms = load_at(fd, symtab->sh_offset, symtab->sh_size); - if (!syms) - goto fail; + if (!syms) { + return; + } nsyms = symtab->sh_size / sizeof(struct elf_sym); /* String table */ if (symtab->sh_link >= ehdr->e_shnum) { - goto fail; + return; } strtab = &shdr_table[symtab->sh_link]; str = load_at(fd, strtab->sh_offset, strtab->sh_size); if (!str) { - goto fail; + return; } i = 0; @@ -170,8 +174,13 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, } i++; } - syms = g_realloc(syms, nsyms * sizeof(*syms)); + /* check we have symbols left */ + if (nsyms == 0) { + return; + } + + syms = g_realloc(syms, nsyms * sizeof(*syms)); qsort(syms, nsyms, sizeof(*syms), glue(symcmp, SZ)); for (i = 0; i < nsyms - 1; i++) { if (syms[i].st_size == 0) { @@ -182,18 +191,11 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, /* Commit */ s = g_malloc0(sizeof(*s)); s->lookup_symbol = glue(lookup_symbol, SZ); - glue(s->disas_symtab.elf, SZ) = syms; + glue(s->disas_symtab.elf, SZ) = g_steal_pointer(&syms); s->disas_num_syms = nsyms; - s->disas_strtab = str; + s->disas_strtab = g_steal_pointer(&str); s->next = syminfos; syminfos = s; - g_free(shdr_table); - return 0; - fail: - g_free(syms); - g_free(str); - g_free(shdr_table); - return -1; } static int glue(elf_reloc, SZ)(struct elfhdr *ehdr, int fd, int must_swab, -- 2.20.1
Checking TARGET_ABI_BITS is sketchy - we should check for the presence of the define to be sure. Also clean up the white space while we are there. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Laurent Vivier <laurent@vivier.eu> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-Id: <20200403191150.863-3-alex.bennee@linaro.org> diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 5af55fca781..b679bc6b136 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -11331,11 +11331,11 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, This is a hint, so ignoring and returning success is ok. */ return 0; #endif -#if TARGET_ABI_BITS == 32 +#ifdef TARGET_NR_fcntl64 case TARGET_NR_fcntl64: { - int cmd; - struct flock64 fl; + int cmd; + struct flock64 fl; from_flock64_fn *copyfrom = copy_from_user_flock64; to_flock64_fn *copyto = copy_to_user_flock64; @@ -11346,7 +11346,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, } #endif - cmd = target_to_host_fcntl_cmd(arg2); + cmd = target_to_host_fcntl_cmd(arg2); if (cmd == -TARGET_EINVAL) { return cmd; } -- 2.20.1
We are not using them and they just get in the way. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-Id: <20200403191150.863-4-alex.bennee@linaro.org> diff --git a/tests/tcg/x86_64/system/boot.S b/tests/tcg/x86_64/system/boot.S index 205cfbd3982..73b19a2bda6 100644 --- a/tests/tcg/x86_64/system/boot.S +++ b/tests/tcg/x86_64/system/boot.S @@ -41,10 +41,7 @@ #define XEN_ELFNOTE_PHYS32_ENTRY 18 #define __ASM_FORM(x) x -#define __ASM_FORM_RAW(x) x -#define __ASM_FORM_COMMA(x) x, -#define __ASM_SEL(a,b) __ASM_FORM(b) -#define __ASM_SEL_RAW(a,b) __ASM_FORM_RAW(b) +#define __ASM_SEL(a,b) __ASM_FORM(b) #define _ASM_PTR __ASM_SEL(.long, .quad) ELFNOTE(Xen, XEN_ELFNOTE_VIRT_BASE, _ASM_PTR 0x100000) -- 2.20.1
Searching for memory space can cause problems so lets extend the CPU_LOG_PAGE output so you can watch init_guest_space fail to allocate memory. A more involved fix is actually required to make this function play nicely with the large guard pages the sanitiser likes to use. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Laurent Vivier <laurent@vivier.eu> Message-Id: <20200403191150.863-5-alex.bennee@linaro.org> diff --git a/linux-user/elfload.c b/linux-user/elfload.c index 8198be04460..619c054cc48 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -2172,6 +2172,8 @@ unsigned long init_guest_space(unsigned long host_start, /* Check to see if the address is valid. */ if (host_start && real_start != current_start) { + qemu_log_mask(CPU_LOG_PAGE, "invalid %lx && %lx != %lx\n", + host_start, real_start, current_start); goto try_again; } @@ -2240,7 +2242,11 @@ unsigned long init_guest_space(unsigned long host_start, * probably a bad strategy if not, which means we got here * because of trouble with ARM commpage setup. */ - munmap((void *)real_start, real_size); + if (munmap((void *)real_start, real_size) != 0) { + error_report("%s: failed to unmap %lx:%lx (%s)", __func__, + real_start, real_size, strerror(errno)); + abort(); + } current_start += align; if (host_start == current_start) { /* Theoretically possible if host doesn't have any suitably -- 2.20.1
Dynamically allocating a new structure within the DisasContext can potentially leak as we can longjmp out of the translation loop (see test_phys_mem). The proper fix would be to use static allocation within the DisasContext but as the Xtensa translator imports it's code from elsewhere I leave that as an exercise for the maintainer. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Acked-by: Max Filippov <jcmvbkbc@gmail.com> Message-Id: <20200403191150.863-6-alex.bennee@linaro.org> diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c index 8aa972cafdf..37f65b1f030 100644 --- a/target/xtensa/translate.c +++ b/target/xtensa/translate.c @@ -1174,6 +1174,11 @@ static void xtensa_tr_init_disas_context(DisasContextBase *dcbase, dc->callinc = ((tb_flags & XTENSA_TBFLAG_CALLINC_MASK) >> XTENSA_TBFLAG_CALLINC_SHIFT); + /* + * FIXME: This will leak when a failed instruction load or similar + * event causes us to longjump out of the translation loop and + * hence not clean-up in xtensa_tr_tb_stop + */ if (dc->config->isa) { dc->insnbuf = xtensa_insnbuf_alloc(dc->config->isa); dc->slotbuf = xtensa_insnbuf_alloc(dc->config->isa); -- 2.20.1
From: Denis Plotnikov <dplotnikov@virtuozzo.com> ./gdbstub.c: In function ‘handle_query_thread_extra’: /usr/include/glib-2.0/glib/glib-autocleanups.h:28:10: error: ‘cpu_name’ may be used uninitialized in this function [-Werror=maybe-uninitialized] g_free (*pp); ^ ./gdbstub.c:2063:26: note: ‘cpu_name’ was declared here g_autofree char *cpu_name; ^ cc1: all warnings being treated as errors Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> Message-Id: <20200326151407.25046-1-dplotnikov@virtuozzo.com> Reported-by: Euler Robot <euler.robot@huawei.com> Reported-by: Chen Qun <kuhn.chenqun@huawei.com> Reviewed-by: Miroslav Rezanina <mrezanin@redhat.com> Message-Id: <20200325092137.24020-1-kuhn.chenqun@huawei.com> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Message-Id: <20200403191150.863-7-alex.bennee@linaro.org> diff --git a/gdbstub.c b/gdbstub.c index 013fb1ac0f1..171e1509509 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -2060,8 +2060,8 @@ static void handle_query_thread_extra(GdbCmdContext *gdb_ctx, void *user_ctx) /* Print the CPU model and name in multiprocess mode */ ObjectClass *oc = object_get_class(OBJECT(cpu)); const char *cpu_model = object_class_get_name(oc); - g_autofree char *cpu_name; - cpu_name = object_get_canonical_path_component(OBJECT(cpu)); + g_autofree char *cpu_name = + object_get_canonical_path_component(OBJECT(cpu)); g_string_printf(rs, "%s %s [%s]", cpu_model, cpu_name, cpu->halted ? "halted " : "running"); } else { -- 2.20.1
From: Richard Henderson <richard.henderson@linaro.org> All other calls to normalize*Subnormal detect zero input before the call -- this is the only outlier. This case can happen with +0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of the correct sign. Reported-by: Coverity (CID 1421991) Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org> Message-Id: <20200403191150.863-8-alex.bennee@linaro.org> diff --git a/fpu/softfloat.c b/fpu/softfloat.c index 301ce3b537b..ae6ba718540 100644 --- a/fpu/softfloat.c +++ b/fpu/softfloat.c @@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b, flag zSign, zSig1 = 0; zSig0 = aSig + bSig; if ( aExp == 0 ) { + if (zSig0 == 0) { + return packFloatx80(zSign, 0, 0); + } normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 ); goto roundAndPack; } -- 2.20.1
17:55 Uto, 07.04.2020. Alex Bennée <alex.bennee@linaro.org> је написао/ла: > > From: Richard Henderson <richard.henderson@linaro.org> > > All other calls to normalize*Subnormal detect zero input before > the call -- this is the only outlier. This case can happen with > +0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of > the correct sign. > > Reported-by: Coverity (CID 1421991) > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org> > Message-Id: <20200403191150.863-8-alex.bennee@linaro.org> > > diff --git a/fpu/softfloat.c b/fpu/softfloat.c > index 301ce3b537b..ae6ba718540 100644 > --- a/fpu/softfloat.c > +++ b/fpu/softfloat.c > @@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b, flag zSign, > zSig1 = 0; > zSig0 = aSig + bSig; > if ( aExp == 0 ) { > + if (zSig0 == 0) { > + return packFloatx80(zSign, 0, 0); > + } > normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 ); > goto roundAndPack; > } > -- > 2.20.1 > > We in MIPS have extensive FP tests, that certainly include many cases of operations with +0 and -0. And they are all correct even before this patch. Unfortunately, because of my current remote work, I don't havecthese tests with me, and can't confirm if they work correctly, or perhaps are unaffected at all. Alex, from the commit message, it not clear if this is a fix of a bug (in which case a test example would be useful to have, and the assesment on what scenarios could be affected), or just a correction for some rare condition that practically for all intents and purposes was never triggered, or perhaps something third. Alex, please explain this in more detail to me. Secondly, and not related to this patch only, I see more and more patches integrated into the main tree without "Reviewed-by:" tag. I don't think this is the best way an open source community works. In my personal opinion, this must stop. Regards, Aleksandar
On 4/10/20 2:38 AM, Aleksandar Markovic wrote: > 17:55 Uto, 07.04.2020. Alex Bennée <alex.bennee@linaro.org > <mailto:alex.bennee@linaro.org>> је написао/ла: >> >> From: Richard Henderson <richard.henderson@linaro.org > <mailto:richard.henderson@linaro.org>> >> >> All other calls to normalize*Subnormal detect zero input before >> the call -- this is the only outlier. This case can happen with >> +0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of >> the correct sign. >> >> Reported-by: Coverity (CID 1421991) >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org > <mailto:richard.henderson@linaro.org>> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org > <mailto:alex.bennee@linaro.org>> >> Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org > <mailto:20200327232042.10008-1-richard.henderson@linaro.org>> >> Message-Id: <20200403191150.863-8-alex.bennee@linaro.org > <mailto:20200403191150.863-8-alex.bennee@linaro.org>> >> >> diff --git a/fpu/softfloat.c b/fpu/softfloat.c >> index 301ce3b537b..ae6ba718540 100644 >> --- a/fpu/softfloat.c >> +++ b/fpu/softfloat.c >> @@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b, > flag zSign, >> zSig1 = 0; >> zSig0 = aSig + bSig; >> if ( aExp == 0 ) { >> + if (zSig0 == 0) { >> + return packFloatx80(zSign, 0, 0); >> + } >> normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 ); >> goto roundAndPack; >> } >> -- >> 2.20.1 >> >> > > We in MIPS have extensive FP tests, that certainly include many cases of > operations with +0 and -0. And they are all correct even before this patch. This is for the 80-bit extended-double type, only used on x86 and m68k. You will not execute this path using MIPS. > Alex, from the commit message, it not clear if this is a fix of a bug (in which > case a test example would be useful to have, and the assesment on what > scenarios could be affected), or just a correction for some rare condition that > practically for all intents and purposes was never triggered, or perhaps > something third. This only avoids a Coverity out-of-range shift warning. Beforehand, we executed 0 << 64, got 0 as the result (regardless of whether or not the host truncates the shift count), and constructed the correctly signed fp zero in the end. There was more discussion about this in an earlier thread, associated with a different patch for this same problem: https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg08278.html > Secondly, and not related to this patch only, I see more and more patches > integrated into the main tree without "Reviewed-by:" tag. I don't think this is > the best way an open source community works. In my personal opinion, this must > stop. The only way to avoid this is to have more developers review code outside their own bailiwick. The patch has been on the list for two weeks and was pinged twice. Although why Alex didn't add his own R-b to my patch when merging it to his branch, I don't know. r~
17:17 Pet, 10.04.2020. Richard Henderson <richard.henderson@linaro.org> је написао/ла: > > On 4/10/20 2:38 AM, Aleksandar Markovic wrote: > > 17:55 Uto, 07.04.2020. Alex Bennée <alex.bennee@linaro.org > > <mailto:alex.bennee@linaro.org>> је написао/ла: > >> > >> From: Richard Henderson <richard.henderson@linaro.org > > <mailto:richard.henderson@linaro.org>> > >> > >> All other calls to normalize*Subnormal detect zero input before > >> the call -- this is the only outlier. This case can happen with > >> +0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of > >> the correct sign. > >> > >> Reported-by: Coverity (CID 1421991) > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org > > <mailto:richard.henderson@linaro.org>> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org > > <mailto:alex.bennee@linaro.org>> > >> Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org > > <mailto:20200327232042.10008-1-richard.henderson@linaro.org>> > >> Message-Id: <20200403191150.863-8-alex.bennee@linaro.org > > <mailto:20200403191150.863-8-alex.bennee@linaro.org>> > >> > >> diff --git a/fpu/softfloat.c b/fpu/softfloat.c > >> index 301ce3b537b..ae6ba718540 100644 > >> --- a/fpu/softfloat.c > >> +++ b/fpu/softfloat.c > >> @@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b, > > flag zSign, > >> zSig1 = 0; > >> zSig0 = aSig + bSig; > >> if ( aExp == 0 ) { > >> + if (zSig0 == 0) { > >> + return packFloatx80(zSign, 0, 0); > >> + } > >> normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 ); > >> goto roundAndPack; > >> } > >> -- > >> 2.20.1 > >> > >> > > > > We in MIPS have extensive FP tests, that certainly include many cases of > > operations with +0 and -0. And they are all correct even before this patch. > > This is for the 80-bit extended-double type, only used on x86 and m68k. You > will not execute this path using MIPS. > Thanks, Richard! First and foremost, may health be with you and all people of United States and all Americas!! Yes, the fact that m68k also uses 80-bit FP arithmetic was known to me. Though probably many people think 80-bit FP is limited to x86. I was just afraid of some strange way that other targets may end up using the function in question. But again, thanks for reassurances! > > Alex, from the commit message, it not clear if this is a fix of a bug (in which > > case a test example would be useful to have, and the assesment on what > > scenarios could be affected), or just a correction for some rare condition that > > practically for all intents and purposes was never triggered, or perhaps > > something third. > > This only avoids a Coverity out-of-range shift warning. > > Beforehand, we executed 0 << 64, got 0 as the result (regardless of whether or > not the host truncates the shift count), and constructed the correctly signed > fp zero in the end. > > There was more discussion about this in an earlier thread, associated with a > different patch for this same problem: > > https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg08278.html > OK. I didn't have a chance to read this pattivular thread. Thanks for the pointer! > > > Secondly, and not related to this patch only, I see more and more patches > > integrated into the main tree without "Reviewed-by:" tag. I don't think this is > > the best way an open source community works. In my personal opinion, this must > > stop. > > The only way to avoid this is to have more developers review code outside their > own bailiwick. The patch has been on the list for two weeks and was pinged > twice. > > Although why Alex didn't add his own R-b to my patch when merging it to his > branch, I don't know. > I also have a very similar impression as yours, that Alex in fact reviewed the patch (as if he implicitely gave R-B, but forgot to insert it in a hurry, given these hectic days around 5.0 final release). Best regards, and best wishes to all Sietlans, or wherever you happen to live! Aleksandar > > r~
On Fri, 10 Apr 2020 at 16:17, Richard Henderson <richard.henderson@linaro.org> wrote: > Although why Alex didn't add his own R-b to my patch when merging it to his > branch, I don't know. I think this is one of those areas where different submaintainers have different work practices. Personally I distinguish "did I actually review this" from "did I just put this into my tree and rely on others doing the review" and use r-by for the former and not on the latter (although obviously everything I put in my tree I will have at least very very briefly looked over). But I think some submaintainers don't bother to add r-by tags for things they review in the process of assembling their tree because they see it as implicit in the process. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 10 Apr 2020 at 16:17, Richard Henderson > <richard.henderson@linaro.org> wrote: >> Although why Alex didn't add his own R-b to my patch when merging it to his >> branch, I don't know. > > I think this is one of those areas where different submaintainers > have different work practices. Personally I distinguish "did I > actually review this" from "did I just put this into my tree and > rely on others doing the review" and use r-by for the former > and not on the latter (although obviously everything I put in > my tree I will have at least very very briefly looked over). > But I think some submaintainers don't bother to add r-by tags > for things they review in the process of assembling their > tree because they see it as implicit in the process. It was exactly this - pulling in via my tree and adding my own s-o-b implies I'm happy enough with it. Typically for longer series that gestate on the list the total number of r-b tags grows with each re-roll until the series gets pulled into a maintainer branch. This PR is atypical in that regard because it's a fairly random collection of fixes. I think the only patches we should be wary of are those with only a single s-o-b tag from the author. I have to admit there was one such patch in this PR: Subject: [PULL 09/13] linux-user: factor out reading of /proc/self/maps Date: Tue, 7 Apr 2020 16:51:14 +0100 Message-Id: <20200407155118.20139-10-alex.bennee@linaro.org> I made an executive decision to include it as it was part of the bug fix for patch 10 and as we approach RC releases I wanted to get it merged. If you follow the msg-id in the patch you will see the changes in the patch are purely in response to review comments so while missing a r-b tag it's not like it's not been on the list and had some scrutiny. However we should certainly aim for most patches to be fully reviewed even if we never achieve that level of perfection. > > thanks > -- PMM -- Alex Bennée
18:14 Pet, 10.04.2020. Peter Maydell <peter.maydell@linaro.org> је написао/ла: > But I think some submaintainers don't bother to add r-by tags > for things they review in the process of assembling their > tree because they see it as implicit in the process. > I think that was precisely the case in this patch. May I wish you, Peter, the best health, and thanks UK for giving the world Dr. John Campbell from northern England, whose videos I watch every day with the closest possible attention, and the highest admiration. Aleksandar > thanks > -- PMM
Don't use magic spaces, calculate the justification for the file field like the kernel does with seq_pad. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Message-Id: <20200403191150.863-10-alex.bennee@linaro.org> diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 5f117872947..6495ddc4cda 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -7235,6 +7235,7 @@ static int open_self_maps(void *cpu_env, int fd) TaskState *ts = cpu->opaque; GSList *map_info = read_self_maps(); GSList *s; + int count; for (s = map_info; s; s = g_slist_next(s)) { MapInfo *e = (MapInfo *) s->data; @@ -7253,20 +7254,24 @@ static int open_self_maps(void *cpu_env, int fd) } if (h2g(min) == ts->info->stack_limit) { - path = " [stack]"; + path = "[stack]"; } else { path = e->path; } - dprintf(fd, TARGET_ABI_FMT_ptr "-" TARGET_ABI_FMT_ptr - " %c%c%c%c %08" PRIx64 " %s %"PRId64" %s%s\n", - h2g(min), h2g(max - 1) + 1, - e->is_read ? 'r' : '-', - e->is_write ? 'w' : '-', - e->is_exec ? 'x' : '-', - e->is_priv ? 'p' : '-', - (uint64_t) e->offset, e->dev, e->inode, - path ? " " : "", path ? path : ""); + count = dprintf(fd, TARGET_ABI_FMT_ptr "-" TARGET_ABI_FMT_ptr + " %c%c%c%c %08" PRIx64 " %s %"PRId64, + h2g(min), h2g(max - 1) + 1, + e->is_read ? 'r' : '-', + e->is_write ? 'w' : '-', + e->is_exec ? 'x' : '-', + e->is_priv ? 'p' : '-', + (uint64_t) e->offset, e->dev, e->inode); + if (path) { + dprintf(fd, "%*s%s\n", 73 - count, "", path); + } else { + dprintf(fd, "\n"); + } } } @@ -7277,9 +7282,10 @@ static int open_self_maps(void *cpu_env, int fd) * We only support execution from the vsyscall page. * This is as if CONFIG_LEGACY_VSYSCALL_XONLY=y from v5.3. */ - dprintf(fd, TARGET_FMT_lx "-" TARGET_FMT_lx - " --xp 00000000 00:00 0 [vsyscall]\n", - TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + TARGET_PAGE_SIZE); + count = dprintf(fd, TARGET_FMT_lx "-" TARGET_FMT_lx + " --xp 00000000 00:00 0", + TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + TARGET_PAGE_SIZE); + dprintf(fd, "%*s%s\n", 73 - count, "", "[vsyscall]"); #endif return 0; -- 2.20.1
The https://makecode.microbit.org/#editor generates slightly weird .hex files which work fine on a real microbit but causes QEMU to choke. The reason is extraneous data after the EOF record which causes the loader to attempt to write a bigger file than it should to the "rom". According to the HEX file spec an EOF really should be the last thing we process so lets do that. Reported-by: Ursula Bennée <alex.bennee@linaro.org> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Message-Id: <20200403191150.863-12-alex.bennee@linaro.org> diff --git a/hw/core/loader.c b/hw/core/loader.c index eeef6da9a1b..8bbb1797a4c 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -1447,6 +1447,7 @@ typedef struct { uint32_t current_rom_index; uint32_t rom_start_address; AddressSpace *as; + bool complete; } HexParser; /* return size or -1 if error */ @@ -1484,6 +1485,7 @@ static int handle_record_type(HexParser *parser) parser->current_rom_index, parser->rom_start_address, parser->as); } + parser->complete = true; return parser->total_size; case EXT_SEG_ADDR_RECORD: case EXT_LINEAR_ADDR_RECORD: @@ -1548,11 +1550,12 @@ static int parse_hex_blob(const char *filename, hwaddr *addr, uint8_t *hex_blob, .bin_buf = g_malloc(hex_blob_size), .start_addr = addr, .as = as, + .complete = false }; rom_transaction_begin(); - for (; hex_blob < end; ++hex_blob) { + for (; hex_blob < end && !parser.complete; ++hex_blob) { switch (*hex_blob) { case '\r': case '\n': -- 2.20.1
From: Richard Henderson <richard.henderson@linaro.org> Without -Werror, the probe may succeed, but then compilation fails later when -Werror is added for other reasons. Shows up on windows, where the compiler complains about -fPIC. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> Message-Id: <20200401214756.6559-1-richard.henderson@linaro.org> Message-Id: <20200403191150.863-13-alex.bennee@linaro.org> diff --git a/configure b/configure index 22870f38672..233c671aaa9 100755 --- a/configure +++ b/configure @@ -2119,7 +2119,7 @@ if compile_prog "-Werror -fno-pie" "-no-pie"; then fi if test "$static" = "yes"; then - if test "$pie" != "no" && compile_prog "-fPIE -DPIE" "-static-pie"; then + if test "$pie" != "no" && compile_prog "-Werror -fPIE -DPIE" "-static-pie"; then QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS" QEMU_LDFLAGS="-static-pie $QEMU_LDFLAGS" pie="yes" @@ -2132,7 +2132,7 @@ if test "$static" = "yes"; then elif test "$pie" = "no"; then QEMU_CFLAGS="$CFLAGS_NOPIE $QEMU_CFLAGS" QEMU_LDFLAGS="$LDFLAGS_NOPIE $QEMU_LDFLAGS" -elif compile_prog "-fPIE -DPIE" "-pie"; then +elif compile_prog "-Werror -fPIE -DPIE" "-pie"; then QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS" QEMU_LDFLAGS="-pie $QEMU_LDFLAGS" pie="yes" -- 2.20.1
On 07/04/2020 16:51, Alex Bennée wrote: > From: Richard Henderson <richard.henderson@linaro.org> > > Without -Werror, the probe may succeed, but then compilation fails > later when -Werror is added for other reasons. Shows up on windows, > where the compiler complains about -fPIC. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Message-Id: <20200401214756.6559-1-richard.henderson@linaro.org> > Message-Id: <20200403191150.863-13-alex.bennee@linaro.org> > > diff --git a/configure b/configure > index 22870f38672..233c671aaa9 100755 > --- a/configure > +++ b/configure > @@ -2119,7 +2119,7 @@ if compile_prog "-Werror -fno-pie" "-no-pie"; then > fi > > if test "$static" = "yes"; then > - if test "$pie" != "no" && compile_prog "-fPIE -DPIE" "-static-pie"; then > + if test "$pie" != "no" && compile_prog "-Werror -fPIE -DPIE" "-static-pie"; then > QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS" > QEMU_LDFLAGS="-static-pie $QEMU_LDFLAGS" > pie="yes" > @@ -2132,7 +2132,7 @@ if test "$static" = "yes"; then > elif test "$pie" = "no"; then > QEMU_CFLAGS="$CFLAGS_NOPIE $QEMU_CFLAGS" > QEMU_LDFLAGS="$LDFLAGS_NOPIE $QEMU_LDFLAGS" > -elif compile_prog "-fPIE -DPIE" "-pie"; then > +elif compile_prog "-Werror -fPIE -DPIE" "-pie"; then > QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS" > QEMU_LDFLAGS="-pie $QEMU_LDFLAGS" > pie="yes" Note that even with this build fix, the native and cross-compiled Windows binaries crash upon startup unless --disable-pie is explicitly passed to configure (this seems like an unexpected side-effect of d2cd29e307: "configure: Do not force pie=no for non-x86"). Given that users such as Howard and https://bugs.launchpad.net/bugs/1870911 are starting to notice, would it be best to set this configure option as the default for Windows builds for now? ATB, Mark.
On 4/7/20 9:08 AM, Mark Cave-Ayland wrote: > Given that users such as Howard and https://bugs.launchpad.net/bugs/1870911 are > starting to notice, would it be best to set this configure option as the default for > Windows builds for now? I guess so. I'm mildly annoyed that Howard's compiler is silently accepting -fPIE and then producing incorrect code, rather than rejecting -fPIE as the docker win32 cross-compiler is doing. r~
From: Richard Henderson <richard.henderson@linaro.org> When %gs cannot be used, we use register offset addressing. This path is almost never used, so it was clearly not tested. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Tested-by: Alex Bennée <alex.bennee@linaro.org> Message-Id: <20200406174803.8192-1-richard.henderson@linaro.org> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c index 7f61eeedd09..ec083bddcfb 100644 --- a/tcg/i386/tcg-target.inc.c +++ b/tcg/i386/tcg-target.inc.c @@ -3737,7 +3737,7 @@ static void tcg_target_qemu_prologue(TCGContext *s) } else { /* Choose R12 because, as a base, it requires a SIB byte. */ x86_guest_base_index = TCG_REG_R12; - tcg_out_mov(s, TCG_TYPE_PTR, x86_guest_base_index, guest_base); + tcg_out_movi(s, TCG_TYPE_PTR, x86_guest_base_index, guest_base); tcg_regset_set_reg(s->reserved_regs, x86_guest_base_index); } } -- 2.20.1
© 2016 - 2024 Red Hat, Inc.