-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
libc: implement common abs
for various integer sizes
#23893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
93b5ff9
58f1637
060e066
b30db23
d682fa5
c9a33d4
18a2714
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
const std = @import("std"); | ||
const common = @import("common.zig"); | ||
const builtin = @import("builtin"); | ||
const intmax_t = std.c.intmax_t; | ||
|
||
comptime { | ||
if (builtin.target.isMuslLibC() or builtin.target.isWasiLibC()) { | ||
// Functions specific to musl and wasi-libc. | ||
@export(&imaxabs, .{ .name = "imaxabs", .linkage = common.linkage, .visibility = common.visibility }); | ||
} | ||
} | ||
|
||
fn imaxabs(a: intmax_t) callconv(.c) intmax_t { | ||
return @intCast(@abs(a)); | ||
} | ||
|
||
test imaxabs { | ||
const val: intmax_t = -10; | ||
try std.testing.expectEqual(10, imaxabs(val)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
const std = @import("std"); | ||
const common = @import("common.zig"); | ||
const builtin = @import("builtin"); | ||
|
||
comptime { | ||
if (builtin.target.isMuslLibC() or builtin.target.isWasiLibC()) { | ||
// Functions specific to musl and wasi-libc. | ||
@export(&abs, .{ .name = "abs", .linkage = common.linkage, .visibility = common.visibility }); | ||
@export(&labs, .{ .name = "labs", .linkage = common.linkage, .visibility = common.visibility }); | ||
@export(&llabs, .{ .name = "llabs", .linkage = common.linkage, .visibility = common.visibility }); | ||
} | ||
} | ||
|
||
fn abs(a: c_int) callconv(.c) c_int { | ||
return @intCast(@abs(a)); | ||
} | ||
|
||
fn labs(a: c_long) callconv(.c) c_long { | ||
return @intCast(@abs(a)); | ||
} | ||
|
||
fn llabs(a: c_longlong) callconv(.c) c_longlong { | ||
return @intCast(@abs(a)); | ||
} | ||
|
||
test abs { | ||
const val: c_int = -10; | ||
try std.testing.expectEqual(10, abs(val)); | ||
} | ||
|
||
test labs { | ||
const val: c_long = -10; | ||
try std.testing.expectEqual(10, labs(val)); | ||
} | ||
|
||
test llabs { | ||
const val: c_longlong = -10; | ||
try std.testing.expectEqual(10, llabs(val)); | ||
} |
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@intCast
is not correct here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't it correct? The C standard specifies that
abs(minInt(c_int))
will exhibit UB assuming a two's complement representation. C99 7.20.6.1(2):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, this extremely non-obvious requirement needs to be cited in a comment in the implementation, and someone should have pointed out the motivation for adding
@intCast
rather than the previous implementation, which does not invoke UB, was intentional to better comply with the standard.Given the previous review was
It's pretty clear that this UB was an accident, that just so happens to comply with the C standard.
This also brings up another use case for implementing libc in zig: the ability to enable ubsan for the code. Other libcs such as musl will not catch it since they effectively
@bitCast
rather than@intCast
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what way is it clear this was an accident? I don't understand what you're getting at. I thought it was well-known that this is UB in C, especially given that there's... kind of no sane definition.
Huh? This implementation has just as much UB as the implementation the review comment was on, and just as much UB as the musl implementation this PR replaces; in both cases, the negation expression
-x
is what exhibits the undefined/illegal behavior in the "min int" case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-x
in C does not invoke UB, it is a bitcast.That's my mistake for not noticing the
-x
in Zig for the UB, but my main point stands:and
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, looks like I am wrong again:
I apologize, I misremembered C semantics.
However, the point stands that intentional UB should be noted along with the citation in the spec that says it is UB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could perhaps buy that, but... I imagine you'd need a lot of comments if you wanted to do that for every single instance of potential UB in ziglibc, since libc has rather a lot of it. And where are you drawing the line? Should dereferencing a pointer argument have a comment pointing out where in the spec the pointer is assumed to be valid?
I'm not saying ziglibc shouldn't have some kind of UB-commenting rule, but this is definitely a new thing you're coming up with rather than an existing convention, so I certainly don't think it was incorrect for Alex to merge this as-is. All the old implementation (previous iteration of this PR, and musl's implementation) did was hide the UB from you by disguising it behind a negation; OTOH, this implementation makes it obvious, so you know it's there, but can easily verify that it aligns with the spec. Isn't that a clear improvement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it was incorrect for Alex to merge as-is either. I apologize for the alarm.
I agree it's a clear improvement. So clear that I didn't even realize there was UB in the C code. My reaction was uncalled for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I'd be okay with a policy of adding comments for a subset of UB. As Matthew said, we obviously wouldn't want to do it everywhere, but even a subjective evaluation like "does any reviewer feel that it'd be helpful here?" is probably better than nothing.