[PATCH v6 10/12] lib: sbi: Add print_info for sbi_console.c
Anup Patel
anup at brainfault.org
Tue Jul 4 03:34:23 PDT 2023
On Tue, Jul 4, 2023 at 3:19 PM Xiang W <wxjstz at 126.com> wrote:
>
> 在 2023-07-04星期二的 11:27 +0530，Anup Patel写道：
> > On Mon, Jun 12, 2023 at 1:40 PM Xiang W <wxjstz at 126.com> wrote:
> > >
> > > Create a structure print_info for print, this structure simplifies
> > > parameter passing.
> >
> > I don't see any advantage of this new structure.
> >
> > NACK to this patch.
> printi is reduced from 6 to 2 parameters
> prints is reduced from 5 to 2 parameters
> processing of pc reduced from 21 to 8
>
> This patch simplifies parameter passing. Because of this, the size
> of the code will also be reduced.
>
> After applying the patch
> size build/lib/sbi/sbi_console.o
> text data bss dec hex filename
> 3329 0 272 3601 e11 build/lib/sbi/sbi_console.o
>
> befor applying the patch
> size build/lib/sbi/sbi_console.o
> text data bss dec hex filename
> 3559 0 272 3831 ef7 build/lib/sbi/sbi_console.o
Mixing parameters across functions into a struct is a NOGO
from myside.
Still NACK.
Regards,
Anup
>
> Regards,
> Xiang W
> >
> > Regards,
> > Anup
> >
> > >
> > > Signed-off-by: Xiang W <wxjstz at 126.com>
> > > ---
> > > lib/sbi/sbi_console.c | 235 +++++++++++++++++++++---------------------
> > > 1 file changed, 120 insertions(+), 115 deletions(-)
> > >
> > > diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> > > index af93f51..c227b0f 100644
> > > --- a/lib/sbi/sbi_console.c
> > > +++ b/lib/sbi/sbi_console.c
> > > @@ -27,11 +27,22 @@
> > >
> > > typedef __builtin_va_list va_list;
> > >
> > > +struct print_info {
> > > + char *out;
> > > + int len;
> > > + int pos;
> > > + int pc;
> > > + int flags;
> > > + int width;
> > > + int wcnt;
> > > + char type;
> > > +};
> > > +
> > > static const struct sbi_console_device *console_dev = NULL;
> > > static char console_tbuf[CONSOLE_TBUF_MAX];
> > > -static u32 console_tbuf_len;
> > > static spinlock_t console_out_lock = SPIN_LOCK_INITIALIZER;
> > >
> > > +
> > > bool sbi_isprintable(char c)
> > > {
> > > if (((31 < c) && (c < 127)) || (c == '\f') || (c == '\r') ||
> > > @@ -128,70 +139,63 @@ unsigned long sbi_ngets(char *str, unsigned long len)
> > > return i;
> > > }
> > >
> > > -static void printc(char **out, u32 *out_len, char ch)
> > > +static void printc(struct print_info *info, char ch)
> > > {
> > > - if (!out) {
> > > + /* output to the console */
> > > + if (info->out == NULL) {
> > > sbi_putc(ch);
> > > + info->pc++;
> > > return;
> > > }
> > >
> > > - /*
> > > - * The *printf entry point functions have enforced that (*out) can
> > > - * only be null when out_len is non-null and its value is zero.
> > > - */
> > > - if (!out_len || *out_len > 1) {
> > > - *(*out)++ = ch;
> > > - **out = '\0';
> > > - }
> > > + /* unknown buffer length */
> > > + if (info->len == 0)
> > > + goto append;
> > >
> > > - if (out_len && *out_len > 0)
> > > - --(*out_len);
> > > + /* The buffer is long enough to add new characters */
> > > + if (info->len - info->pos > 1)
> > > + goto append;
> > > + return;
> > > +
> > > +append:
> > > + info->out[info->pos++] = ch;
> > > + info->out[info->pos] = '\0';
> > > + info->pc++;
> > > }
> > >
> > > -static int prints(char **out, u32 *out_len, const char *string, int width,
> > > - int flags)
> > > +static void prints(struct print_info *info, const char *string)
> > > {
> > > - int pc = 0;
> > > - width -= sbi_strlen(string);
> > > - if (!(flags & PAD_RIGHT)) {
> > > - for (; width > 0; --width) {
> > > - printc(out, out_len, flags & PAD_ZERO ? '0' : ' ');
> > > - ++pc;
> > > - }
> > > - }
> > > - for (; *string; ++string) {
> > > - printc(out, out_len, *string);
> > > - ++pc;
> > > - }
> > > - for (; width > 0; --width) {
> > > - printc(out, out_len, ' ');
> > > - ++pc;
> > > + info->wcnt += sbi_strlen(string);
> > > + if (!(info->flags & PAD_RIGHT)) {
> > > + for (; info->wcnt < info->width; info->wcnt++)
> > > + printc(info, info->flags & PAD_ZERO ? '0' : ' ');
> > > }
> > > -
> > > - return pc;
> > > + for (; *string; ++string)
> > > + printc(info, *string);
> > > + for (; info->wcnt < info->width; info->wcnt++)
> > > + printc(info, ' ');
> > > }
> > >
> > > -static int printi(char **out, u32 *out_len, long long i,
> > > - int width, int flags, int type)
> > > +static void printi(struct print_info *info, long long i)
> > > {
> > > - int pc = 0;
> > > char *s, sign, letbase, print_buf[PRINT_BUF_LEN];
> > > unsigned long long u, b, t;
> > >
> > > b = 10;
> > > letbase = 'a';
> > > - if (type == 'o')
> > > + if (info->type == 'o')
> > > b = 8;
> > > - else if (type == 'x' || type == 'X' || type == 'p' || type == 'P') {
> > > + else if (info->type == 'x' || info->type == 'X'
> > > + || info->type == 'p' || info->type == 'P') {
> > > b = 16;
> > > letbase &= ~0x20;
> > > - letbase |= type & 0x20;
> > > + letbase |= info->type & 0x20;
> > > }
> > >
> > > u = i;
> > > sign = 0;
> > > - if (type == 'i' || type == 'd') {
> > > - if ((flags & PAD_SIGN) && i > 0)
> > > + if (info->type == 'i' || info->type == 'd') {
> > > + if ((info->flags & PAD_SIGN) && i > 0)
> > > sign = '+';
> > > if (i < 0) {
> > > sign = '-';
> > > @@ -214,26 +218,23 @@ static int printi(char **out, u32 *out_len, long long i,
> > > }
> > > }
> > >
> > > - if (flags & PAD_ZERO) {
> > > + if (info->flags & PAD_ZERO) {
> > > if (sign) {
> > > - printc(out, out_len, sign);
> > > - ++pc;
> > > - --width;
> > > + printc(info, sign);
> > > + ++info->wcnt;
> > > }
> > > - if (i && (flags & PAD_ALTERNATE)) {
> > > + if (i && (info->flags & PAD_ALTERNATE)) {
> > > if (b == 16 || b == 8) {
> > > - printc(out, out_len, '0');
> > > - ++pc;
> > > - --width;
> > > + printc(info, '0');
> > > + ++info->wcnt;
> > > }
> > > if (b == 16) {
> > > - printc(out, out_len, 'x' - 'a' + letbase);
> > > - ++pc;
> > > - --width;
> > > + printc(info, 'x' - 'a' + letbase);
> > > + ++info->wcnt;
> > > }
> > > }
> > > } else {
> > > - if (i && (flags & PAD_ALTERNATE)) {
> > > + if (i && (info->flags & PAD_ALTERNATE)) {
> > > if (b == 16)
> > > *--s = 'x' - 'a' + letbase;
> > > if (b == 16 || b == 8)
> > > @@ -243,15 +244,16 @@ static int printi(char **out, u32 *out_len, long long i,
> > > *--s = sign;
> > > }
> > >
> > > - return pc + prints(out, out_len, s, width, flags);
> > > + prints(info, s);
> > > }
> > >
> > > -static int print(char **out, u32 *out_len, const char *format, va_list args)
> > > +static void print(struct print_info *info, const char *format, va_list args)
> > > {
> > > bool flags_done;
> > > - int width, flags, pc = 0;
> > > - char type, scr[2], *tout;
> > > - bool use_tbuf = (!out) ? true : false;
> > > + char scr[2];
> > > + bool use_tbuf = (!info->out) ? true : false;
> > > + info->pos = 0;
> > > + info->pc = 0;
> > >
> > > /*
> > > * The console_tbuf is protected by console_out_lock and
> > > @@ -259,19 +261,15 @@ static int print(char **out, u32 *out_len, const char *format, va_list args)
> > > * when out == NULL.
> > > */
> > > if (use_tbuf) {
> > > - console_tbuf_len = CONSOLE_TBUF_MAX;
> > > - tout = console_tbuf;
> > > - out = &tout;
> > > - out_len = &console_tbuf_len;
> > > + info->out = console_tbuf;
> > > + info->len = CONSOLE_TBUF_MAX;
> > > }
> > >
> > > for (; *format != 0; ++format) {
> > > - if (use_tbuf && !console_tbuf_len) {
> > > - nputs_all(console_tbuf, CONSOLE_TBUF_MAX);
> > > - console_tbuf_len = CONSOLE_TBUF_MAX;
> > > - tout = console_tbuf;
> > > - }
> > > -
> > > + if (use_tbuf && (info->len - info->pos <= 1)) {
> > > + nputs_all(info->out, info->pos);
> > > + info->pos = 0;
> > > + }
> > > if (*format == '%') {
> > > ++format;
> > > if (*format == '\0')
> > > @@ -279,21 +277,21 @@ static int print(char **out, u32 *out_len, const char *format, va_list args)
> > > if (*format == '%')
> > > goto literal;
> > > /* Get flags */
> > > - flags = 0;
> > > + info->flags = 0;
> > > flags_done = false;
> > > while (!flags_done) {
> > > switch (*format) {
> > > case '-':
> > > - flags |= PAD_RIGHT;
> > > + info->flags |= PAD_RIGHT;
> > > break;
> > > case '+':
> > > - flags |= PAD_SIGN;
> > > + info->flags |= PAD_SIGN;
> > > break;
> > > case '#':
> > > - flags |= PAD_ALTERNATE;
> > > + info->flags |= PAD_ALTERNATE;
> > > break;
> > > case '0':
> > > - flags |= PAD_ZERO;
> > > + info->flags |= PAD_ZERO;
> > > break;
> > > case ' ':
> > > case '\'':
> > > @@ -306,153 +304,160 @@ static int print(char **out, u32 *out_len, const char *format, va_list args)
> > > if (!flags_done)
> > > ++format;
> > > }
> > > - if (flags & PAD_RIGHT)
> > > - flags &= ~PAD_ZERO;
> > > + if (info->flags & PAD_RIGHT)
> > > + info->flags &= ~PAD_ZERO;
> > > /* Get width */
> > > - width = 0;
> > > + info->width = 0;
> > > + info->wcnt = 0;
> > > for (; *format >= '0' && *format <= '9'; ++format) {
> > > - width *= 10;
> > > - width += *format - '0';
> > > + info->width *= 10;
> > > + info->width += *format - '0';
> > > }
> > > + info->type = *format;
> > > if (*format == 's') {
> > > char *s = va_arg(args, char *);
> > > - pc += prints(out, out_len, s ? s : "(null)",
> > > - width, flags);
> > > + prints(info, s ? s : "(null)");
> > > continue;
> > > }
> > > if ((*format == 'd') || (*format == 'i')) {
> > > - pc += printi(out, out_len, va_arg(args, int),
> > > - width, flags, *format);
> > > + printi(info, va_arg(args, int));
> > > continue;
> > > }
> > > if ((*format == 'u') || (*format == 'o')
> > > || (*format == 'x') || (*format == 'X')) {
> > > - pc += printi(out, out_len, va_arg(args, unsigned int),
> > > - width, flags, *format);
> > > + printi(info, va_arg(args, unsigned int));
> > > continue;
> > > }
> > > if ((*format == 'p') || (*format == 'P')) {
> > > - pc += printi(out, out_len, (uintptr_t)va_arg(args, void*),
> > > - width, flags, *format);
> > > + printi(info, (uintptr_t)va_arg(args, void*));
> > > continue;
> > > }
> > > if (*format == 'l') {
> > > - type = 'i';
> > > + info->type = 'i';
> > > if (format[1] == 'l') {
> > > ++format;
> > > if ((format[1] == 'u') || (format[1] == 'o')
> > > || (format[1] == 'd') || (format[1] == 'i')
> > > || (format[1] == 'x') || (format[1] == 'X')) {
> > > ++format;
> > > - type = *format;
> > > + info->type = *format;
> > > }
> > > - pc += printi(out, out_len, va_arg(args, long long),
> > > - width, flags, type);
> > > + printi(info, va_arg(args, long long));
> > > continue;
> > > }
> > > if ((format[1] == 'u') || (format[1] == 'o')
> > > || (format[1] == 'd') || (format[1] == 'i')
> > > || (format[1] == 'x') || (format[1] == 'X')) {
> > > ++format;
> > > - type = *format;
> > > + info->type = *format;
> > > }
> > > - if ((type == 'd') || (type == 'i'))
> > > - pc += printi(out, out_len, va_arg(args, long),
> > > - width, flags, type);
> > > + if ((info->type == 'd') || (info->type == 'i'))
> > > + printi(info, va_arg(args, long));
> > > else
> > > - pc += printi(out, out_len, va_arg(args, unsigned long),
> > > - width, flags, type);
> > > + printi(info, va_arg(args, unsigned long));
> > > continue;
> > > }
> > > if (*format == 'c') {
> > > /* char are converted to int then pushed on the stack */
> > > scr[0] = va_arg(args, int);
> > > scr[1] = '\0';
> > > - pc += prints(out, out_len, scr, width, flags);
> > > + prints(info, scr);
> > > continue;
> > > }
> > > } else {
> > > literal:
> > > - printc(out, out_len, *format);
> > > - ++pc;
> > > + printc(info, *format);
> > > }
> > > }
> > >
> > > - if (use_tbuf && console_tbuf_len < CONSOLE_TBUF_MAX)
> > > - nputs_all(console_tbuf, CONSOLE_TBUF_MAX - console_tbuf_len);
> > > -
> > > - return pc;
> > > + if (use_tbuf && info->pos > 0)
> > > + nputs_all(info->out, info->pos);
> > > }
> > >
> > > int sbi_sprintf(char *out, const char *format, ...)
> > > {
> > > va_list args;
> > > - int retval;
> > > + struct print_info info;
> > > + info.out = out;
> > > + info.len = 0;
> > > +
> > >
> > > if (unlikely(!out))
> > > sbi_panic("sbi_sprintf called with NULL output string\n");
> > >
> > > va_start(args, format);
> > > - retval = print(&out, NULL, format, args);
> > > + print(&info, format, args);
> > > va_end(args);
> > >
> > > - return retval;
> > > + return info.pc;
> > > }
> > >
> > > int sbi_snprintf(char *out, u32 out_sz, const char *format, ...)
> > > {
> > > va_list args;
> > > - int retval;
> > > + struct print_info info;
> > > + info.out = out;
> > > + info.len = out_sz;
> > > +
> > >
> > > if (unlikely(!out && out_sz != 0))
> > > sbi_panic("sbi_snprintf called with NULL output string and "
> > > "output size is not zero\n");
> > >
> > > va_start(args, format);
> > > - retval = print(&out, &out_sz, format, args);
> > > + print(&info, format, args);
> > > va_end(args);
> > >
> > > - return retval;
> > > + return info.pc;
> > > }
> > >
> > > int sbi_printf(const char *format, ...)
> > > {
> > > va_list args;
> > > - int retval;
> > > + struct print_info info;
> > > + info.out = NULL;
> > > + info.len = 0;
> > > +
> > >
> > > spin_lock(&console_out_lock);
> > > va_start(args, format);
> > > - retval = print(NULL, NULL, format, args);
> > > + print(&info, format, args);
> > > va_end(args);
> > > spin_unlock(&console_out_lock);
> > >
> > > - return retval;
> > > + return info.pc;
> > > }
> > >
> > > int sbi_dprintf(const char *format, ...)
> > > {
> > > va_list args;
> > > - int retval = 0;
> > > struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
> > > + struct print_info info;
> > > + info.out = NULL;
> > > + info.len = 0;
> > > +
> > >
> > > va_start(args, format);
> > > if (scratch->options & SBI_SCRATCH_DEBUG_PRINTS) {
> > > spin_lock(&console_out_lock);
> > > - retval = print(NULL, NULL, format, args);
> > > + print(&info, format, args);
> > > spin_unlock(&console_out_lock);
> > > }
> > > va_end(args);
> > >
> > > - return retval;
> > > + return info.pc;
> > > }
> > >
> > > void sbi_panic(const char *format, ...)
> > > {
> > > va_list args;
> > > + struct print_info info;
> > > + info.out = NULL;
> > > + info.len = 0;
> > >
> > > spin_lock(&console_out_lock);
> > > va_start(args, format);
> > > - print(NULL, NULL, format, args);
> > > + print(&info, format, args);
> > > va_end(args);
> > > spin_unlock(&console_out_lock);
> > >
> > > --
> > > 2.39.2
> > >
>
More information about the opensbi
mailing list