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

lib/os: fix ll format specifier in z_prf #29171

Closed
wants to merge 2 commits into from

Conversation

pabigot
Copy link
Collaborator

@pabigot pabigot commented Oct 13, 2020

Transfer the missed Kconfig that controls support for %ll? format specifier in z_prf out of minimal libc and into the lib/os Kconfig area. Give it a better name, since it's not in minimal libc anymore.

Also add just enough of a test case to verify this works.

Fixes #29165

When z_prf moved from minimal libc to lib os a Kconfig option was
missed, causing loss of the ability to display 64-bit values in the
shell.  Put it back with a better name.

Signed-off-by: Peter Bigot <[email protected]>
@pabigot
Copy link
Collaborator Author

pabigot commented Oct 13, 2020

@sdalu

@github-actions github-actions bot added area: C Library C Standard Library area: Tests Issues related to a particular existing or missing test labels Oct 13, 2020
Just enough to confirm that formating long-long works.

Signed-off-by: Peter Bigot <[email protected]>
@andrewboie
Copy link
Contributor

I must have missed the patch that moved z_prf out of minimal libc.
Couldn't we have worked around this in the shell instead?? Or used printk()?? Anything but this terrible arrangement.
Now if you use a third-party C library, your code could pull in printk(), z_prf() and whatever is in that C library for printf. not good.
I really think you should reconsider all of this.

@sdalu
Copy link
Contributor

sdalu commented Oct 13, 2020

that's fixing my problem.
(i've no idea on what is / would be the best way of fixing it)

@pabigot
Copy link
Collaborator Author

pabigot commented Oct 13, 2020

Couldn't we have worked around this in the shell instead?? Or used printk()??

@andrewboie Not really. The requirements and rationale are in #27655: only z_prf() and z_vprintk() have the interface necessary, and of those z_prf() is far more powerful.

This only affects things that explicitly use z_prf(), which is the shell, the log subsystem, and for minimal libc the *printf functions. It should have no impact on using third-party C code.

@andrewboie
Copy link
Contributor

The shell requires a *printf function that produces its output by calling a function like putchar(3c) to output one character at a time. This eliminates dynamic or static allocation of buffers large enough to hold arbitrary messages.

At any point did anyone consider that maybe we should change the shell code to not have this inflexible requirement such that any C library could be used, instead of us now having to support and maintain TWO printf implementations in the core kernel itself?

@pabigot
Copy link
Collaborator Author

pabigot commented Oct 14, 2020

From #27655 I don't see how to change the shell code: the shell requires generating output to arbitrary destinations, and there is no way to do that using the C standard library. z_prf() or something just like it is needed, v_printk() is "just like it" but is too limited.

If we want to eliminate duplicate code, we could convert v_printk() to be a wrapper around z_prf().

In any case such discussion is not directly related to this PR which fixes a bug.

@andrewboie
Copy link
Contributor

the shell requires generating output to arbitrary destinations

So add some buffering or re-work the code so this isn't a requirement?
Maintaining two different printf implementations in the Zephyr codebase is not the way to go.

If we want to eliminate duplicate code, we could convert v_printk() to be a wrapper around z_prf().

Not happening. z_prf uses far more stack space.

You should really think about:

  1. What could be changed in the shell to allow C standard library functions to be used
  2. Do you REALLY need the extra formatting that z_prf supports and vprintk() doesn't have

In other words, start over from first principles. IMO the other patch should be reverted. This is a mess.

@andrewboie
Copy link
Contributor

I will provide you with a path forward that I would find acceptable:
Remove all dependencies on z_prf.
Implement this in terms of printk() family of functions instead.
If we must have float, find a way to add a Kconfig or something that adds optional float support to printk, in a way that leaves footprint and stack usage unchanged (give or take a few bytes) with the config turned off.
But depending on z_prf this must stop.

@pabigot pabigot added the dev-review To be discussed in dev-review meeting label Oct 14, 2020
@pabigot
Copy link
Collaborator Author

pabigot commented Oct 14, 2020

Added to dev-review for tomorrow @galak.

I have some sympathy with @andrewboie's concerns, but do not have time or interest in taking on other solution paths. My acceptable paths are (1) take this fix, (2) revert #27655 breaking shell display of floating point depending on which libc the app happens to link with, or (3) close this unmerged leaving the 64-bit shell printing broken.

Changes like re-architecting the shell or trying to make vprintk() support both kernel and application needs are not in my wheelhouse and somebody else can pick them up after one of the above choices is taken. If no decision is reached the default path would be status quo (option 3).

@andrewboie
Copy link
Contributor

(2) revert #27655 breaking shell display of floating point depending on which libc the app happens to link with,

I already sent a PR that does exactly this #29203

Added to dev-review for tomorrow @galak.

What is the point? @galak does not own lib/os. That is owned by me and Andy. This isn't up for discussion.

somebody else can pick them up

So be it.

@pabigot
Copy link
Collaborator Author

pabigot commented Oct 14, 2020

I already sent a PR that does exactly this #29203

OK, didn't see that anywhere.

Added to dev-review for tomorrow @galak.

What is the point? @galak does not own lib/os. That is owned by me and Andy. This isn't up for discussion.

We had been using dev-review to get a wider perspective, but yes, I do agree that maintainers get to make decisions in the face of a lack of consensus. So we'll go with option 2.

@pabigot pabigot closed this Oct 14, 2020
@nashif
Copy link
Member

nashif commented Oct 14, 2020

What is the point? @galak does not own lib/os. That is owned by me and Andy. This isn't up for discussion.

We had been using dev-review to get a wider perspective, but yes, I do agree that maintainers get to make decisions in the face of a lack of consensus. So we'll go with option 2.

We should try as much as possible to close on such issues here and in related PRs and issues. Not everyone can join meetings and we should not use meetings for things that can be closed on GH.

What is the point? @galak does not own lib/os.

@galak is just the facilitator of the meeting, do not shoot the messenger :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: C Library C Standard Library area: Tests Issues related to a particular existing or missing test dev-review To be discussed in dev-review meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shell_print doesn't support anymore %llx when used with newlib
4 participants