Messages in this thread | | | Date | Thu, 03 Mar 2022 10:22:59 +0000 | From | Paul Cercueil <> | Subject | Re: [PATCH v3] serial: make uart_console_write->putchar()'s character an unsigned char |
| |
Hi Maciej,
Le jeu., mars 3 2022 at 09:55:17 +0000, Maciej W. Rozycki <macro@orcam.me.uk> a écrit : > On Thu, 3 Mar 2022, Jiri Slaby wrote: > >> > The real problem is that using char (or short) for a function >> parameter >> > or result is very likely to require the compile add code to mask >> > the value to 8 (or 16) bits. >> > >> > Remember that almost every time you do anything with a signed or >> unsigned >> > char/short variable the compiler has to use the integer promotion >> rules >> > to convert the value to int. >> > >> > You'll almost certainly get better code if the value is left in an >> > int (or unsigned int) variable until the low 8 bits get written to >> > a buffer (or hardware register). >> >> So should we use int/uint instead of more appropriate shorter types >> everywhere >> now? The answer is: definitely not. The assembly on x86 looks good >> (it uses >> movz, no ands), RISC architectures have to do what they chose to. > > We do have an issue, because we still have this: > > void uart_console_write(struct uart_port *port, const char *s, > unsigned int count, > void (*putchar)(struct uart_port *, int)) > > and then: > > putchar(port, *s); > > there. Consequently on targets where plain `char' type is signed the > value retrieved from `*s' has to be truncated in the call to > `putchar'. > And indeed it happens with the MIPS target: > > 803ae47c: 82050000 lb a1,0(s0) > 803ae480: 26100001 addiu s0,s0,1 > 803ae484: 02402025 move a0,s2 > 803ae488: 0220f809 jalr s1 > 803ae48c: 30a500ff andi a1,a1,0xff > > vs current code: > > 803ae47c: 82050000 lb a1,0(s0) > 803ae480: 26100001 addiu s0,s0,1 > 803ae484: 0220f809 jalr s1 > 803ae488: 02402025 move a0,s2
And how is that at all a problem?
> (NB the last instruction shown after the call instruction, JALR, is > in the > delay slot that is executed before the PC gets updated). Now > arguably the > compiler might notice that and use an unsigned LBU load instruction > rather > than the signed LB load instruction, which would make the ANDI > instruction > redundant, but still I think we ought to avoid gratuitous type > signedness > changes. > > So I'd recommend changing `s' here to `const unsigned char *' or, as > I > previously suggested, maybe to `const u8 *' even.
Just cast the string to "const u8 *" within the function, while keeping a "const char *s" argument. The compiler will then most likely generate LBUs.
Cheers, -Paul
| |