[PATCH 05/25] treewide: Correct use of long help
Tom Rini
trini at konsulko.com
Sat Oct 7 19:25:16 CEST 2023
On Sat, Oct 07, 2023 at 09:37:07AM -0600, Simon Glass wrote:
> Hi Tom,
>
> On Fri, 6 Oct 2023 at 19:01, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Fri, Oct 06, 2023 at 04:42:44PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 6 Oct 2023 at 10:55, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Fri, Oct 06, 2023 at 07:03:17AM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Thu, 5 Oct 2023 at 20:16, Tom Rini <trini at konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 05, 2023 at 07:41:49PM -0600, Simon Glass wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Thu, 5 Oct 2023 at 08:53, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Oct 04, 2023 at 07:23:47PM -0600, Simon Glass wrote:
> > > > > > > > > Hi Tom,
> > > > > > > > >
> > > > > > > > > On Sun, 24 Sept 2023 at 17:27, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Sun, Sep 24, 2023 at 02:39:23PM -0600, Simon Glass wrote:
> > > > > > > > > > > Some commands assume that CONFIG_SYS_LONGHELP is always defined.
> > > > > > > > > > > Declaration of long help should be bracketed by an #ifdef to avoid an
> > > > > > > > > > > 'unused variable' warning.
> > > > > > > > > > >
> > > > > > > > > > > Fix this treewide.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > > > [snip]
> > > > > > > > > > > diff --git a/arch/arm/mach-imx/cmd_dek.c b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > > > index 6fa5b41fcd38..25ea7d3b37da 100644
> > > > > > > > > > > --- a/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > > > +++ b/arch/arm/mach-imx/cmd_dek.c
> > > > > > > > > > > @@ -393,11 +393,12 @@ static int do_dek_blob(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > > > > > > > > return blob_encap_dek(src_addr, dst_addr, len);
> > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > -/***************************************************/
> > > > > > > > > > > +#if IS_ENABLED(CONFIG_SYS_LONGHELP)
> > > > > > > > > > > static char dek_blob_help_text[] =
> > > > > > > > > > > "src dst len - Encapsulate and create blob of data\n"
> > > > > > > > > > > " $len bits long at address $src and\n"
> > > > > > > > > > > " store the result at address $dst.\n";
> > > > > > > > > > > +#endif
> > > > > > > > > > >
> > > > > > > > > > > U_BOOT_CMD(
> > > > > > > > > > > dek_blob, 4, 1, do_dek_blob,
> > > > > > > > > >
> > > > > > > > > > This really doesn't read nicely. I would rather (globally and fix
> > > > > > > > > > existing users) __maybe_unused this instead. I think there's just one
> > > > > > > > > > example today that isn't "foo_help_text".
> > > > > > > > >
> > > > > > > > > Hmm, what do you think about adding a __longhelp symbol to cause the
> > > > > > > > > linker to discard it when not needed?
> > > > > > > >
> > > > > > > > Well, I don't think we need linker list magic when __maybe_unused will
> > > > > > > > just have them be discarded normally.
> > > > > > >
> > > > > > > Yes, perhaps things are in a better state than they used to be, but
> > > > > > > there is a linker discard for commands at present.
> > > > > >
> > > > > > Yes, but __maybe_unused means we don't get a warning about it, and it's
> > > > > > literally discarded as part of --gc-sections as it done everywhere with
> > > > > > maybe 3 exceptions?
> > > > >
> > > > > Actually with this series we get a lot closer to that. The problem
> > > > > with the status quo is that disabling CMDLINE doesn't disable most
> > > > > commands, relying instead on discarding them at link time.
> > > >
> > > > I don't follow you here. We're talking only about the long help.
> > >
> > > I was actually talking about how the command code is removed. This
> > > series allows that to happen via Kconfig rather than needing a
> > > linker-script discard rule, something I only just fully appreciated.
> >
> > OK. But this series as-is has a lot of problems and I don't see it as
> > more than a proof of concept.
>
> Probably I need to send a few version as this discussion is becoming a
> bit theoretical.
>
> >
> > > > There's already an option to enable/disable it. When disabled all of
> > > > the long help text should be discarded, because we reference it via
> > > > U_BOOT_CMD macro which in turn cares about it, or not. What's missing
> > > > is a U_BOOT_LONGHELP macro so that instead of:
> > > > #ifdef CONFIG_SYS_LONGHELP
> > > > static char cat_help_text[] =
> > > > "<interface> <dev[:part]> <file>\n"
> > > > " - Print file from 'dev' on 'interface' to standard output\n";
> > > > #endif
> > > >
> > > > We do:
> > > > U_BOOT_LONGHELP(cat,
> > > > "<interface> <dev[:part]> <file>\n"
> > > > " - Print file from 'dev' on 'interface' to standard output\n"
> > > > );
> > > >
> > > > Which then does:
> > > > static __maybe_unused char cat_help_text[] =
> > > > ...
> > > > ;
> > > >
> > > > And we discard the text automatically due to --gc-sections. We possibly
> > > > haven't already been doing this since back when we first started using
> > > > --gc-sections there was a bug in binutils that caused text like the
> > > > above to still get combined in to other objects and not discarded.
> > > > That's been fixed for ages.
> > > >
> > > > And the above macro would also let us clean up U_BOOT_CMD macro slightly
> > > > to just omit the longhelp option and instead always do _CMD_HELP(_name)
> > > > inside the macros. It'll also make it harder to add new commands
> > > > without a long help by accident.
> > >
> > > Gosh this is complicated.
> > >
> > > At present the U_BOOT_CMD() macro just drops the strings...the problem
> > > with the unused var only happens in a small number of cases where an
> > > explicit var is used. I don't see why creating a var (when none is
> > > there today) helps anything? It doesn't detect missing longhelp, since
> > > this is already an error (missing argument). Sure, "" can be
> > > provided, but your macro doesn't stop that either.
> >
> > The problem today is that 95% of the cases surround the help text with
> > #ifdef CONFIG_SYS_LONGHELP ... #endif. That's why the rest of the
> > macros are the way they are today. And that in turn is due to (in part
> > at least) old compiler bugs.
>
> I see about 46 #idefs for that and nearly 300 commands that don't have one.
>
> >
> > > If you are suggesting a new U_BOOT_LONGHELP() macro, bear in mind that
> > > we already have quite a lot...each new 'top-level' macro results in
> > > more combinations.
> >
> > It really should just be a single macro. I think I'll make an attempt
> > at this, to show what I'm talking about.
>
> OK thanks.
>
> >
> > > > > With this series, it looks like we can in fact do that, so I should
> > > > > remove the discards as well.
> > > > >
> > > > > The one proviso is that this does drop a lot of things we want...e.g.
> > > > > BOOTSTD_DEFAULTS cannot be enabled due to its use of 'select', meaning
> > > > > that common filesystems are dropped. So we'll need more effort after
> > > > > this, to allow (for example) bootmeths that don't need commands to
> > > > > work correctly. But I think this series at least provides a better
> > > > > starting point for teasing things apart.
> > > > >
> > > > > So OK I'll go with __maybe_unused for the ones that need it, which
> > > > > isn't too many given that many of the strings are just included
> > > > > directly in the macro. It is considerably easier than trying to be to
> > > > > clever about things.
> > > >
> > > > Yes, this series itself has a lot of problems that need to be addressed
> > > > before reposting because I don't like "hiding" that network stuff no
> > > > longer works, and I also saw that DFU also silently failing.
> > >
> > > Yes I saw your other comments. But I think this patch is the big area
> > > of confusion.
> >
> > This isn't the tricky part of the series that I'm saying has problems,
> > that part is all of the functionality that's not being untangled and I
> > think leads to the question of what exactly is the use case where we do
> > remove the command line but don't need some of that functionality still.
>
> For security and code-size reasons it is useful to disable commands,
> perhaps all of them. Quite a few features don't need it, but it
> certainly would take more effort to get a usable version. The goal of
> this series is to avoid people adding Kconfig mistakes which need to
> be cleaned up later.
Maybe the biggest take away should be to do things smaller. DFU
intentionally and fundamentally constructs something for the CLI parser
to use. Rework that. Same for the network stack. Do some slight
re-org / fixing of more intentional dependencies on their own.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231007/845398a5/attachment.sig>
More information about the U-Boot
mailing list