Re: [RFC PATCH v3 04/34] Hexagon (target/hexagon) scalar core definition

[Richard Henderson]

On 8/18/20 8:50 AM, Taylor Simpson wrote:
> +#include <fenv.h>

This should not be in cpu.h.  What's up?


> +#define TARGET_PAGE_BITS 16     /* 64K pages */
> +#define TARGET_LONG_BITS 32

Belongs in cpu-param.h

> +#ifdef CONFIG_USER_ONLY
> +#define TOTAL_PER_THREAD_REGS 64
> +#else
...
> +    target_ulong gpr[TOTAL_PER_THREAD_REGS];

Do I not understand hexagon enough to know why the number of general registers
would vary with system mode?  Why is the define conditional on user-only?

> +/*
> + * Change HEX_DEBUG to 1 to turn on debugging output
> + */
> +#define HEX_DEBUG 0
> +#define HEX_DEBUG_LOG(...) \
> +    do { \
> +        if (HEX_DEBUG) { \
> +            rcu_read_lock(); \
> +            fprintf(stderr, __VA_ARGS__); \
> +            rcu_read_unlock(); \
> +        } \
> +    } while (0)
> +

No.  There are plenty of bad examples of this in qemu, let's not add another.

First, the lock doesn't do what you think.
Second, stderr is never right.
Third, just about any time you want this, there's a tracepoint that you could
add that would be better, correct, and toggleable from the command-line.

> +/*
> + * One of the main debugging techniques is to use "-d cpu" and compare 
> against
> + * LLDB output when single stepping.  However, the target and qemu put the
> + * stacks at different locations.  This is used to compensate so the diff is
> + * cleaner.
> + */
> +static inline target_ulong hack_stack_ptrs(CPUHexagonState *env,
> +                                           target_ulong addr)
> +{
> +    static bool first = true;
> +    if (first) {
> +        first = false;
> +        env->stack_start = env->gpr[HEX_REG_SP];
> +        env->gpr[HEX_REG_USR] = 0x56000;
> +
> +#define ADJUST_STACK 0
> +#if ADJUST_STACK
> +        /*
> +         * Change the two numbers below to
> +         *     1    qemu stack location
> +         *     2    hardware stack location
> +         * Or set to zero for normal mode (no stack adjustment)
> +         */
> +        env->stack_adjust = 0xfffeeb80 - 0xbf89f980;
> +#else
> +        env->stack_adjust = 0;
> +#endif
> +    }
> +
> +    target_ulong stack_start = env->stack_start;
> +    target_ulong stack_size = 0x10000;
> +    target_ulong stack_adjust = env->stack_adjust;
> +
> +    if (stack_start + 0x1000 >= addr && addr >= (stack_start - stack_size)) {
> +        return addr - stack_adjust;
> +    }
> +    return addr;
> +}

I understand your desire for this sort of comparison.  What I don't understand
is the method.  Surely it would be preferable to actually change the stack
location in qemu, rather than constantly adjust for it.

Add a per-target hook to linux-user/hexagon/target_elf.h that controls the
allocation of the stack in elfload.c, setup_arg_pages().

> +static void hexagon_dump(CPUHexagonState *env, FILE *f)
> +{
> +    static target_ulong last_pc;
> +    int i;
> +
> +    /*
> +     * When comparing with LLDB, it doesn't step through single-cycle
> +     * hardware loops the same way.  So, we just skip them here
> +     */
> +    if (env->gpr[HEX_REG_PC] == last_pc) {
> +        return;
> +    }

Multi-threaded data race.  Might as well move last_pc to env->dump_last_pc or
something.

But I'd also suggest that all of this lldb compatibility stuff be optional,
switchable from the command-line with a cpu property.  Because there are going
to be real cases where *not* single-stepping will result in dumps from the same
PC, and you've just squashed all of those.

Call the property x-lldb-compat, or something, and default it to off.  You then
turn it on with "-cpu v67,x-lldb-compat=on".


r~