Hi,
> >> The issue is that disabling TINY_PRINTF may not be possible (size
> >> constraints) and some code is compiled for different stages and people
> >> typically don't check whether the format used in printf is valid with
> >> tiny_printf. I've had this issue already in the past, I vaguely recall
> >> "complaining" about it on IRC.
> >
> > Yes, I've stumbled on "%pa" with tiny printf (i.e. in
> > drivers/pinctrl/pinctrl-single.c) which is printing the very wrong
> > value, actually :) So printing anything unknown as '?' would really
> > help here.
>
> Something like that would help:
> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> index 48db7b1f78f..b918d6d7386 100644
> --- a/lib/tiny-printf.c
> +++ b/lib/tiny-printf.c
> @@ -280,6 +280,12 @@ static int _vprintf(struct printf_info *info, const char
> *fmt, va_list va)
> while (isalnum(fmt[0]))
> fmt++;
> break;
> + } else {
> + if (fmt[0] != '\0' && (fmt[0] == 'a'
> || fmt[0] == 'm' ||
> + fmt[0] == 'M'
> || fmt[0] == 'I')) {
> + fmt++;
> + goto unsupported;
> + }
I wouldn't mind printing the pointer for %p[mMI], but %pa prints the
*content* of the pointer which is really confusing. I.e. in
pinctrl-single.c the reg value pairs are printed like
dev_dbg(dev, "reg/val %pa/0x%08x\n", ®, val);
with reg being a pointer to a physical address. So with tiny_printf
the address of reg (which is a pointer to the stack) is printed in
this case.
I don't think we can print %p without putting more logic into the
decoding. I think the culprit here is the fallthrough to %x, which
then leads to the confusing behavior shown above. IMHO if we want to
avoid that, we'd have to make %p entirely unsupported.
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
index faf55d7f327..8147ffa2c1b 100644
--- a/lib/tiny-printf.c
+++ b/lib/tiny-printf.c
@@ -269,21 +269,18 @@ static int _vprintf(struct printf_info *info, const char
*fmt, va_list va)
div_out(info, &num, div);
}
break;
+#if CONFIG_IS_ENABLED(NET) || CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG
case 'p':
- if (CONFIG_IS_ENABLED(NET) ||
- CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG) {
- pointer(info, fmt, va_arg(va, void *));
- /*
- * Skip this because it pulls in _ctype
which is
- * 256 bytes, and we don't generally
implement
- * pointer anyway
- */
- while (isalnum(fmt[0]))
- fmt++;
- break;
- }
- islong = true;
- /* no break */
+ pointer(info, fmt, va_arg(va, void *));
+ /*
+ * Skip this because it pulls in _ctype which is
+ * 256 bytes, and we don't generally implement
+ * pointer anyway
+ */
+ while (isalnum(fmt[0]))
+ fmt++;
+ break;
+#endif
case 'x':
if (islong) {
num = va_arg(va, unsigned long);
@@ -310,6 +307,8 @@ static int _vprintf(struct printf_info *info, const char
*fmt, va_list va)
case '%':
out(info, '%');
default:
+ out(info, '?');
break;
}
-michael
signature.asc
Description: PGP signature
