lkml.org 
[lkml]   [2022]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2022-03-03 11:23    [W:4.000 / U:1.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site