Re: [PATCH v11 00/20] Compile-time stack validation

[Ingo Molnar <mingo@kernel.org>]

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> > > My feeling is that the subcommand model wouldn't fit this tool very well.  
> > > Its core functionality is to analyze code paths -- which it does in a single 
> > > pass, regardless of whether it's checking frame pointers, checking CFI, 
> > > generating CFI, or some combination.  Splitting it up into subcommands would 
> > > mean having to repeat the same code analysis pass multiple times 
> > > unnecessarily.
> > 
> > Huh?
> > 
> > The subcommand approach is a user UI that does not limit the tool in any way: 
> > you are free to provide subcommands that combine more atomic functionality - 
> > similarly to how Git provides a 'git pull' subcommand that is a combination of 
> > 'fetch' and 'merge' steps.
> 
> Sure, but it doesn't scale if *all* the subcommands are combinable.  For n 
> subcommands which can be combined, you'd need (2^n - 1) total subcommands to 
> cover all possible combinations.  In that case, subcommands would be much more 
> unwieldy than just having n flags that can be easily combined.

I think there's some misunderstanding here. Initially you only need a single 
subcommand, 'check'. With that if the main subcommand for checking is:

  debuginfo check

then you can still add options after the 'check' subcommand if you think it's more 
intuitive - or list them as sub-subcommands - which is generally more intuitive to 
humans:

  debuginfo check cfi fp

or:

  debuginfo check all

See below for more complex examples:

> This is an important point because I think any hypothetical future options would 
> be likely to be combinable if they take advantage of the tool's main 
> functionality, which is walking all the code paths.  If they don't take 
> advantage of that, they should probably be in a separate tool anyway.

It's a simple option string namespace - look at how tools/perf/ is using it, it's 
very flexible.

> > In this case it would be a simple:
> > 
> >    debuginfo check all
> > 
> > to check everything. You can also make the selection of debuginfo components 
> > to check a regular option, not a subcommand.
> 
> The reason I proposed a name change is that it will soon do *more* than just 
> checking.  It will also do CFI generation by modifying the object file.
> 
> What subcommand would you suggest for the following?
> 
> - do frame pointer validation; and
> 
> - if CFI exists, do CFI validation, else do CFI generation.

The main functionality here is to fix up the CFI info, so I'd name it:

   debuginfo fix cfi

where the 'fix' subcommand would use functionality from the 'check' subcommand to 
see whether there's CFI info present (and if yes, sanity check it and warn if it's 
not good).
 
perf does this all the time: for example 'perf top', 'perf report' and 'perf 
annotate' deeply share functionality. Since under the hood it's all one single 
binary, it's all very easy and intuitive to do.

> > etc. By limiting the name at inception unreasonably you make all these things 
> > less obvious to add.
> 
> But note these examples are still related to stacks, so having "stack" in the 
> name of the tool wouldn't be limiting (for these examples at least).

Absolutely, I'd name it 'debuginfo' at minimum to not unnecessarily limit things 
at the inception of the tool with 'stackfix'.

> I proposed the "fix" in "stackfix" because it will do more than just checking: 
> it will also be able to modify the object file (as I describe above).  And 
> "stack" because thus far the proposed scope of the tool is strictly related to 
> stacks.
> 
> I think "debuginfo" is limiting in its own way.  The core functionality of the 
> tool is to analyze all possible code paths, which isn't directly related to 
> debuginfo.  We might want to do other kinds of code path analysis which are 
> unrelated to debuginfo.

So if you can think of an even more generic name than 'debuginfo', that would be 
even better - what I objected to was the limiting 'stackfix' name.

For example 'binary' might work well too, here's a few mockup subcommands:

   binary check fp                   # checks framepointers in a binary
   binary check all                  # checks everything it can in a binary
   binary generate cfi               # generates CFI info
   binary ls                         # prints section sizes
   binary compress                   # strip out NOPs and other padding from a binary if possible

(But 'fix' instead of 'generate' would work as well.)

Note how intuitive the wording it, it's almost a free flowing English sentence.

> For example, the tool could have a replacement for "make checkstack", which 
> generates a list of functions which are stack hogs.  That has nothing to do with 
> debuginfo.

That's actually a powerful example of how subcommands would work naturally:

  binary check cfi fp stacksize

see how it's checking various aspects of an executable?

Note that per Git and perf option parsing only the first word after 'debuginfo' is 
a subcommand. The 'cfi fp stacksize' options will be interpreted within the 
'check' subcommand.

and note how writing:

  binary check help

or:

  binary check -h

will give the user context sensitive help. It won't print any help text about any 
'fix' functionality, generating CFI information for example.

> (And note this is a further example of why subcommands are not a good fit.  We 
> would want to be able to combine this option with the others without needing an 
> exponential growth in the number of subcommands.)

I still don't see where we'd (ever!) get such exponential growth of subcommands. 
If you do it right and structure it into an intuitive interface, it won't happen.

Thanks,

	Ingo