Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Dwarf on windows #8717

Merged
merged 4 commits into from Jun 21, 2021
Merged

Dwarf on windows #8717

merged 4 commits into from Jun 21, 2021

Conversation

mchudleigh
Copy link
Contributor

This PR does 3 things but can be split up if needed.

  • Fix a compile time crash/unreachable-code when using bash based terminals on windows
  • Emit Dwarf debug info if targetting gnu abi on windows (maybe this should be a flag instead)
  • Add support for reading Dwarf info from COFF executables for stack and error tracing

@mchudleigh
Copy link
Contributor Author

Also it has been compiled and tested on Windows and Linux

LemonBoy
LemonBoy reviewed May 9, 2021
View reviewed changes
Copy link
Contributor

@LemonBoy LemonBoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I've left a few trivial comments here and there (and please run zig fmt).

lib/std/Progress.zig
@@ -143,6 +147,7 @@ pub fn start(self: *Progress, name: []const u8, estimated_total_items: usize) !*
self.terminal = stderr;
self.supports_ansi_escape_codes = true;
} else if (std.builtin.os.tag == .windows and stderr.isTty()) {
self.is_windows_terminal = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you get here? isCygwinPty and supportsAnsiEscapeCodes should detect this case and use escape sequences instead of WinAPI functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is the case. I was just trying to make it more explicit. The first case of the if/else if chain there is:

if (stderr.supportsAnsiEscapeCodes() which is just isCygwinPty() on windows.

I didn't add a comment but supports_ansi_escapse_codes and is_windows_terminal are mutually exclusive.

lib/std/coff.zig Outdated
@@ -162,20 +163,51 @@ pub const Coff = struct {
try self.loadOptionalHeader();
}

fn readStringFromTable(self: *Coff, offset: u32, buff: []u8) !u32 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn readStringFromTable(self: *Coff, offset: u32, buff: []u8) !u32 {
fn readStringFromTable(self: *Coff, offset: usize, buf: []u8) ![]const u8 {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

lib/std/coff.zig
// No symbol table therefore no string table
return error.MissingStringTable;
}
const string_table_offset = self.coff_header.pointer_to_symbol_table + (self.coff_header.number_of_symbols * 18) + offset;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment explaining what 18 is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

lib/std/coff.zig Outdated
try self.in_file.seekTo(string_table_offset);

var i: u8 = 0;
while (i < buff.len) : (i+=1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop can be simplified down to:

const str = try in.readUntilDelimiterOrEof(buf, 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know. I'm still getting used to the stdlib

lib/std/coff.zig Outdated
}
buff[i] = byte;
}
try self.in_file.seekTo(old_pos);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd place this in a defer block below the other seekTo, this way you're also rewinding the stream in case of error.

lib/std/coff.zig Outdated
skip_size = 2 * @sizeOf(u8) + 8 * @sizeOf(u16) + 18 * @sizeOf(u32);
num_rva_pos = opt_header_pos + 92;

try self.in_file.seekTo(opt_header_pos+28);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please format all the files you're touching with zig fmt.

lib/std/coff.zig Outdated
@@ -277,6 +309,19 @@ pub const Coff = struct {
.characteristics = try in.readIntLittle(u32),
},
});
const sec = &self.sections.items[self.sections.items.len-1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not move the name expansion logic before the append call?

lib/std/coff.zig Outdated
const in = self.in_file.reader();
try self.in_file.seekTo(sec.header.pointer_to_raw_data);
const out_buff = try allocator.alloc(u8, sec.header.misc.virtual_size);
const readBytes = in.read(out_buff);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const readBytes = in.read(out_buff);
in.readNoEof(out_buff);

lib/std/debug.zig Outdated
@@ -1525,6 +1548,7 @@ pub const ModuleDebugInfo = switch (builtin.os.tag) {
.uefi, .windows => struct {
base_address: usize,
pdb: pdb.Pdb,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pdb/dwarf can be consolidated into a union (enum) { pdb: pdb.Pdb, dwarf: DW.Dwarfinfo }.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea

@mchudleigh
Copy link
Contributor Author

Thanks for the quick review and constructive comments.

I'll fix everything you recommended and I'll include one more commit if it's alright:
Get rid of the ugly @intCast in Debug.zig by normalizing DwarfInfo to take u64 for all addresses

@LemonBoy
Copy link
Contributor

LemonBoy commented May 9, 2021

Get rid of the ugly @intcast in Debug.zig by normalizing DwarfInfo to take u64 for all addresses

Sure thing, go ahead. Many functions take u64 instead of usize because they are target-agnostic and you may want to load DWARF infos from a 64bit file on a 32bit system.

@mchudleigh
Copy link
Contributor Author

Okay, all changes discussed are pushed. Let me know if there's anything else.

@mchudleigh
Copy link
Contributor Author

Looking back on the commits in this PR, this is kind of a mess. Let me know if you'd prefer I rework it into fewer, tighter commits.

@LemonBoy LemonBoy mentioned this pull request May 25, 2021
Closed
@Vexu Vexu added the standard library This issue involves writing Zig code for the standard library. label Jun 13, 2021
@mchudleigh
Copy link
Contributor Author

I rebased these changes onto a post 0.8 master so they will be much easier to merge.

@mchudleigh mchudleigh mentioned this pull request Jun 18, 2021
Closed
@Vexu Vexu merged commit 8a6de78 into ziglang:master Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LemonBoy LemonBoy LemonBoy left review comments

@Vexu Vexu Vexu approved these changes

Assignees
No one assigned
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mchudleigh @LemonBoy @Vexu