Message ID | 20190208035551.3036-1-richard.henderson@linaro.org |
---|---|
State | Superseded |
Headers | show |
Series | tcg: Diagnose referenced labels that have not been emitted | expand |
Patchew URL: https://patchew.org/QEMU/20190208035551.3036-1-richard.henderson@linaro.org/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted Type: series Message-id: 20190208035551.3036-1-richard.henderson@linaro.org === TEST SCRIPT BEGIN === #!/bin/bash git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20190208035551.3036-1-richard.henderson@linaro.org -> patchew/20190208035551.3036-1-richard.henderson@linaro.org Switched to a new branch 'test' 702a152f7d tcg: Diagnose referenced labels that have not been emitted === OUTPUT BEGIN === ERROR: spaces prohibited around that ':' (ctx:WxW) #90: FILE: tcg/tcg.h:249: + unsigned present : 1; ^ ERROR: spaces prohibited around that ':' (ctx:WxW) #93: FILE: tcg/tcg.h:251: + unsigned id : 14; ^ total: 2 errors, 0 warnings, 79 lines checked Commit 702a152f7d06 (tcg: Diagnose referenced labels that have not been emitted) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190208035551.3036-1-richard.henderson@linaro.org/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20190208035551.3036-1-richard.henderson@linaro.org/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Subject: [Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted Message-id: 20190208035551.3036-1-richard.henderson@linaro.org === TEST SCRIPT BEGIN === #!/bin/bash git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === From https://github.com/patchew-project/qemu * [new tag] patchew/20190208035551.3036-1-richard.henderson@linaro.org -> patchew/20190208035551.3036-1-richard.henderson@linaro.org Switched to a new branch 'test' 702a152 tcg: Diagnose referenced labels that have not been emitted === OUTPUT BEGIN === ERROR: spaces prohibited around that ':' (ctx:WxW) #90: FILE: tcg/tcg.h:249: + unsigned present : 1; ^ ERROR: spaces prohibited around that ':' (ctx:WxW) #93: FILE: tcg/tcg.h:251: + unsigned id : 14; ^ total: 2 errors, 0 warnings, 79 lines checked Commit 702a152f7d06 (tcg: Diagnose referenced labels that have not been emitted) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190208035551.3036-1-richard.henderson@linaro.org/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20190208035551.3036-1-richard.henderson@linaro.org/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted Message-id: 20190208035551.3036-1-richard.henderson@linaro.org Type: series === TEST SCRIPT BEGIN === #!/bin/bash git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20190208035551.3036-1-richard.henderson@linaro.org -> patchew/20190208035551.3036-1-richard.henderson@linaro.org Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone' Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc' Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers' Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF' Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe' Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios' Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware' Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode' Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios' Submodule 'roms/seabios-hppa' (https://github.com/hdeller/seabios-hppa.git) registered for path 'roms/seabios-hppa' Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios' Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot' Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot' Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex' Submodule 'tests/fp/berkeley-softfloat-3' (https://github.com/cota/berkeley-softfloat-3) registered for path 'tests/fp/berkeley-softfloat-3' Submodule 'tests/fp/berkeley-testfloat-3' (https://github.com/cota/berkeley-testfloat-3) registered for path 'tests/fp/berkeley-testfloat-3' Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into 'capstone'... Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf' Cloning into 'dtc'... Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536' Cloning into 'roms/QemuMacDrivers'... Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266' Cloning into 'roms/SLOF'... Submodule path 'roms/SLOF': checked out 'a5b428e1c1eae703bdd62a3f527223c291ee3fdc' Cloning into 'roms/ipxe'... Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17' Cloning into 'roms/openbios'... Submodule path 'roms/openbios': checked out '441a84d3a642a10b948369c63f32367e8ff6395b' Cloning into 'roms/openhackware'... Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5' Cloning into 'roms/qemu-palcode'... Submodule path 'roms/qemu-palcode': checked out '51c237d7e20d05100eacadee2f61abc17e6bc097' Cloning into 'roms/seabios'... Submodule path 'roms/seabios': checked out 'a698c8995ffb2838296ec284fe3c4ad33dfca307' Cloning into 'roms/seabios-hppa'... Submodule path 'roms/seabios-hppa': checked out '1ef99a01572c2581c30e16e6fe69e9ea2ef92ce0' Cloning into 'roms/sgabios'... Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a' Cloning into 'roms/skiboot'... Submodule path 'roms/skiboot': checked out 'e0ee24c27a172bcf482f6f2bc905e6211c134bcc' Cloning into 'roms/u-boot'... Submodule path 'roms/u-boot': checked out 'd85ca029f257b53a96da6c2fb421e78a003a9943' Cloning into 'roms/u-boot-sam460ex'... Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588' Cloning into 'tests/fp/berkeley-softfloat-3'... Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037' Cloning into 'tests/fp/berkeley-testfloat-3'... Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3' Cloning into 'ui/keycodemapdb'... Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce' Switched to a new branch 'test' 702a152 tcg: Diagnose referenced labels that have not been emitted === OUTPUT BEGIN === ERROR: spaces prohibited around that ':' (ctx:WxW) #90: FILE: tcg/tcg.h:249: + unsigned present : 1; ^ ERROR: spaces prohibited around that ':' (ctx:WxW) #93: FILE: tcg/tcg.h:251: + unsigned id : 14; ^ total: 2 errors, 0 warnings, 79 lines checked Commit 702a152f7d06 (tcg: Diagnose referenced labels that have not been emitted) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190208035551.3036-1-richard.henderson@linaro.org/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Hi Richard, On 2/8/19 4:55 AM, Richard Henderson wrote: > Currently, a jump to a label that is not defined anywhere will > be emitted not be relocated. This results in a jump to a random > jump target. With tcg debugging, print a diagnostic to the -d op > file and abort. > > This could help debug or detect errors like > c2d9644e6d ("target/arm: Fix crash on conditional instruction in an IT block") > > Reported-by: Roman Kapl <code@rkapl.cz> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > tcg/tcg-op.h | 1 + > tcg/tcg.h | 12 +++++++++--- > tcg/tcg.c | 23 +++++++++++++++++++++++ > 3 files changed, 33 insertions(+), 3 deletions(-) > --- > This detects errors earlier than the patch that Roman posted. > > > r~ > > > diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h > index 2d98868d8f..d3e51b15af 100644 > --- a/tcg/tcg-op.h > +++ b/tcg/tcg-op.h > @@ -255,6 +255,7 @@ static inline void tcg_gen_op6ii_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2, > > static inline void gen_set_label(TCGLabel *l) > { > + l->present = 1; > tcg_gen_op1(INDEX_op_set_label, label_arg(l)); > } > > diff --git a/tcg/tcg.h b/tcg/tcg.h > index 045c24a357..32b7cf3489 100644 > --- a/tcg/tcg.h > +++ b/tcg/tcg.h > @@ -244,16 +244,21 @@ typedef struct TCGRelocation { > intptr_t addend; > } TCGRelocation; > > -typedef struct TCGLabel { > +typedef struct TCGLabel TCGLabel; > +struct TCGLabel { > + unsigned present : 1; > unsigned has_value : 1; > - unsigned id : 15; > + unsigned id : 14; > unsigned refs : 16; > union { > uintptr_t value; > tcg_insn_unit *value_ptr; > TCGRelocation *first_reloc; > } u; > -} TCGLabel; > +#ifdef CONFIG_DEBUG_TCG > + QSIMPLEQ_ENTRY(TCGLabel) next; > +#endif > +}; > > typedef struct TCGPool { > struct TCGPool *next; > @@ -685,6 +690,7 @@ struct TCGContext { > #endif > > #ifdef CONFIG_DEBUG_TCG > + QSIMPLEQ_HEAD(, TCGLabel) labels; > int temps_in_use; > int goto_tb_issue_mask; > #endif > diff --git a/tcg/tcg.c b/tcg/tcg.c > index 20a5d8f315..9b2bf7f439 100644 > --- a/tcg/tcg.c > +++ b/tcg/tcg.c > @@ -305,6 +305,9 @@ TCGLabel *gen_new_label(void) Not related to this patch content, but I'm wondering why, TCGLabels never get freed? > *l = (TCGLabel){ > .id = s->nb_labels++ > }; > +#ifdef CONFIG_DEBUG_TCG > + QSIMPLEQ_INSERT_TAIL(&s->labels, l, next); > +#endif > > return l; > } > @@ -1092,6 +1095,9 @@ void tcg_func_start(TCGContext *s) > > QTAILQ_INIT(&s->ops); > QTAILQ_INIT(&s->free_ops); > +#ifdef CONFIG_DEBUG_TCG > + QSIMPLEQ_INIT(&s->labels); > +#endif > } > > static inline TCGTemp *tcg_temp_alloc(TCGContext *s) > @@ -3841,6 +3847,23 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb) > } > #endif > > +#ifdef CONFIG_DEBUG_TCG > + /* Ensure all labels referenced have been emitted. */ > + { > + TCGLabel *l; > + bool error = false; > + > + QSIMPLEQ_FOREACH(l, &s->labels, next) { > + if (unlikely(!l->present) && l->refs) { > + qemu_log_mask(CPU_LOG_TB_OP, > + "$L%d referenced but not present.\n", l->id); > + error = true; > + } > + } > + assert(!error); > + } > +#endif > + > #ifdef CONFIG_PROFILER > atomic_set(&prof->opt_time, prof->opt_time - profile_getclock()); > #endif > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 2/8/19 2:41 AM, Philippe Mathieu-Daudé wrote: > Not related to this patch content, but I'm wondering why, TCGLabels > never get freed? They are allocated with tcg_malloc, which is a pool allocator. The entire pool is freed after each TB is compiled. r~
On 2/8/19 5:52 PM, Richard Henderson wrote: > On 2/8/19 2:41 AM, Philippe Mathieu-Daudé wrote: >> Not related to this patch content, but I'm wondering why, TCGLabels >> never get freed? > > They are allocated with tcg_malloc, which is a pool allocator. The entire pool > is freed after each TB is compiled. Got it now, thanks! Phil.
diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h index 2d98868d8f..d3e51b15af 100644 --- a/tcg/tcg-op.h +++ b/tcg/tcg-op.h @@ -255,6 +255,7 @@ static inline void tcg_gen_op6ii_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2, static inline void gen_set_label(TCGLabel *l) { + l->present = 1; tcg_gen_op1(INDEX_op_set_label, label_arg(l)); } diff --git a/tcg/tcg.h b/tcg/tcg.h index 045c24a357..32b7cf3489 100644 --- a/tcg/tcg.h +++ b/tcg/tcg.h @@ -244,16 +244,21 @@ typedef struct TCGRelocation { intptr_t addend; } TCGRelocation; -typedef struct TCGLabel { +typedef struct TCGLabel TCGLabel; +struct TCGLabel { + unsigned present : 1; unsigned has_value : 1; - unsigned id : 15; + unsigned id : 14; unsigned refs : 16; union { uintptr_t value; tcg_insn_unit *value_ptr; TCGRelocation *first_reloc; } u; -} TCGLabel; +#ifdef CONFIG_DEBUG_TCG + QSIMPLEQ_ENTRY(TCGLabel) next; +#endif +}; typedef struct TCGPool { struct TCGPool *next; @@ -685,6 +690,7 @@ struct TCGContext { #endif #ifdef CONFIG_DEBUG_TCG + QSIMPLEQ_HEAD(, TCGLabel) labels; int temps_in_use; int goto_tb_issue_mask; #endif diff --git a/tcg/tcg.c b/tcg/tcg.c index 20a5d8f315..9b2bf7f439 100644 --- a/tcg/tcg.c +++ b/tcg/tcg.c @@ -305,6 +305,9 @@ TCGLabel *gen_new_label(void) *l = (TCGLabel){ .id = s->nb_labels++ }; +#ifdef CONFIG_DEBUG_TCG + QSIMPLEQ_INSERT_TAIL(&s->labels, l, next); +#endif return l; } @@ -1092,6 +1095,9 @@ void tcg_func_start(TCGContext *s) QTAILQ_INIT(&s->ops); QTAILQ_INIT(&s->free_ops); +#ifdef CONFIG_DEBUG_TCG + QSIMPLEQ_INIT(&s->labels); +#endif } static inline TCGTemp *tcg_temp_alloc(TCGContext *s) @@ -3841,6 +3847,23 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb) } #endif +#ifdef CONFIG_DEBUG_TCG + /* Ensure all labels referenced have been emitted. */ + { + TCGLabel *l; + bool error = false; + + QSIMPLEQ_FOREACH(l, &s->labels, next) { + if (unlikely(!l->present) && l->refs) { + qemu_log_mask(CPU_LOG_TB_OP, + "$L%d referenced but not present.\n", l->id); + error = true; + } + } + assert(!error); + } +#endif + #ifdef CONFIG_PROFILER atomic_set(&prof->opt_time, prof->opt_time - profile_getclock()); #endif
Currently, a jump to a label that is not defined anywhere will be emitted not be relocated. This results in a jump to a random jump target. With tcg debugging, print a diagnostic to the -d op file and abort. This could help debug or detect errors like c2d9644e6d ("target/arm: Fix crash on conditional instruction in an IT block") Reported-by: Roman Kapl <code@rkapl.cz> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- tcg/tcg-op.h | 1 + tcg/tcg.h | 12 +++++++++--- tcg/tcg.c | 23 +++++++++++++++++++++++ 3 files changed, 33 insertions(+), 3 deletions(-) --- This detects errors earlier than the patch that Roman posted. r~ -- 2.17.2