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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > > > > > 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 <[email protected]> > > > > > > > > [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
signature.asc
Description: PGP signature
