-
Notifications
You must be signed in to change notification settings - Fork 277
Add some usage examples into the goto-instrument readme file. [DOC-41] #2793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add some usage examples into the goto-instrument readme file. [DOC-41] #2793
Conversation
src/goto-instrument/README.md
Outdated
it uses to write the result of the transformation. This is important | ||
because many times, if you don't supply a second filename for the | ||
goto-binary to be written to, you get the equivalent of the `--help` | ||
option output, with not indication of what has gone wrong. For more specific |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/with not/with no/
src/goto-instrument/README.md
Outdated
|
||
### Call Graph ### | ||
|
||
You can see the call graph of a particular goto-binary by issuing `goto-instrument --call-graph <goto_binary>`. This gives a result similar to this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite a surprising first example, because it's one of those that does not require three parameters.
src/goto-instrument/README.md
Outdated
// 27 file function_pointer.c line 24 function func | ||
END_FUNCTION | ||
|
||
You can now see that the blind indexed call has now been substituted by a dispatch table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a "blind indexed call?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With that I meant that it's an index call with no particularly obvious target - I had it in mind with comparison to the dispatch table after the function pointer removal. What's a better way to describe this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should stick with established terminology. There is the concept of function pointers (and possibly lookup-tables) in high-level languages, or indirect calls (or indirect branches) at machine-instruction level. To make clear that these concepts are being combined with indexed access of an array, you might just have to extend this text a bit. But let's just not invent new terminology.
7c4fce4
to
524eec9
Compare
524eec9
to
7d390a2
Compare
@peterschrammel @tautschnig The latest review comments have all been addressed. Can you take a look at your earliest convenience to tell me if there's something else I should change? |
src/goto-instrument/README.md
Outdated
|
||
You can now see that the function pointer (indirect) call (resulting from | ||
the array indexing of the array containing the function pointers) | ||
has now been substituted by a dispatch table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is much better. Just the final "dispatch table" I'm not sure about. Wouldn't that be what the array is/was? I think the result are direct, conditional calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right. I had gotten initially confused.
src/goto-instrument/README.md
Outdated
### Call Graph ### | ||
|
||
This is an example of a command line flag that doesn't require two file | ||
arguments, one for input and one for output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about: "This is an example of a command-line flag that requires only one argument, specifying the input file." (Talk about what is the case, not what is not.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is a good suggestion, I'll get down to it now.
a4bbf60
to
68fea9d
Compare
@tautschnig Hey, I implemented the last changes in your approval, but now I need it again to be able to merge it myself. Can I get a reapproval now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[...] but now I need it again to be able to merge it myself. Can I get a reapproval now?
Hmm, to me GitHub suggests that approval by @peterschrammel @chrisr-diffblue and/or @martin-cs is required. (As well as a rebase onto current HEAD as #2863 has been merged.) But I could be wrong, happy to re-approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of really minor comments, but happy enough as it is.
68fea9d
to
2819f60
Compare
Added some usage examples that demonstrate the usage of
goto-instrument
with one and two arguments (as a transformation pass usually requires).