llvm: fix UB in metadata printer

[Rexicon226]

No description provided.

[alexrp]

For the benefit of people who aren't super familiar with the metadata formatter, can you explain what the UB is?

[Rexicon226]

Yeah sorry, seems I left out the description of the PR accidentally.

Here, the boolean is set to undefined,

zig/src/codegen/llvm/Builder.zig

Lines 9643 to 9644 in 497592c

	metadata_formatter.need_comma = false;
	defer metadata_formatter.need_comma = undefined;

which leads to unexpected results and invalid IR being printed.

Now that I think about it, why isn't this an optional?

[andrewrk]

Now that I think about it, why isn't this an optional?

Because the logic never checks whether it is null.

[Rexicon226]

Because the logic never checks whether it is null.

I don't think I understand your message here. How would it check whether it's null without it being an optional?

I'm thinking about something like this:

Diff

diff --git a/src/codegen/llvm/Builder.zig b/src/codegen/llvm/Builder.zig
index f5b05dfecd..38bb0db15d 100644
--- a/src/codegen/llvm/Builder.zig
+++ b/src/codegen/llvm/Builder.zig
@@ -8235,7 +8235,7 @@ pub const Metadata = enum(u32) {
 
     const Formatter = struct {
         builder: *Builder,
-        need_comma: bool,
+        need_comma: ?bool,
         map: std.AutoArrayHashMapUnmanaged(union(enum) {
             metadata: Metadata,
             debug_location: DebugLocation.Location,
@@ -8281,7 +8281,7 @@ pub const Metadata = enum(u32) {
             const is_specialized = fmt_str.len > 0 and fmt_str[0] == 'S';
             const recurse_fmt_str = if (is_specialized) fmt_str[1..] else fmt_str;
 
-            if (data.formatter.need_comma) try writer.writeAll(", ");
+            if (data.formatter.need_comma.?) try writer.writeAll(", ");
             defer data.formatter.need_comma = true;
             try writer.writeAll(data.prefix);
 
@@ -9495,7 +9495,7 @@ pub fn printUnbuffered(
     const writer = writer_with_errors.writer();
 
     var need_newline = false;
-    var metadata_formatter: Metadata.Formatter = .{ .builder = self, .need_comma = undefined };
+    var metadata_formatter: Metadata.Formatter = .{ .builder = self, .need_comma = null };
     defer metadata_formatter.map.deinit(self.gpa);
 
     if (self.source_filename != .none or self.data_layout != .none or self.target_triple != .none) {
@@ -9539,7 +9539,7 @@ pub fn printUnbuffered(
             if (variable.global.getReplacement(self) != .none) continue;
             const global = variable.global.ptrConst(self);
             metadata_formatter.need_comma = true;
-            defer metadata_formatter.need_comma = undefined;
+            defer metadata_formatter.need_comma = null;
             try writer.print(
                 \\{} ={}{}{}{}{ }{}{ }{} {s} {%}{ }{, }{}
                 \\
@@ -9569,7 +9569,7 @@ pub fn printUnbuffered(
             if (alias.global.getReplacement(self) != .none) continue;
             const global = alias.global.ptrConst(self);
             metadata_formatter.need_comma = true;
-            defer metadata_formatter.need_comma = undefined;
+            defer metadata_formatter.need_comma = null;
             try writer.print(
                 \\{} ={}{}{}{}{ }{} alias {%}, {%}{}
                 \\
@@ -9641,7 +9641,7 @@ pub fn printUnbuffered(
         });
         {
             metadata_formatter.need_comma = false;
-            defer metadata_formatter.need_comma = undefined;
+            defer metadata_formatter.need_comma = null;
             try writer.print("{ }{}", .{
                 function.alignment,
                 try metadata_formatter.fmt(" !dbg ", global.dbg),
@@ -9830,7 +9830,7 @@ pub fn printUnbuffered(
                             extra.@"else".toInst(&function).fmt(function_index, self),
                         });
                         metadata_formatter.need_comma = true;
-                        defer metadata_formatter.need_comma = undefined;
+                        defer metadata_formatter.need_comma = null;
                         switch (extra.weights) {
                             .none => {},
                             .unpredictable => try writer.writeAll("!unpredictable !{}"),
@@ -9874,7 +9874,7 @@ pub fn printUnbuffered(
                         for (0.., args) |arg_index, arg| {
                             if (arg_index > 0) try writer.writeAll(", ");
                             metadata_formatter.need_comma = false;
-                            defer metadata_formatter.need_comma = undefined;
+                            defer metadata_formatter.need_comma = null;
                             try writer.print("{%}{}{}", .{
                                 arg.typeOf(function_index, self).fmt(self),
                                 extra.data.attributes.param(arg_index, self).fmt(self),
@@ -10113,7 +10113,7 @@ pub fn printUnbuffered(
                         );
                         try writer.writeAll("  ]");
                         metadata_formatter.need_comma = true;
-                        defer metadata_formatter.need_comma = undefined;
+                        defer metadata_formatter.need_comma = null;
                         switch (extra.data.weights) {
                             .none => {},
                             .unpredictable => try writer.writeAll("!unpredictable !{}"),
@@ -10161,7 +10161,7 @@ pub fn printUnbuffered(
             try printEscapedString(name.slice(self), .quote_unless_valid_identifier, writer);
             try writer.writeAll(" = !{");
             metadata_formatter.need_comma = false;
-            defer metadata_formatter.need_comma = undefined;
+            defer metadata_formatter.need_comma = null;
             for (elements) |element| try writer.print("{}", .{try metadata_formatter.fmt("", element)});
             try writer.writeAll("}\n");
         }
@@ -10174,7 +10174,7 @@ pub fn printUnbuffered(
             @setEvalBranchQuota(10_000);
             try writer.print("!{} = ", .{metadata_index});
             metadata_formatter.need_comma = false;
-            defer metadata_formatter.need_comma = undefined;
+            defer metadata_formatter.need_comma = null;
 
             const key = metadata_formatter.map.keys()[metadata_index];
             const metadata_item = switch (key) {

This would have prevented the bug and would prevent future bugs.

[andrewrk]

I don't think I understand your message here. How would it check whether it's null without it being an optional?

It can only check whether it's null if it is an optional. It does not check whether it's null, therefore it is not an optional.

This would have prevented the bug and would prevent future bugs.

No, it would have had the same bug - illegal behavior at the .? the same as if the value was undefined.

The difference is in whether the illegal behavior is checkable or not. Where possible, illegal behavior checkability should be improved in the compiler and language rather than requiring user code to work around lack of checkability. For example, #63.

[Rexicon226]

No, it would have had the same bug - illegal behavior at the .? the same as if the value was undefined.

I understand your argument here, but yes, it would have prevented the bug from being merged. While implemented runtime checking for undefined would have also caught the bug, the fact of the matter is that right now we don’t have such a feature. Relying on something that does not yet exist seems a bit harmful to me.

Would it not be better to go for the existing “checked illegal behavior” option for now, until #63 is implemented?

Either way, just a thought, I’m fine just fixing the UB in this PR.

[andrewrk]

Would it not be better to go for the existing “checked illegal behavior” option for now, until #63 is implemented?

No. It's better to model the data accurately, and then bend the language and compiler around an accurate data model, than to form conventions that work around deficiencies in an incomplete language.