-
Notifications
You must be signed in to change notification settings - Fork 59
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
X86-64 kernel backtrace #4
base: master
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
06d371d
to
7e2a887
Compare
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
@@ -367,7 +371,7 @@ config_arrakismon = False | |||
defines :: [RuleToken] | |||
defines = [ Str ("-D" ++ d) | d <- [ | |||
if microbenchmarks then "CONFIG_MICROBENCHMARKS" else "", | |||
if trace then "CONFIG_TRACE" else "", | |||
if stack_trace then "CONFIG_KERNEL_STACK_TRACE" else "", |
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.
Shouldn't delete trace/CONFIG_TRACE
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.
@antiguru, I have welded in a patch that rectifies this travesty, sorry!
void debug_vaddr_identify(lvaddr_t pml4, lvaddr_t vaddr); | ||
|
||
void debug_sort_dynsyms (struct Elf64_Sym *dynsyms, int n); | ||
void debug_setup_stackwalker (uint64_t stack_top, uint64_t stack_bottom, uint64_t text_start, uint64_t text_end, struct Elf64_Sym *dynsyms, char *dynstr, int nsyms); |
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.
It's better to use more specific types instead of uint64_t
, for example lpaddr_t
for local physical addresses or lvaddr_t
for virtual addresses. We might not do it correctly all the time but at least it gives a hint on how that address could be accessed.
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.
Addressed this in additional patches..
@@ -62,6 +62,20 @@ | |||
|
|||
#ifndef __ASSEMBLER__ | |||
|
|||
static inline uint64_t __attribute__((always_inline)) rdrip(void) |
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 guess this should have a return type of lvaddr_t
.
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.
Addressed this in additional patches..
@@ -0,0 +1,266 @@ | |||
/* |
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.
An alternative of copying newlib's qsort.c here would be to use the one in /lib/newlib/newlib/libc/search/qsort.c directly. I'm undecided what makes more sense.
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.
Please, do tell if you come to a decision!
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 initially thought there was no precedent to this kind of external inclusion of out-of-kernel-directory sources, but kernel/Hakefile
references ../usr/drivers/cpuboot/init_ap_x86_64.S
.
Not sure if this constitutes a significant point..
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.
Thanks for sending us the pull request. It seems like a valuable addition to the kernel. I added some comments in-line where I see potential room for improvement.
Could the code be unified for all architectures, especially ARMv8?
@barrelfish-bot ok to test |
eba42c4
to
39cf69c
Compare
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
..doesn't work due to: NixOS/nixpkgs#21374 Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
…bols missing from .dynsyms Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
Signed-off-by: Kosyrev Serge <[email protected]>
…r code review Signed-off-by: Kosyrev Serge <[email protected]>
…er review Signed-off-by: Kosyrev Serge <[email protected]>
4cad1eb
to
5040d46
Compare
An extra note ought to be made -- perhaps even documented. The backtrace mechanism currently depends on the A more advanced solution would be to promote a more complete symbol section, in a format amenable to runtime parsing, to be marked as Since, apparently (I'd be happy to be wrong!), So, please, tell me what all of this makes you think.. |
On unification. The code can be generalised, indeed. It would take one more pass, though.. |
Do I get the interpretation of the test suite right -- namely that the boot on physical hardware has failed?
|
@deepfire Looks like the machine didn't come up, this does not say anything about the code under test. |
This optionally provides a backtrace mechanism on x86-64:
Config.stack_trace