Commit 3fefdc1a0b

Evan Haas <evan@lagerdata.com>
2021-11-19 23:00:18
translate-c: Allow negative denominator in remainder (%) operator
Fixes #10176
1 parent e8112f7
Changed files (5)
lib/std/zig/c_translation.zig
@@ -346,6 +346,17 @@ test "Flexible Array Type" {
     try testing.expectEqual(FlexibleArrayType(*const volatile Container, c_int), [*c]const volatile c_int);
 }
 
+/// C `%` operator for signed integers
+/// C standard states: "If the quotient a/b is representable, the expression (a/b)*b + a%b shall equal a"
+/// The quotient is not representable if denominator is zero, or if numerator is the minimum integer for
+/// the type and denominator is -1. C has undefined behavior for those two cases; this function has safety
+/// checked undefined behavior
+pub fn signedRemainder(numerator: anytype, denominator: anytype) @TypeOf(numerator, denominator) {
+    std.debug.assert(@typeInfo(@TypeOf(numerator, denominator)).Int.signedness == .signed);
+    if (denominator > 0) return @rem(numerator, denominator);
+    return numerator - @divTrunc(numerator, denominator) * denominator;
+}
+
 pub const Macros = struct {
     pub fn U_SUFFIX(comptime n: comptime_int) @TypeOf(promoteIntLiteral(c_uint, n, .decimal)) {
         return promoteIntLiteral(c_uint, n, .decimal);
src/translate_c/ast.zig
@@ -128,8 +128,8 @@ pub const Node = extern union {
         helpers_promoteIntLiteral,
         /// @import("std").meta.alignment(value)
         std_meta_alignment,
-        /// @rem(lhs, rhs)
-        rem,
+        /// @import("std").zig.c_translation.signedRemainder(lhs, rhs)
+        signed_remainder,
         /// @divTrunc(lhs, rhs)
         div_trunc,
         /// @boolToInt(operand)
@@ -310,7 +310,7 @@ pub const Node = extern union {
                 .bit_xor,
                 .bit_xor_assign,
                 .div_trunc,
-                .rem,
+                .signed_remainder,
                 .int_cast,
                 .as,
                 .truncate,
@@ -1293,9 +1293,10 @@ fn renderNode(c: *Context, node: Node) Allocator.Error!NodeIndex {
             const payload = node.castTag(.int_cast).?.data;
             return renderBuiltinCall(c, "@intCast", &.{ payload.lhs, payload.rhs });
         },
-        .rem => {
-            const payload = node.castTag(.rem).?.data;
-            return renderBuiltinCall(c, "@rem", &.{ payload.lhs, payload.rhs });
+        .signed_remainder => {
+            const payload = node.castTag(.signed_remainder).?.data;
+            const import_node = try renderStdImport(c, &.{ "zig", "c_translation", "signedRemainder" });
+            return renderCall(c, import_node, &.{ payload.lhs, payload.rhs });
         },
         .div_trunc => {
             const payload = node.castTag(.div_trunc).?.data;
@@ -2207,7 +2208,7 @@ fn renderNodeGrouped(c: *Context, node: Node) !NodeIndex {
         .noreturn_type,
         .@"anytype",
         .div_trunc,
-        .rem,
+        .signed_remainder,
         .int_cast,
         .as,
         .truncate,
src/translate_c.zig
@@ -1620,10 +1620,10 @@ fn transBinaryOperator(
         },
         .Rem => {
             if (cIsSignedInteger(qt)) {
-                // signed integer division uses @rem
+                // signed integer remainder uses std.zig.c_translation.signedRemainder
                 const lhs = try transExpr(c, scope, stmt.getLHS(), .used);
                 const rhs = try transExpr(c, scope, stmt.getRHS(), .used);
-                const rem = try Tag.rem.create(c.arena, .{ .lhs = lhs, .rhs = rhs });
+                const rem = try Tag.signed_remainder.create(c.arena, .{ .lhs = lhs, .rhs = rhs });
                 return maybeSuppressResult(c, scope, result_used, rem);
             }
         },
@@ -3831,7 +3831,7 @@ fn transCreateCompoundAssign(
             if (requires_int_cast) rhs_node = try transCCast(c, scope, loc, lhs_qt, rhs_qt, rhs_node);
             const operands = .{ .lhs = lhs_node, .rhs = rhs_node };
             const builtin = if (is_mod)
-                try Tag.rem.create(c.arena, operands)
+                try Tag.signed_remainder.create(c.arena, operands)
             else
                 try Tag.div_trunc.create(c.arena, operands);
 
@@ -3871,7 +3871,7 @@ fn transCreateCompoundAssign(
         if (requires_int_cast) rhs_node = try transCCast(c, scope, loc, lhs_qt, rhs_qt, rhs_node);
         const operands = .{ .lhs = ref_node, .rhs = rhs_node };
         const builtin = if (is_mod)
-            try Tag.rem.create(c.arena, operands)
+            try Tag.signed_remainder.create(c.arena, operands)
         else
             try Tag.div_trunc.create(c.arena, operands);
 
test/run_translated_c.zig
@@ -1784,4 +1784,16 @@ pub fn addCases(cases: *tests.RunTranslatedCContext) void {
         \\    return 0;
         \\}
     , "");
+
+    cases.add("Remainder operator with negative integers. Issue #10176",
+        \\#include <stdlib.h>
+        \\int main(void) {
+        \\    int denominator = -2;
+        \\    int numerator = 5;
+        \\    if (numerator % denominator != 1) abort();
+        \\    numerator = -5; denominator = 2;
+        \\    if (numerator % denominator != -1) abort();
+        \\    return 0;
+        \\}
+    , "");
 }
test/translate_c.zig
@@ -885,7 +885,7 @@ pub fn addCases(cases: *tests.TranslateCContext) void {
         \\    c = a - b;
         \\    c = a * b;
         \\    c = @divTrunc(a, b);
-        \\    c = @rem(a, b);
+        \\    c = @import("std").zig.c_translation.signedRemainder(a, b);
         \\    return 0;
         \\}
         \\pub export fn u() c_uint {
@@ -2932,9 +2932,9 @@ pub fn addCases(cases: *tests.TranslateCContext) void {
         \\        ref.* = @divTrunc(ref.*, @as(c_int, 1));
         \\        break :blk ref.*;
         \\    });
-        \\    a = @rem(a, blk: {
+        \\    a = @import("std").zig.c_translation.signedRemainder(a, blk: {
         \\        const ref = &a;
-        \\        ref.* = @rem(ref.*, @as(c_int, 1));
+        \\        ref.* = @import("std").zig.c_translation.signedRemainder(ref.*, @as(c_int, 1));
         \\        break :blk ref.*;
         \\    });
         \\    b /= blk: {