Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1224138 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v11 00/20] Compile-time stack validation Ingo Molnar <mingo@kernel.org> - 2015-09-14 15:30 +0200
    Re: [PATCH v11 00/20] Compile-time stack validation Josh Poimboeuf <jpoimboe@redhat.com> - 2015-09-14 16:20 +0200

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

* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1224196

On Mon, Sep 14, 2015 at 03:19:52PM +0200, Ingo Molnar wrote:
> > > 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).

I still don't see how that would work.  Here's how we would achieve that
example with my latest proposal:

  stacktool --check-frame-pointer --check-cfi --gen-cfi

Notice how clear it is *exactly* what the tool is doing.

On the other hand, "debuginfo fix cfi" hides two of the three tasks.

It's also confusing and inconsistent: use the "check" subcommand for
checking, but use the "fix" subcommand for both checking *and*
generation?  That's far from obvious for the user.

Further, with my proposal all three options can be added and removed in
various combinations.  So you can do things like:

stacktool --gen-cfi
stacktool --check-frame-pointer --gen-cfi
stacktool --check-cfi --gen-cfi

How would you do those with subcommands?

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

Actually I would use the same argument *against* debuginfo.  We've
already identified a realistic potential usage that's unrelated to debug
info: stack size checking.  "debuginfo" is too limiting for that case.

OTOH, I don't think we've yet conceived of any non-stack-related uses of
the tool (other than some suggestions which are outside the scope of its
core functionality of analyzing all code paths).  So I really think
something with "stack" in the name would be appropriate.

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

That's way too generic IMO.  A tool named "binary" could do practically
anything.  The name doesn't give the user any idea about what it
actually does.

Also the proposed subcommands "ls" and "compress" have nothing to do
with recursive code path analysis and really belong in separate tools.

I really don't see any benefit to making a big monolithic tool which
does all things related to ELF/DWARF binary analysis.  There are already
a lot of good tools out there which do a lot of those things.

I don't see any good reason to grow this tool beyond its core
functionality of recursive code analysis.  At some point we have to draw
a reasonable line in the sand about what it will eventually do.
Otherwise we should just call it "tool" ;-)

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web