jdetter / Chronos

The Chronos kernel
GNU General Public License v3.0
9 stars 1 forks source link

DEBUG printing -- DON'T MERGE -- Looking for code review #5

Closed spwilson2 closed 8 years ago

spwilson2 commented 8 years ago

_DON'T MERGE YET_

So this is essentially what the debug statements are going to look like. See comments in panic.h and change to Makefile for how to enable debug output. Feel free to chew my ass out if you don't like, or offer anything you got :underage:.

jdetter commented 8 years ago

I think it's looking good so far. You might want to create a separate branch for this so that we have something to work out of if we both want to make changes. Something like debug or debug-system would be cool. I didn't see that you forked lol, you can keep this in your repository if you would like, if I make changes I'll probably make a new branch in my repo though.

It would be nice to type as little as possible when writing debugging statements. DEBUG(D_LEVEL_DEFAULT, D_SYSTEM_DEFAULT "message") might be a bit too much typing for a simple output statement. Even if the macro names could be shortened, it would be better.

I would prefer only having to pass one argument to the macro. Something like DEBUG(KERNEL_VER, "message") that includes both the thing you want debugged (the kernel) and the level (verbose) in one argument would be preferable in my opinion over DEBUG(D_LEVEL_DEFAULT, D_SYSTEM_DEFAULT "message")

Also one thing that is a pain in the ass is that every time I write debug statements, I always do stuff like cprintf("syscall: system call debugging.");. It would be cool if the macro could append "syscall: " to the beginning of the message so that I don't have to tag it every time.

One last thing, most commonly when I have an issue with system calls, I enable the print outs just for system calls. On it's own this can be kind of a flood so it would be nice if there would be a way where I could do DEBUG(KERNEL_SYSCALL, "message") and only get output for system calls and no other general debugging output.

So I think this is a good step in the right direction. The only thing I'm iffy about is the length of the macros that I would need to type to get a simple debug statement out. Besides that, everything is looking good.

spwilson2 commented 8 years ago

Yeah, I definitely didn't think about the developing use cases of this. So, as is, if you really want to make a quick call, you can just do DEBUG(0,0,<fmt-here>). EDIT: I really was shooting at making the long term debug statements easier to compartmentalize and not have them hurt code readability.

I definitely will add the tracing "syscall; " like stuff to output based on the D_SYSTEM defined. What I'd really like to be able to do (but we don't have the infrastructure for) is have a conditional include for each system we're building so that way you wouldn't even need to add the system argument for this to happen. My only concern right now is that we can have at most 32 different systems? (32 bits for defines) So if we try to add too many systems it could get crufty. Also trying to add defines for each system will get very crufty without a config system.

^ So, basically I'm saying let me know if there are any systems that important to have specific prints for right now. Otherwise I'll try my best to make relevant ones.

I'll also add just a quick debug macro like D_PRINT that would be part of D_SYSTEM_DEFAULT, but have it's own LEVEL category so you could enable it just for development. With compiling with -DD_LEVEL=DEVELOP So the usage of it would be something like D_PRINT("I'm developing!\n");

jdetter commented 8 years ago

Ok sweet, the only ones I can think of right now are system calls and the everything in fsman.c should probably be separate.

When we are implementing stuff I think it probably makes most sense to add more cases. Then when the implementation is finished, move those cases over to the a general case and then delete them. For example if you were implementing a driver but you didn't want all of the output from all of the other drivers, it would be nice if there was an output level where you would only see your driver's output. Then when you think you're finished with the driver, you could convert the debug statements over to general driver debug output.

Also one side note: we have no support for asserts right now. Since you're doing debugging stuff anyway, do you want to add support for asserts?

spwilson2 commented 8 years ago

Also one side note: we have no support for asserts right now. Since you're doing debugging stuff anyway, do you want to add support for asserts?

Will do daddio.

Also closing this, and will move changes to a branch on this repo.