On 4/21/25 23:04, Markus Armbruster wrote:
Pierrick Bouvier <pierrick.bouv...@linaro.org> writes:
[...]
At this point, I would like to focus on having a first version of TargetInfo
API, and not reviewing any other changes, as things may be modified, and they
would need to be reviewed again. It's hard to follow the same abstraction done
multiple times in multiple series.
Regarding your proposal for target_system_arch(), I understand that you tried
to reuse the existing SysEmuTarget, which was a good intention.
However, I don't think we should use any string compare for this (which
qemu_api_parse does). It has several flaws:
qemu_api_parse()? Do you mean qapi_enum_parse()?
Yes, sorry for the typo.
- The most important one: it can fail (what if -1 is returned?). Enums can be
guaranteed and exhaustive at compile time.
- It's slower than having the current arch directly known at compile time.
As well, since SysEmuTarget is a generated enum, it makes it much harder to
follow code IMHO.
QAPI requires those things to be defined from a json file for external usage,
but it's not a good reason for being forced to use it in all the codebase as
the only possible abstraction.
To have something fast and infallible, we can adopt this solution:
In target_info.h:
/* Named TargetArch to not clash with poisoned TARGET_X */
typedef enum TargetArch {
TARGET_ARCH_AARCH64,
TARGET_ARCH_ALPHA,
TARGET_ARCH_ARM,
TARGET_ARCH_AVR,
TARGET_ARCH_HPPA,
TARGET_ARCH_I386,
TARGET_ARCH_LOONGARCH64,
TARGET_ARCH_M68K,
TARGET_ARCH_MICROBLAZE,
TARGET_ARCH_MICROBLAZEEL,
TARGET_ARCH_MIPS,
TARGET_ARCH_MIPS64,
TARGET_ARCH_MIPS64EL,
TARGET_ARCH_MIPSEL,
TARGET_ARCH_OR1K,
TARGET_ARCH_PPC,
TARGET_ARCH_PPC64,
TARGET_ARCH_RISCV32,
TARGET_ARCH_RISCV64,
TARGET_ARCH_RX,
TARGET_ARCH_S390X,
TARGET_ARCH_SH4,
TARGET_ARCH_SH4EB,
TARGET_ARCH_SPARC,
TARGET_ARCH_SPARC64,
TARGET_ARCH_TRICORE,
TARGET_ARCH_X86_64,
TARGET_ARCH_XTENSA,
TARGET_ARCH_XTENSAEB,
} TargetArch;
Effectively duplicates generated enum SysEmuTarget. Can you explain why
that's useful?
In terms of code, it doesn't change anything. we could reuse SysEmuTarget.
I just think it's more clear to have the enum defined next to the
structure using it, compared to have this buried in generated code that
can't be grepped easily.
(git grep SYS_EMU returns nothing, so people have to remember it's
converted from a Json, and that naming is different).
IMHO, DRY principle should not always be followed blindly when it hurts
code readability.
That said, my editor is able to find the generated definition as well,
so I don't really mind reusing SysEmuTarget if we think the code
readability is not worth the duplication.
However, I think it's a problem to compare strings to find this, when it
can be set at compile time, in a file that will have to stay target
specific anyway.
typedef struct TargetInfo {
...
TargetArch target_arch;
...
}
static inline target_arch() {
return target_info()->target_arch;
}
static inline target_aarch64() {
return target_arch() == TARGET_ARCH_AARCH64;
}
In target_info-stub.c:
#ifdef TARGET_AARCH64
# define TARGET_ARCH TARGET_ARCH_AARCH64
#elif TARGET_ARCH_ALPHA
# define TARGET_ARCH TARGET_ARCH_ALPHA
...
#endif
static const TargetInfo target_info_stub = {
...
.target_arch = TARGET_ARCH;
...
}
