Re: [PATCH 05/25] treewide: Correct use of long help

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 7521 bytes --]

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@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@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@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@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@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.

> > 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.

> 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.

> > > 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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]