From bda5180b2ccfed22da04dd0b7a48d60beda03538 Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Fri, 9 Dec 2022 12:40:53 +0100 Subject: [PATCH 1/3] llvm: resolve all relative paths when creating DIFiles This will make stack traces and debugging experience more consistent in the sense that the presence of source lines in stack traces will not be dependent on the current working directory of the running process. --- src/codegen/llvm.zig | 44 +++++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 79c253949ae2ef295d152c84fea189e0772ea8d6..4115a4870ea3f8731b929afd65f5111bee1f2983 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -443,20 +443,19 @@ pub const Object = struct { }); defer gpa.free(producer); - // For macOS stack traces, we want to avoid having to parse the compilation unit debug - // info. As long as each debug info file has a path independent of the compilation unit - // directory (DW_AT_comp_dir), then we never have to look at the compilation unit debug - // info. If we provide an absolute path to LLVM here for the compilation unit debug - // info, LLVM will emit DWARF info that depends on DW_AT_comp_dir. To avoid this, we - // pass "." for the compilation unit directory. This forces each debug file to have a - // directory rather than be relative to DW_AT_comp_dir. According to DWARF 5, debug - // files will no longer reference DW_AT_comp_dir, for the purpose of being able to - // support the common practice of stripping all but the line number sections from an - // executable. - const compile_unit_dir = d: { - if (options.target.isDarwin()) break :d "."; - const mod = options.module orelse break :d "."; - break :d mod.root_pkg.root_src_directory.path orelse "."; + // We fully resolve all paths at this point to avoid lack of source line info in stack + // traces or lack of debugging information which, if relative paths were used, would + // be very location dependent. + // TODO: the only concern I have with this is WASI as either host or target, should + // we leave the paths as relative then? + var buf: [std.fs.MAX_PATH_BYTES]u8 = undefined; + const compile_unit_dir = blk: { + const path = d: { + const mod = options.module orelse break :d "."; + break :d mod.root_pkg.root_src_directory.path orelse "."; + }; + if (std.fs.path.isAbsolute(path)) break :blk path; + break :blk std.os.realpath(path, &buf) catch path; // If realpath fails, fallback to whatever path was }; const compile_unit_dir_z = try gpa.dupeZ(u8, compile_unit_dir); defer gpa.free(compile_unit_dir_z); @@ -1389,13 +1388,20 @@ pub const Object = struct { if (gop.found_existing) { return @ptrCast(*llvm.DIFile, gop.value_ptr.*); } - const dir_path = file.pkg.root_src_directory.path orelse "."; + const dir_path_z = d: { + var buffer: [std.fs.MAX_PATH_BYTES]u8 = undefined; + const dir_path = file.pkg.root_src_directory.path orelse "."; + const resolved_dir_path = if (std.fs.path.isAbsolute(dir_path)) + dir_path + else + std.os.realpath(dir_path, &buffer) catch dir_path; // If realpath fails, fallback to whatever dir_path was + break :d try std.fs.path.joinZ(gpa, &.{ + resolved_dir_path, std.fs.path.dirname(file.sub_file_path) orelse "", + }); + }; + defer gpa.free(dir_path_z); const sub_file_path_z = try gpa.dupeZ(u8, std.fs.path.basename(file.sub_file_path)); defer gpa.free(sub_file_path_z); - const dir_path_z = try std.fs.path.joinZ(gpa, &.{ - dir_path, std.fs.path.dirname(file.sub_file_path) orelse "", - }); - defer gpa.free(dir_path_z); const di_file = o.di_builder.?.createFile(sub_file_path_z, dir_path_z); gop.value_ptr.* = di_file.toNode(); return di_file; -- 2.54.0 From 4d640f9bb98d334f7fab8d3037a36cfd95dc174a Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Fri, 9 Dec 2022 12:43:06 +0100 Subject: [PATCH 2/3] dwarf: resolve all relative paths when generating include_dirs and file_names lists --- src/link/Dwarf.zig | 54 ++++++++++++++++----------------- src/link/Elf.zig | 2 +- src/link/MachO/DebugSymbols.zig | 2 +- src/link/Wasm.zig | 2 +- 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/link/Dwarf.zig b/src/link/Dwarf.zig index d02f994eb7ec727e0d78b17c78cea75015431efc..6cc3a7d68f817a2e7c7044bf783be4ab1bba90aa 100644 --- a/src/link/Dwarf.zig +++ b/src/link/Dwarf.zig @@ -1771,7 +1771,8 @@ pub fn writeDbgInfoHeader(self: *Dwarf, module: *Module, low_pc: u64, high_pc: u } // Write the form for the compile unit, which must match the abbrev table above. const name_strp = try self.makeString(module.root_pkg.root_src_path); - const compile_unit_dir = self.getCompDir(module); + var compile_unit_dir_buffer: [std.fs.MAX_PATH_BYTES]u8 = undefined; + const compile_unit_dir = resolveCompilationDir(module, &compile_unit_dir_buffer); const comp_dir_strp = try self.makeString(compile_unit_dir); const producer_strp = try self.makeString(link.producer_string); @@ -1823,19 +1824,15 @@ pub fn writeDbgInfoHeader(self: *Dwarf, module: *Module, low_pc: u64, high_pc: u } } -fn getCompDir(self: Dwarf, module: *Module) []const u8 { - // For macOS stack traces, we want to avoid having to parse the compilation unit debug - // info. As long as each debug info file has a path independent of the compilation unit - // directory (DW_AT_comp_dir), then we never have to look at the compilation unit debug - // info. If we provide an absolute path to LLVM here for the compilation unit debug - // info, LLVM will emit DWARF info that depends on DW_AT_comp_dir. To avoid this, we - // pass "." for the compilation unit directory. This forces each debug file to have a - // directory rather than be relative to DW_AT_comp_dir. According to DWARF 5, debug - // files will no longer reference DW_AT_comp_dir, for the purpose of being able to - // support the common practice of stripping all but the line number sections from an - // executable. - if (self.bin_file.tag == .macho) return "."; - return module.root_pkg.root_src_directory.path orelse "."; +fn resolveCompilationDir(module: *Module, buffer: *[std.fs.MAX_PATH_BYTES]u8) []const u8 { + // We fully resolve all paths at this point to avoid lack of source line info in stack + // traces or lack of debugging information which, if relative paths were used, would + // be very location dependent. + // TODO: the only concern I have with this is WASI as either host or target, should + // we leave the paths as relative then? + const comp_dir_path = module.root_pkg.root_src_directory.path orelse "."; + if (std.fs.path.isAbsolute(comp_dir_path)) return comp_dir_path; + return std.os.realpath(comp_dir_path, buffer) catch comp_dir_path; // If realpath fails, fallback to whatever comp_dir_path was } fn writeAddrAssumeCapacity(self: *Dwarf, buf: *std.ArrayList(u8), addr: u64) void { @@ -2149,7 +2146,7 @@ pub fn writeDbgAranges(self: *Dwarf, addr: u64, size: u64) !void { } } -pub fn writeDbgLineHeader(self: *Dwarf, module: *Module) !void { +pub fn writeDbgLineHeader(self: *Dwarf) !void { const gpa = self.allocator; const ptr_width_bytes: u8 = self.ptrWidthBytes(); @@ -2167,7 +2164,7 @@ pub fn writeDbgLineHeader(self: *Dwarf, module: *Module) !void { // Convert all input DI files into a set of include dirs and file names. var arena = std.heap.ArenaAllocator.init(gpa); defer arena.deinit(); - const paths = try self.genIncludeDirsAndFileNames(arena.allocator(), module); + const paths = try self.genIncludeDirsAndFileNames(arena.allocator()); // The size of this header is variable, depending on the number of directories, // files, and padding. We have a function to compute the upper bound size, however, @@ -2551,7 +2548,7 @@ fn addDIFile(self: *Dwarf, mod: *Module, decl_index: Module.Decl.Index) !u28 { return @intCast(u28, gop.index + 1); } -fn genIncludeDirsAndFileNames(self: *Dwarf, arena: Allocator, module: *Module) !struct { +fn genIncludeDirsAndFileNames(self: *Dwarf, arena: Allocator) !struct { dirs: []const []const u8, files: []const []const u8, files_dirs_indexes: []u28, @@ -2565,19 +2562,22 @@ fn genIncludeDirsAndFileNames(self: *Dwarf, arena: Allocator, module: *Module) ! var files_dir_indexes = std.ArrayList(u28).init(arena); try files_dir_indexes.ensureTotalCapacity(self.di_files.count()); - const comp_dir = self.getCompDir(module); - for (self.di_files.keys()) |dif| { - const full_path = try dif.fullPath(arena); - const dir_path = std.fs.path.dirname(full_path) orelse "."; - const sub_file_path = std.fs.path.basename(full_path); + const dir_path = d: { + var buffer: [std.fs.MAX_PATH_BYTES]u8 = undefined; + const dir_path = dif.pkg.root_src_directory.path orelse "."; + const abs_dir_path = if (std.fs.path.isAbsolute(dir_path)) + dir_path + else + std.os.realpath(dir_path, &buffer) catch dir_path; // If realpath fails, fallback to whatever dir_path was + break :d try std.fs.path.join(arena, &.{ + abs_dir_path, std.fs.path.dirname(dif.sub_file_path) orelse "", + }); + }; + const sub_file_path = try arena.dupe(u8, std.fs.path.basename(dif.sub_file_path)); const dir_index: u28 = blk: { - const actual_dir_path = if (mem.indexOf(u8, dir_path, comp_dir)) |_| inner: { - if (comp_dir.len == dir_path.len) break :blk 0; - break :inner dir_path[comp_dir.len + 1 ..]; - } else dir_path; - const dirs_gop = dirs.getOrPutAssumeCapacity(actual_dir_path); + const dirs_gop = dirs.getOrPutAssumeCapacity(dir_path); break :blk @intCast(u28, dirs_gop.index + 1); }; diff --git a/src/link/Elf.zig b/src/link/Elf.zig index 601b1a3e939fbec5bdcb2023e26b97f7889d00cb..ebb1cbdfb8efc867d34e0315256e6efa4c90a035 100644 --- a/src/link/Elf.zig +++ b/src/link/Elf.zig @@ -1149,7 +1149,7 @@ pub fn flushModule(self: *Elf, comp: *Compilation, prog_node: *std.Progress.Node } if (self.debug_line_header_dirty) { - try dw.writeDbgLineHeader(module); + try dw.writeDbgLineHeader(); self.debug_line_header_dirty = false; } } diff --git a/src/link/MachO/DebugSymbols.zig b/src/link/MachO/DebugSymbols.zig index 7fa4a8f4ddc3d2f2922492c4d0d1aeacdf485bb3..655ba7162fcbdd1fa3d652da5a9724d1af786a3d 100644 --- a/src/link/MachO/DebugSymbols.zig +++ b/src/link/MachO/DebugSymbols.zig @@ -281,7 +281,7 @@ pub fn flushModule(self: *DebugSymbols, macho_file: *MachO) !void { } if (self.debug_line_header_dirty) { - try self.dwarf.writeDbgLineHeader(module); + try self.dwarf.writeDbgLineHeader(); self.debug_line_header_dirty = false; } diff --git a/src/link/Wasm.zig b/src/link/Wasm.zig index f8821d796ffc9fbe1ebf849577f59ceefc01be0c..74a303b8c7ae237ca596466be1b4e0c0bb9c97a8 100644 --- a/src/link/Wasm.zig +++ b/src/link/Wasm.zig @@ -2672,7 +2672,7 @@ pub fn flushModule(wasm: *Wasm, comp: *Compilation, prog_node: *std.Progress.Nod // as locations are always offsets relative to 'code' section. try dwarf.writeDbgInfoHeader(mod, 0, code_section_size); try dwarf.writeDbgAranges(0, code_section_size); - try dwarf.writeDbgLineHeader(mod); + try dwarf.writeDbgLineHeader(); } var debug_bytes = std.ArrayList(u8).init(wasm.base.allocator); -- 2.54.0 From 182751ba27edf64304d2ae78043576b78c45fc1c Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Fri, 9 Dec 2022 12:57:15 +0100 Subject: [PATCH 3/3] Revert "coff: specify default base path for relative source paths in pdb" --- src/Compilation.zig | 25 ------------------------- src/link.zig | 4 ---- src/link/Coff/lld.zig | 4 ---- 3 files changed, 33 deletions(-) diff --git a/src/Compilation.zig b/src/Compilation.zig index 7f6314bbb04c1cb5338736273625109bee76c3e7..5623fdcbc52ded66c99abaf90987dae22093ffc9 100644 --- a/src/Compilation.zig +++ b/src/Compilation.zig @@ -1035,9 +1035,6 @@ pub const InitOptions = struct { /// (Darwin) remove dylibs that are unreachable by the entry point or exported symbols dead_strip_dylibs: bool = false, libcxx_abi_version: libcxx.AbiVersion = libcxx.AbiVersion.default, - /// (Windows) PDB source path prefix to instruct the linker how to resolve relative - /// paths when consolidating CodeView streams into a single PDB file. - pdb_source_path: ?[]const u8 = null, }; fn addPackageTableToCacheHash( @@ -1740,27 +1737,6 @@ pub fn create(gpa: Allocator, options: InitOptions) !*Compilation { }; }; - const pdb_source_path: ?[]const u8 = options.pdb_source_path orelse blk: { - if (builtin.target.os.tag == .windows) { - // PDB requires all file paths to be fully resolved, and it is really the - // linker's responsibility to canonicalize any path extracted from the CodeView - // in the object file. However, LLD-link has some very questionable defaults, and - // in particular, it purposely bakes in path separator of the host system it was - // built on rather than the targets, or just throw an error. Thankfully, they have - // left a backdoor we can use via -PDBSOURCEPATH. - const mod = module orelse break :blk null; - var buffer: [std.fs.MAX_PATH_BYTES]u8 = undefined; - const resolved_path = if (mod.main_pkg.root_src_directory.path) |base_path| p: { - if (std.fs.path.isAbsolute(base_path)) break :blk base_path; - const resolved_path = std.os.realpath(base_path, &buffer) catch break :blk null; - const pos = std.mem.lastIndexOfLinear(u8, resolved_path, base_path) orelse resolved_path.len; - break :p resolved_path[0..pos]; - } else std.os.realpath(".", &buffer) catch break :blk null; - break :blk try arena.dupe(u8, resolved_path); - } - break :blk null; - }; - const implib_emit: ?link.Emit = blk: { const emit_implib = options.emit_implib orelse break :blk null; @@ -1906,7 +1882,6 @@ pub fn create(gpa: Allocator, options: InitOptions) !*Compilation { .headerpad_max_install_names = options.headerpad_max_install_names, .dead_strip_dylibs = options.dead_strip_dylibs, .force_undefined_symbols = .{}, - .pdb_source_path = pdb_source_path, }); errdefer bin_file.destroy(); comp.* = .{ diff --git a/src/link.zig b/src/link.zig index e57ffd425662b361c89dfd32267a348c0eff142e..450763ac18af6d2930be0703858d61f218d18002 100644 --- a/src/link.zig +++ b/src/link.zig @@ -217,10 +217,6 @@ pub const Options = struct { /// (Darwin) remove dylibs that are unreachable by the entry point or exported symbols dead_strip_dylibs: bool = false, - /// (Windows) PDB source path prefix to instruct the linker how to resolve relative - /// paths when consolidating CodeView streams into a single PDB file. - pdb_source_path: ?[]const u8 = null, - pub fn effectiveOutputMode(options: Options) std.builtin.OutputMode { return if (options.use_lld) .Obj else options.output_mode; } diff --git a/src/link/Coff/lld.zig b/src/link/Coff/lld.zig index c1edb55b8092e3751dab57592c4704ecc1f12367..46b0130542898e3a32fbddb643bf2e7320b9cf7b 100644 --- a/src/link/Coff/lld.zig +++ b/src/link/Coff/lld.zig @@ -171,10 +171,6 @@ pub fn linkWithLLD(self: *Coff, comp: *Compilation, prog_node: *std.Progress.Nod }); try argv.append(try allocPrint(arena, "-PDB:{s}", .{out_pdb})); try argv.append(try allocPrint(arena, "-PDBALTPATH:{s}", .{out_pdb})); - - if (self.base.options.pdb_source_path) |path| { - try argv.append(try std.fmt.allocPrint(arena, "-PDBSOURCEPATH:{s}", .{path})); - } } if (self.base.options.lto) { switch (self.base.options.optimize_mode) { -- 2.54.0