| Message ID | 20220501055028.646596-18-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Cleanups, new features, new cpus | expand |
On Sun, 1 May 2022 at 07:17, Richard Henderson <richard.henderson@linaro.org> wrote: > > Perform the override check early, so that it is still done > even when we decide to discard an unreachable cpreg. > > Use assert not printf+abort. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/helper.c | 22 ++++++++-------------- > 1 file changed, 8 insertions(+), 14 deletions(-) The reason I put the printf in here originally was that I felt the conditionality with which we define cpregs was complicated enough that conflicting cpreg definitions might be found not by the developer writing new code but perhaps by users in the field who used rarer CPU types or oddball feature enable combinations, and I wanted the reports to provide enough detail to identify the conflicting registers without having to repro running under a debugger to print out the values of oldreg and r2. Still, I suppose it's N years later and we haven't in fact particularly had that kind of bug report, so it's not strictly necessary. Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
diff --git a/target/arm/helper.c b/target/arm/helper.c index f1fbbdb9e0..2ed07795d8 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -8538,6 +8538,14 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r, g_assert_not_reached(); } + /* Overriding of an existing definition must be explicitly requested. */ + if (!(r->type & ARM_CP_OVERRIDE)) { + const ARMCPRegInfo *oldreg = get_arm_cp_reginfo(cpu->cp_regs, key); + if (oldreg) { + assert(oldreg->type & ARM_CP_OVERRIDE); + } + } + /* Combine cpreg and name into one allocation. */ name_len = strlen(name) + 1; r2 = g_malloc(sizeof(*r2) + name_len); @@ -8622,20 +8630,6 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r, assert(!raw_accessors_invalid(r2)); } - /* Overriding of an existing definition must be explicitly - * requested. - */ - if (!(r->type & ARM_CP_OVERRIDE)) { - const ARMCPRegInfo *oldreg = get_arm_cp_reginfo(cpu->cp_regs, key); - if (oldreg && !(oldreg->type & ARM_CP_OVERRIDE)) { - fprintf(stderr, "Register redefined: cp=%d %d bit " - "crn=%d crm=%d opc1=%d opc2=%d, " - "was %s, now %s\n", r2->cp, 32 + 32 * is64, - r2->crn, r2->crm, r2->opc1, r2->opc2, - oldreg->name, r2->name); - g_assert_not_reached(); - } - } g_hash_table_insert(cpu->cp_regs, (gpointer)(uintptr_t)key, r2); }
Perform the override check early, so that it is still done even when we decide to discard an unreachable cpreg. Use assert not printf+abort. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/helper.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)