a patch I made almost 2 years ago which adds a bunch of new operators to DDB command language. I used back then to debug an issue I had with a network driver, but the intent of the code alays was to be the first pass towards supporting conditional breakpoint in DDB
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 3568 Build 3608: arc lint + arc unit
Event Timeline
Comment Actions
I did some cleanups that were incorporated but I didn't test it.
We should push it for FreeBSD 11, I think.
| sys/ddb/db_expr.c | ||
|---|---|---|
| 213 | This seems like a mismerge, and it also seems better served by using a switch with cases instead? | |
Comment Actions
@jhb true. Ill convert it to a case and fix the issue. I can also apply it on current and test it there. Once its fixed, what is the prefered course of action ?
Comment Actions
- Updating D4230: DDB operators patch #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment. #
- If you intended to create a new revision, use:
- $ arc diff --create
Applied the patch the current, previously it was applied to a very old version
Changed db_add_expr function to use case statements
Fixed what I considered to be a bug in how db_print_cmd handles print formats.
Previously an incorrect format specifier whould not be cought and the invalid
specifier saved in db_print_format global varibale, altering in a unwanted way
the behaviour of subsequent calls to db_printf
kernel compiled cleanly after the patch was applied
Comment Actions
I have a complementary enhancement from DragonFly: D6146
It would be really nice to document the new features (not sure where).
Comment Actions
Now that the ddb examine (x) command changes are in, I would like to bring this change. It would be a step forward for FBSD 11.
There are some stray spaces which I will fix soon.
Comment Actions
It would be good to document the expression syntax in ddb(4) if possible. For example, that would be the best place to mention that >> assumes unsigned.
| sys/ddb/db_expr.c | ||
|---|---|---|
| 63 | It would be good to avoid mixing whitespace changes in. We can always fix those as a followup if needed, but this obscures the diff. Alternatively, you could reindent this function to match the rest of the file first and then update this diff so it just includes the functional changes. | |
| 101 | On the one hand I appreciate the more verbose syntax error messages (and I probably come down on the side of keeping them). On the other hand, having a single string saves a small amount of space in the kernel .rodata section. | |
| 172 | Inconsistent indentation here. | |
| 226 | Ah, if you used this here, you could use this above in db_mult_expr() as well to pacify others who are worried about the space bloat of verbose error messages. :) For example: db_printf("Expression syntax error after '%c'\n", '!'); | |
| 237 | Perhaps use 'default' here to pacify compiler warnings about unused cases that aren't actually possible? | |
| 256 | Indentation. | |
| 295 | If we're going the space-savings route, then perhaps use strings instead of characters for the argument: db_error("Expression syntax error after '%s'\n", "&&");Note that this use would mean using the same quotes in all cases, but I think that the consistency of that would be worth it. | |
| sys/ddb/db_lex.h | ||
| 39 | This is actually permitted in style(9), so I would leave this be. | |
Comment Actions
I haven't figure out the strings yet.
| sys/ddb/db_expr.c | ||
|---|---|---|
| 237 | I think this corresponded to line 220, where I added __unreachable(); now. | |
| sys/ddb/db_expr.c | ||
|---|---|---|
| 237 | Yes. There's another one around 301 as well. | |
Comment Actions
Im still learning the FreeBSD style, Ill be more careful next time with indentation and spaces. Meanwhile, if I have some questions on
style , can I mail you Pedro ? About documenting, yes maybe next week I could work on man page, but it will have to wait untill then.
Comment Actions
Well, this is not style(9), the code originated in Mach so we are respecting the previous style in order to minimize the differences. We probably should reformat this but the other BSDs have some interesting changes as well.
No problem about mailing me, except that I may take a while to reply.
Comment Actions
Fix compilation.
Not sure if this is the correct approach but since db_error() doesn't
let me print messages with extra characters, I use db_printf()
followed by a db_error(NULL).
Not sure what db_print_format() does (?).
Comment Actions
Dan ..
Were these supposed to come from Mach? I can't find them in Mach3 or in Darwin. Just curious.
Comment Actions
@Pedro. Yes, Pedro , they where brought over from a Mach kernel. I believe it was 3.0 . Then applied some bug fixes on the recursive parser.
Comment Actions
Pedro, you asked for licensing reasons too ? Should I try to find exactly the version I used ?
Comment Actions
No licensing issue. I was wondering about the convenience of the grammar. Once we add functions we cannot really take them back. Having the historical backing helps.
OTOH, I won't commit it until someone else approves it.
Comment Actions
OK.. I've waited some prudential time but it's time to commit this.
Still pending are the documentation updates(?).