Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Snprintk used at many place while dummy build if CONFIG_PRINTK is undef #24181

Closed
XavierChapron opened this issue Apr 8, 2020 · 6 comments
Closed
Assignees
Labels
area: Console bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@XavierChapron
Copy link
Contributor

XavierChapron commented Apr 8, 2020

Describe the bug
snprintk() defined in include/sys/printk.h has a dummy version in case CONFIG_PRINTK is not defined:

static inline __printf_like(3, 4) int snprintk(char *str, size_t size,
					       const char *fmt, ...)
{
	ARG_UNUSED(str);
	ARG_UNUSED(size);
	ARG_UNUSED(fmt);
	return 0;
}

However, it is used in many places (git grep snprintk | wc gives 213 matchs), including some that must not be "optimized" when logs are removed:

  • it is used to create AT commands in ublox-sara-r4, wncm14a2a, esp drivers
  • it is used to create JSON payload in lib/os/json.c
  • it is used in many Bluetooth related functions.

To Reproduce
Build any project without CONFIG_PRINTK.
To be sure, that it's not re-enable by (CONFIG_BOOT_BANNER for example) also remove CONFIG_CONSOLE and CONFIG_LOG.
See that snprintk() as been "optimized" in generated .map file.

Expected behavior
snprintk() should either be:

  • Always built with a full implementation.
  • Change snprintk() dummy implementation to be a trampoline to snprintf().
  • Not exposed in printk.h and only used in functions already optimized when CONFIG_PRINTK is not defined. This will force to change every call to snprintk() by snprintf().

Impact
This become really dangerous if CONFIG_PRINTK is not defined.

Environment :

  • On Zephyr master at the time of writtting this (SHA: 5b991aa)
@XavierChapron XavierChapron added the bug The issue is a bug, or the PR is fixing a bug label Apr 8, 2020
@XavierChapron
Copy link
Contributor Author

XavierChapron commented Apr 8, 2020

Currently both snprintf() and snprintk() are used.
This leads to having both functions in generated binary which is an unnecessary duplicate increasing flash footprint.

Should this be in this issue or in another one?

@jukkar
Copy link
Member

jukkar commented Apr 8, 2020

Should this be in this issue or in another one?

Sounds like a separate issue to me.

IMHO best option would be to always have a full implementation of snprintk().

@XavierChapron
Copy link
Contributor Author

Should this be in this issue or in another one?

Sounds like a separate issue to me.

IMHO best option would be to always have a full implementation of snprintk().

Thank for your reply.
I have created a separated issue.

@carlescufi
Copy link
Member

@nashif found this commit which should've decoupled CONFIG_PRINTK and snprintk. d76ae46

@andrewboie
Copy link
Contributor

wait a second...snprintk does not depend on CONFIG_PRINTK

what's the actual problem here?

static inline __printf_like(3, 4) int snprintk(char *str, size_t size,
					       const char *fmt, ...)
{
	ARG_UNUSED(str);
	ARG_UNUSED(size);
	ARG_UNUSED(fmt);
	return 0;
}

I just checked and this code doesn't exist.

Are you on some old version of Zephyr?

@XavierChapron
Copy link
Contributor Author

@andrewboie You are correct, I've not correctly checked zephyr last version and therefore I've not see your commit that is fixing this issue. I'm sorry for the annoyance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Console bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

No branches or pull requests

5 participants