add support for stack traces on macosx #780
Conversation
Add basic address->symbol resolution support. Uses symtab data from the MachO image, not external dSYM data; that's left as a future exercise. The net effect is that we can now map addresses to function names but not much more. File names and line number data will have to wait until a future pull request. Partially fixes #434.
| } | ||
|
|
||
| fn readNoEof(in: &io.FileInStream, sink: var) !void { | ||
| if (@typeOf(sink) == []Nlist64) { |
There was a problem hiding this comment.
I couldn't figure out a way to do this generically.
There was a problem hiding this comment.
I have a lot of responses to this, starting with minimal changes, ending with let's change the standard library.
-
I'm actually fine with this, given that it's at least correct for this file, and it's a private function so it's not really exposed to much of anything.
-
Here's how to do what you wanted to do:
fn readNoEof(in: &io.FileInStream, sink: var) !void {
switch (@typeId(@typeOf(sink)) {
builtin.TypeId.Slice => {
const T = @typeOf(sink).Child;
...
},
builtin.TypeId.Pointer => {
const T = @typeOf(sink).Child;
...
},
else => @compileError("expected slice or pointer"),
}
}- However, let's use this as an example to write idiomatic zig code. I think it would be better to separate this out into 2 functions:
fn readNoEof(in: &io.FileInStream, comptime T: type, result: []T) !void {
return in.stream.readNoEof(([]u8)(result));
}
fn readOneNoEof(in: &io.FileInStream, comptime T: type, result: &T) !void {
return readNoEof(in, T, result[0..1]);
}related: #287
-
One more thing, this looks like it's a pretty common thing to want to do. Let's modify
std.io.InStreamto support these APIs. I think that it's ok to break backwards compatibility of readNoEof and give it a comptime parameter of the type you want to read. Existing callsites can be updated to get the old behavior by passing inu8.And then we can add a safety check, if you pass a struct type to
readNoEof, it will give you a compile error if your struct is notpacked(because you would get an undefined value when deserializing a non-packed struct).I think this macho code should be passing around a
&std.io.InStreaminstead of the&std.io.FileInStreamdirectly. This is where thatreadNoEofAPI would go. I understand that it's a little weird because you'd have to usevarfor the parameter due to error sets. That's In/OutStream design flaw introduced with new error set changes. #764 and we don't have a good solution for that yet (other than "just use var"). -
Finally, I think we should go back to (3) for now, because in order for (4) to make sense, we need the ability to specify endianness in packed structs. That's pending Add endianness as one of the pointer properties #649. Once packed structs support the concept of endianness, it makes sense to use them as types to serialization/deserialization functions.
Hope that helps!
My proposal is:
- do (3) since it's easy
- merge this PR since it's better than status quo
- I'll bump the priority of Add endianness as one of the pointer properties #649
- Once we get packed struct endianness working, we can add support for serializing/deserializing them using
std.io.OutStream/std.io.InStreamand then update this code to usestd.io.InStreaminstead ofstd.io.FileInStream
|
Exciting! I'm happy to merge this given that the code looks good and it passes all the tests, but I won't have access to my mac computer to try it out until Wednesday. I'll give some style feedback as requested. |
| } | ||
| } | ||
|
|
||
| pub fn openSelfDebugInfo(allocator: &mem.Allocator) !&ElfStackTrace { | ||
| switch (builtin.object_format) { | ||
| builtin.ObjectFormat.elf => { | ||
| const st = try allocator.create(ElfStackTrace); | ||
| errdefer allocator.destroy(st); |
| } | ||
|
|
||
| fn readNoEof(in: &io.FileInStream, sink: var) !void { | ||
| if (@typeOf(sink) == []Nlist64) { |
There was a problem hiding this comment.
I have a lot of responses to this, starting with minimal changes, ending with let's change the standard library.
-
I'm actually fine with this, given that it's at least correct for this file, and it's a private function so it's not really exposed to much of anything.
-
Here's how to do what you wanted to do:
fn readNoEof(in: &io.FileInStream, sink: var) !void {
switch (@typeId(@typeOf(sink)) {
builtin.TypeId.Slice => {
const T = @typeOf(sink).Child;
...
},
builtin.TypeId.Pointer => {
const T = @typeOf(sink).Child;
...
},
else => @compileError("expected slice or pointer"),
}
}- However, let's use this as an example to write idiomatic zig code. I think it would be better to separate this out into 2 functions:
fn readNoEof(in: &io.FileInStream, comptime T: type, result: []T) !void {
return in.stream.readNoEof(([]u8)(result));
}
fn readOneNoEof(in: &io.FileInStream, comptime T: type, result: &T) !void {
return readNoEof(in, T, result[0..1]);
}related: #287
-
One more thing, this looks like it's a pretty common thing to want to do. Let's modify
std.io.InStreamto support these APIs. I think that it's ok to break backwards compatibility of readNoEof and give it a comptime parameter of the type you want to read. Existing callsites can be updated to get the old behavior by passing inu8.And then we can add a safety check, if you pass a struct type to
readNoEof, it will give you a compile error if your struct is notpacked(because you would get an undefined value when deserializing a non-packed struct).I think this macho code should be passing around a
&std.io.InStreaminstead of the&std.io.FileInStreamdirectly. This is where thatreadNoEofAPI would go. I understand that it's a little weird because you'd have to usevarfor the parameter due to error sets. That's In/OutStream design flaw introduced with new error set changes. #764 and we don't have a good solution for that yet (other than "just use var"). -
Finally, I think we should go back to (3) for now, because in order for (4) to make sense, we need the ability to specify endianness in packed structs. That's pending Add endianness as one of the pointer properties #649. Once packed structs support the concept of endianness, it makes sense to use them as types to serialization/deserialization functions.
Hope that helps!
My proposal is:
- do (3) since it's easy
- merge this PR since it's better than status quo
- I'll bump the priority of Add endianness as one of the pointer properties #649
- Once we get packed struct endianness working, we can add support for serializing/deserializing them using
std.io.OutStream/std.io.InStreamand then update this code to usestd.io.InStreaminstead ofstd.io.FileInStream
|
cc @jfo - you were looking into this too |
|
I played with this a bit on my mac. Cool - much better than no stack trace: |
|
Awesome! I'd like to try and dig in more to help with improving this if I can find the time! @andrewrk, is this eventually meant to be an error return trace? or does that trigger under slightly different circumstances? |
|
Both error return traces and stack traces are arrays of return addresses. We obtain an error return trace as an actual array of return addresses, and we find the stack trace in a "streaming" fashion by walking up the stack, looking at the return addresses along the way. In both cases we call So improving The above output was generated with: test "aoeu" {
unreachable;
}To output an error return trace you can do: test "aoeu" {
return error.Foo;
}Here's what the former looks like on linux: Here's what the latter looks like on linux: Does that answer your question? |
|
Yes, it does. As a related issue, I noticed this isn't context aware wrt terminal control codes, if I'm printing somewhere that doesn't respond to them or piping the output to a file.
This is a fantastic step though, thanks @bnoordhuis ! |
|
Oops, good catch. We even compute whether we should have tty colors and then ignore the parameter: On a related note - the way that terminal colors works on windows is different. I think we should have a tty colors abstraction in std.io that works for both windows and posix. Here's the abstraction in zig compiler. It's a little clunky: |
FWIW, libuv has an ANSI emulator, which looks like it's the same principle albeit rather more elaborate: Might be a good project for someone looking to kill a few hours to port that over to zig. |
|
That's an interesting idea. So there are 2 approaches here:
One thing that I want to attempt to solve in the standard library, at some point, is the problem of not escaping terminal codes by default. It seems to me that if you do I actually think that the windows API is better here, because you have to explicitly call functions to do terminal actions, so you don't accidentally let user input control your terminal. |
Add basic address->symbol resolution support. Uses symtab data from the
MachO image, not external dSYM data; that's left as a future exercise.
The net effect is that we can now map addresses to function names but
not much more. File names and line number data will have to wait until
a future pull request.
Partially fixes #434.
Style feedback very welcome, by the way. I tried to follow the house style but I probably missed things.