authorgravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2021-12-30 21:28:56-07:00
committergravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2022-01-02 13:16:17-07:00
log73ba6bf30be02d65e19304b0ec45c9a2a495698e
tree02b4df9737a767b929855e27c35b2c62b6c894e9
parent208a6c7d6a58c1fc46ed832acaa8a882f1c6a1dd

stage2: fix memory leak of emit_bin.sub_path

Instead of juggling GPA-allocated sub_path (and ultimately dropping the ball, in this analogy), `Compilation.create` allocates an already-exactly-correct size `sub_path` that has the digest unpopulated. This is then overwritten in place as necessary and used as the `emit_bin.sub_path` value, and no allocations/frees are performed for this file path.

1 files changed, 28 insertions(+), 32 deletions(-)

src/Compilation.zig+28-32
...@@ -98,9 +98,11 @@ clang_argv: []const []const u8,...@@ -98,9 +98,11 @@ clang_argv: []const []const u8,
98cache_parent: *Cache,98cache_parent: *Cache,
99/// Path to own executable for invoking `zig clang`.99/// Path to own executable for invoking `zig clang`.
100self_exe_path: ?[]const u8,100self_exe_path: ?[]const u8,
101/// null means -fno-emit-bin. Contains the basename of the101/// null means -fno-emit-bin.
102/// outputted binary file in case we don't know the directory yet.102/// This is mutable memory allocated into the Compilation-lifetime arena (`arena_state`)
103whole_bin_basename: ?[]const u8,103/// of exactly the correct size for "o/[digest]/[basename]".
104/// The basename is of the outputted binary file in case we don't know the directory yet.
105whole_bin_sub_path: ?[]u8,
104zig_lib_directory: Directory,106zig_lib_directory: Directory,
105local_cache_directory: Directory,107local_cache_directory: Directory,
106global_cache_directory: Directory,108global_cache_directory: Directory,
...@@ -1487,9 +1489,12 @@ pub fn create(gpa: Allocator, options: InitOptions) !*Compilation {...@@ -1487,9 +1489,12 @@ pub fn create(gpa: Allocator, options: InitOptions) !*Compilation {
1487 // can use it for communicating the result directory via `bin_file.emit`.1489 // can use it for communicating the result directory via `bin_file.emit`.
1488 // This is used to distinguish between -fno-emit-bin and -femit-bin1490 // This is used to distinguish between -fno-emit-bin and -femit-bin
1489 // for `CacheMode.whole`.1491 // for `CacheMode.whole`.
1490 const whole_bin_basename: ?[]const u8 = if (options.emit_bin) |x|1492 // This memory will be overwritten with the real digest in update() but
1493 // the basename will be preserved.
1494 const whole_bin_sub_path: ?[]u8 = if (options.emit_bin) |x|
1491 if (x.directory == null)1495 if (x.directory == null)
1492 x.basename1496 try std.fmt.allocPrint(arena, "o" ++ std.fs.path.sep_str ++
1497 ("x" ** Cache.hex_digest_len) ++ std.fs.path.sep_str ++ "{s}", .{x.basename})
1493 else1498 else
1494 null1499 null
1495 else1500 else
...@@ -1600,7 +1605,7 @@ pub fn create(gpa: Allocator, options: InitOptions) !*Compilation {...@@ -1600,7 +1605,7 @@ pub fn create(gpa: Allocator, options: InitOptions) !*Compilation {
1600 .local_cache_directory = options.local_cache_directory,1605 .local_cache_directory = options.local_cache_directory,
1601 .global_cache_directory = options.global_cache_directory,1606 .global_cache_directory = options.global_cache_directory,
1602 .bin_file = bin_file,1607 .bin_file = bin_file,
1603 .whole_bin_basename = whole_bin_basename,1608 .whole_bin_sub_path = whole_bin_sub_path,
1604 .emit_asm = options.emit_asm,1609 .emit_asm = options.emit_asm,
1605 .emit_llvm_ir = options.emit_llvm_ir,1610 .emit_llvm_ir = options.emit_llvm_ir,
1606 .emit_llvm_bc = options.emit_llvm_bc,1611 .emit_llvm_bc = options.emit_llvm_bc,
...@@ -1665,7 +1670,7 @@ pub fn create(gpa: Allocator, options: InitOptions) !*Compilation {...@@ -1665,7 +1670,7 @@ pub fn create(gpa: Allocator, options: InitOptions) !*Compilation {
1665 comp.c_object_table.putAssumeCapacityNoClobber(c_object, {});1670 comp.c_object_table.putAssumeCapacityNoClobber(c_object, {});
1666 }1671 }
16671672
1668 const have_bin_emit = comp.bin_file.options.emit != null or comp.whole_bin_basename != null;1673 const have_bin_emit = comp.bin_file.options.emit != null or comp.whole_bin_sub_path != null;
16691674
1670 if (have_bin_emit and !comp.bin_file.options.skip_linker_dependencies) {1675 if (have_bin_emit and !comp.bin_file.options.skip_linker_dependencies) {
1671 // If we need to build glibc for the target, add work items for it.1676 // If we need to build glibc for the target, add work items for it.
...@@ -1941,19 +1946,7 @@ pub fn update(comp: *Compilation) !void {...@@ -1941,19 +1946,7 @@ pub fn update(comp: *Compilation) !void {
1941 log.debug("CacheMode.whole cache hit for {s}", .{comp.bin_file.options.root_name});1946 log.debug("CacheMode.whole cache hit for {s}", .{comp.bin_file.options.root_name});
1942 const digest = man.final();1947 const digest = man.final();
19431948
1944 // Communicate the output binary location to parent Compilations.1949 comp.wholeCacheModeSetBinFilePath(&digest);
1945 if (comp.whole_bin_basename) |basename| {
1946 const new_sub_path = try std.fs.path.join(comp.gpa, &.{
1947 "o", &digest, basename,
1948 });
1949 if (comp.bin_file.options.emit) |emit| {
1950 comp.gpa.free(emit.sub_path);
1951 }
1952 comp.bin_file.options.emit = .{
1953 .directory = comp.local_cache_directory,
1954 .sub_path = new_sub_path,
1955 };
1956 }
19571950
1958 assert(comp.bin_file.lock == null);1951 assert(comp.bin_file.lock == null);
1959 comp.bin_file.lock = man.toOwnedLock();1952 comp.bin_file.lock = man.toOwnedLock();
...@@ -1989,13 +1982,10 @@ pub fn update(comp: *Compilation) !void {...@@ -1989,13 +1982,10 @@ pub fn update(comp: *Compilation) !void {
1989 // This resets the link.File to operate as if we called openPath() in create()1982 // This resets the link.File to operate as if we called openPath() in create()
1990 // instead of simulating -fno-emit-bin.1983 // instead of simulating -fno-emit-bin.
1991 var options = comp.bin_file.options;1984 var options = comp.bin_file.options;
1992 if (comp.whole_bin_basename) |basename| {1985 if (comp.whole_bin_sub_path) |sub_path| {
1993 if (options.emit) |emit| {
1994 comp.gpa.free(emit.sub_path);
1995 }
1996 options.emit = .{1986 options.emit = .{
1997 .directory = tmp_artifact_directory.?,1987 .directory = tmp_artifact_directory.?,
1998 .sub_path = basename,1988 .sub_path = std.fs.path.basename(sub_path),
1999 };1989 };
2000 }1990 }
2001 comp.bin_file.destroy();1991 comp.bin_file.destroy();
...@@ -2149,13 +2139,7 @@ pub fn update(comp: *Compilation) !void {...@@ -2149,13 +2139,7 @@ pub fn update(comp: *Compilation) !void {
2149 log.warn("failed to write cache manifest: {s}", .{@errorName(err)});2139 log.warn("failed to write cache manifest: {s}", .{@errorName(err)});
2150 };2140 };
21512141
2152 // Communicate the output binary location to parent Compilations.2142 comp.wholeCacheModeSetBinFilePath(&digest);
2153 if (comp.whole_bin_basename) |basename| {
2154 comp.bin_file.options.emit = .{
2155 .directory = comp.local_cache_directory,
2156 .sub_path = try std.fs.path.join(comp.gpa, &.{ "o", &digest, basename }),
2157 };
2158 }
21592143
2160 assert(comp.bin_file.lock == null);2144 assert(comp.bin_file.lock == null);
2161 comp.bin_file.lock = man.toOwnedLock();2145 comp.bin_file.lock = man.toOwnedLock();
...@@ -2163,6 +2147,18 @@ pub fn update(comp: *Compilation) !void {...@@ -2163,6 +2147,18 @@ pub fn update(comp: *Compilation) !void {
2163 }2147 }
2164}2148}
21652149
2150/// Communicate the output binary location to parent Compilations.
2151fn wholeCacheModeSetBinFilePath(comp: *Compilation, digest: *const [Cache.hex_digest_len]u8) void {
2152 const sub_path = comp.whole_bin_sub_path orelse return;
2153 const digest_start = 2; // "o/[digest]/[basename]"
2154 mem.copy(u8, sub_path[digest_start..], digest);
2155
2156 comp.bin_file.options.emit = .{
2157 .directory = comp.local_cache_directory,
2158 .sub_path = sub_path,
2159 };
2160}
2161
2166/// This is only observed at compile-time and used to emit a compile error2162/// This is only observed at compile-time and used to emit a compile error
2167/// to remind the programmer to update multiple related pieces of code that2163/// to remind the programmer to update multiple related pieces of code that
2168/// are in different locations. Bump this number when adding or deleting2164/// are in different locations. Bump this number when adding or deleting