On Tue, Mar 20, 2018 at 1:51 PM, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> Le mar. 20 mars 2018 12:52, Peter Maydell <peter.mayd...@linaro.org> a > écrit : > >> On 19 March 2018 at 21:18, Michael Clark <m...@sifive.com> wrote: >> > This version uses a constant size memory buffer sized for >> > the maximum possible ISA string length. It also uses g_new >> > instead of g_new0, uses more efficient logic to append >> > extensions and adds manual zero termination of the string. >> > >> > Cc: Palmer Dabbelt <pal...@sifive.com> >> > Cc: Peter Maydell <peter.mayd...@linaro.org> >> > Signed-off-by: Michael Clark <m...@sifive.com> >> > Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org> >> > --- >> > target/riscv/cpu.c | 12 ++++++------ >> > 1 file changed, 6 insertions(+), 6 deletions(-) >> > >> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> > index 1f25968..c82359f 100644 >> > --- a/target/riscv/cpu.c >> > +++ b/target/riscv/cpu.c >> > @@ -360,16 +360,16 @@ static void riscv_cpu_class_init(ObjectClass *c, >> void *data) >> > char *riscv_isa_string(RISCVCPU *cpu) >> > { >> > int i; >> > - size_t maxlen = 5 + ctz32(cpu->env.misa); >> > - char *isa_string = g_new0(char, maxlen); >> > - snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS); >> > + const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1; >> > + char *isa_str = g_new(char, maxlen); >> > + char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", >> TARGET_LONG_BITS); >> > for (i = 0; i < sizeof(riscv_exts); i++) { >> > if (cpu->env.misa & RV(riscv_exts[i])) { >> > - isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a'; >> > - >> > + *p++ = tolower(riscv_exts[i]); >> >> This should be qemu_tolower(). Plain tolower() has an awkward problem >> where if the 'char' type is signed and you hand tolower() a char that >> happens to be a negative value you get undefined behaviour. This means >> you need to cast the argument to 'unsigned char', which is what our >> > > Oh, good to know. > > wrapper does. In this specific case the inputs are known constant >> ASCII, so tolower() happens to be safe, > > > Yep. > > but for consistency with >> the rest of the codebase we should use our wrapper function. >> > > Ok. > > >> > } >> > } >> > - return isa_string; >> > + *p = '\0'; >> > + return isa_str; >> > } >> > >> > typedef struct RISCVCPUListState { >> >> Since this bug is causing the build tests to intermittently fail, >> I'm going to apply this patch directly to master, with tolower() >> replaced with qemu_tolower(). >> > > Thanks Peter! > Okay. I'll drop the patch from my post-merge spec conformance fixes series. I've addressed all feedback on the post-merge spec conformance series so i'll make a PR for it... ... these are the extra commits that were erroneously included in the v8.2 pull. They now all have sign-offs... they've been in the riscv tree for a while and have had a lot of testing...