Improve duplicate resource error when outputting COFF obj

squeek502 · Nov 6, 2024 · 98b28b3 · 98b28b3
1 parent d103019
commit 98b28b3
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 20 deletions.
diff --git a/src/cvtres.zig b/src/cvtres.zig
@@ -177,12 +177,26 @@ pub const CoffOptions = struct {
     fold_duplicate_data: bool = false,
 };
 
-pub fn writeCoff(allocator: Allocator, writer: anytype, resources: []const Resource, options: CoffOptions) !void {
+pub const Diagnostics = union {
+    none: void,
+    /// Contains the index of the second resource in a duplicate resource pair.
+    duplicate_resource: usize,
+};
+
+pub fn writeCoff(allocator: Allocator, writer: anytype, resources: []const Resource, options: CoffOptions, diagnostics: ?*Diagnostics) !void {
     var resource_tree = ResourceTree.init(allocator);
     defer resource_tree.deinit();
 
     for (resources, 0..) |*resource, i| {
-        try resource_tree.put(resource, i);
+        resource_tree.put(resource, i) catch |err| {
+            switch (err) {
+                error.DuplicateResource => {
+                    if (diagnostics) |d_ptr| d_ptr.* = .{ .duplicate_resource = i };
+                },
+                else => {},
+            }
+            return err;
+        };
     }
 
     const lengths = resource_tree.dataLengths();
@@ -1059,7 +1073,7 @@ test writeCoff {
     {
         buf.clearRetainingCapacity();
 
-        try writeCoff(std.testing.allocator, buf.writer(), &.{}, .{});
+        try writeCoff(std.testing.allocator, buf.writer(), &.{}, .{}, null);
 
         try std.testing.expectEqualSlices(
             u8,
@@ -1082,7 +1096,7 @@ test writeCoff {
                 .characteristics = 0,
                 .data = "foo",
             },
-        }, .{});
+        }, .{}, null);
 
         try std.testing.expectEqualSlices(
             u8,
@@ -1124,7 +1138,7 @@ fn testResToCoff(res_data: []const u8, expected_coff: []const u8) !void {
     var buf = try std.ArrayList(u8).initCapacity(std.testing.allocator, expected_coff.len);
     defer buf.deinit();
 
-    try writeCoff(std.testing.allocator, buf.writer(), resources.list.items, .{});
+    try writeCoff(std.testing.allocator, buf.writer(), resources.list.items, .{}, null);
 
     try std.testing.expectEqualSlices(u8, expected_coff, buf.items);
 }
diff --git a/src/errors.zig b/src/errors.zig
@@ -626,17 +626,8 @@ pub const ErrorDetails = struct {
             },
             .string_already_defined => switch (self.type) {
                 .err, .warning => {
-                    const language_id = self.extra.string_and_language.language.asInt();
-                    const language_name = language_name: {
-                        if (std.meta.intToEnum(lang.LanguageId, language_id)) |lang_enum_val| {
-                            break :language_name @tagName(lang_enum_val);
-                        } else |_| {}
-                        if (language_id == lang.LOCALE_CUSTOM_UNSPECIFIED) {
-                            break :language_name "LOCALE_CUSTOM_UNSPECIFIED";
-                        }
-                        break :language_name "<UNKNOWN>";
-                    };
-                    return writer.print("string with id {d} (0x{X}) already defined for language {s} (0x{X})", .{ self.extra.string_and_language.id, self.extra.string_and_language.id, language_name, language_id });
+                    const language = self.extra.string_and_language.language;
+                    return writer.print("string with id {d} (0x{X}) already defined for language {}", .{ self.extra.string_and_language.id, self.extra.string_and_language.id, language });
                 },
                 .note => return writer.print("previous definition of string with id {d} (0x{X}) here", .{ self.extra.string_and_language.id, self.extra.string_and_language.id }),
                 .hint => return,

diff --git a/src/main.zig b/src/main.zig
@@ -15,6 +15,7 @@ const hasDisjointCodePage = @import("disjoint_code_page.zig").hasDisjointCodePag
 const cvtres = @import("cvtres.zig");
 const aro = @import("aro");
 const subcommands = @import("subcommands.zig");
+const fmtResourceType = @import("res.zig").NameOrOrdinal.fmtResourceType;
 
 pub fn main() !void {
     var gpa = std.heap.GeneralPurposeAllocator(.{ .stack_trace_frames = 8 }){};
@@ -402,9 +403,22 @@ pub fn main() !void {
 
     var coff_output_buffered_stream = std.io.bufferedWriter(coff_output_file.writer());
 
-    cvtres.writeCoff(allocator, coff_output_buffered_stream.writer(), resources.list.items, options.coff_options) catch |err| {
-        // TODO: Better errors
-        try renderErrorMessage(stderr.writer(), stderr_config, .err, "unable to write coff output file '{s}': {s}", .{ coff_output_filename, @errorName(err) });
+    var cvtres_diagnostics: cvtres.Diagnostics = .{ .none = {} };
+    cvtres.writeCoff(allocator, coff_output_buffered_stream.writer(), resources.list.items, options.coff_options, &cvtres_diagnostics) catch |err| {
+        switch (err) {
+            error.DuplicateResource => {
+                const duplicate_resource = resources.list.items[cvtres_diagnostics.duplicate_resource];
+                try renderErrorMessage(stderr.writer(), stderr_config, .err, "duplicate resource [id: {}, type: {}, language: {}]", .{
+                    duplicate_resource.name_value,
+                    fmtResourceType(duplicate_resource.type_value),
+                    duplicate_resource.language,
+                });
+                // TODO: Add note about where the first and second duplicate resources are if additional_inputs.len > 0
+            },
+            else => {
+                try renderErrorMessage(stderr.writer(), stderr_config, .err, "unable to write coff output file '{s}': {s}", .{ coff_output_filename, @errorName(err) });
+            },
+        }
         // Delete the output file on error
         coff_output_file.close();
         coff_output_file_closed = true;

diff --git a/src/res.zig b/src/res.zig
@@ -162,6 +162,27 @@ pub const Language = packed struct(u16) {
     pub fn asInt(self: Language) u16 {
         return @bitCast(self);
     }
+
+    pub fn format(
+        language: Language,
+        comptime fmt: []const u8,
+        options: std.fmt.FormatOptions,
+        out_stream: anytype,
+    ) !void {
+        _ = fmt;
+        _ = options;
+        const language_id = language.asInt();
+        const language_name = language_name: {
+            if (std.meta.intToEnum(lang.LanguageId, language_id)) |lang_enum_val| {
+                break :language_name @tagName(lang_enum_val);
+            } else |_| {}
+            if (language_id == lang.LOCALE_CUSTOM_UNSPECIFIED) {
+                break :language_name "LOCALE_CUSTOM_UNSPECIFIED";
+            }
+            break :language_name "<UNKNOWN>";
+        };
+        try out_stream.print("{s} (0x{X})", .{ language_name, language_id });
+    }
 };
 
 /// https://learn.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-dlgitemtemplate#remarks
@@ -423,6 +444,50 @@ pub const NameOrOrdinal = union(enum) {
             .name => return null,
         }
     }
+
+    pub fn format(
+        self: NameOrOrdinal,
+        comptime fmt: []const u8,
+        options: std.fmt.FormatOptions,
+        out_stream: anytype,
+    ) !void {
+        _ = fmt;
+        _ = options;
+        switch (self) {
+            .name => |name| {
+                try out_stream.print("{s}", .{std.unicode.fmtUtf16Le(name)});
+            },
+            .ordinal => |ordinal| {
+                try out_stream.print("{d}", .{ordinal});
+            },
+        }
+    }
+
+    fn formatResourceType(
+        self: NameOrOrdinal,
+        comptime fmt: []const u8,
+        options: std.fmt.FormatOptions,
+        out_stream: anytype,
+    ) !void {
+        _ = fmt;
+        _ = options;
+        switch (self) {
+            .name => |name| {
+                try out_stream.print("{s}", .{std.unicode.fmtUtf16Le(name)});
+            },
+            .ordinal => |ordinal| {
+                if (std.enums.tagName(RT, @enumFromInt(ordinal))) |predefined_type_name| {
+                    try out_stream.print("{s}", .{predefined_type_name});
+                } else {
+                    try out_stream.print("{d}", .{ordinal});
+                }
+            },
+        }
+    }
+
+    pub fn fmtResourceType(type_value: NameOrOrdinal) std.fmt.Formatter(formatResourceType) {
+        return .{ .data = type_value };
+    }
 };
 
 fn expectNameOrOrdinal(expected: NameOrOrdinal, actual: NameOrOrdinal) !void {

diff --git a/test/utils.zig b/test/utils.zig
@@ -300,7 +300,7 @@ pub fn getResinatorCvtResResult(allocator: Allocator, res_source: []const u8, op
         .target = options.target,
         .read_only = options.read_only,
         .define_external_symbol = options.define_external_symbol,
-    }) catch |err| {
+    }, null) catch |err| {
         buf.deinit();
         return .{
             .coff_err = err,