[PATCH 05/25] treewide: Correct use of long help
Simon Glass
sjg at chromium.org
Sat Oct 7 17:37:07 CEST 2023
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.
Regards,
Simon
More information about the U-Boot
mailing list