libc: implement common abs for various integer sizes

[kada49]

Implemented common libc functions:

• abs
• imaxabs
• labs
• llabs

abs functions for floating point numbers are already implemented in compiler_rt.

[alexrp]

abs functions for floating point numbers are already implemented in compiler_rt.

That means you can delete the duplicate libc implementations.

[alexrp]

Looks good. I think it would be nice to use @abs in the implementation, but it's not required.

[kada49]

I tried this locally (linux) and it works with @abs but what did I break for windows and macos?

[nektro]

#23937

[andrewrk]

@alexrp please be more careful merging contributor contributions to libc functions. This was not sufficiently reviewed or tested.

This undefined behavior needs to be promptly reverted or fixed.

[andrewrk]

@intCast is not correct here.

[mlugg]

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):

If the result cannot be represented, the behavior is undefined. [265]
[...]
265) The absolute value of the most negative number cannot be represented in two’s complement.

[andrewrk]

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

As long as it's functionally compatible with the musl implementation, you're free to implement it in a more "Ziggy" way.

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.

[mlugg]

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.

[...] the previous implementation, which does not invoke UB [...]

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.

[andrewrk]

-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:

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.

and

It's pretty clear that this UB was an accident, that just so happens to comply with the C standard.

[andrewrk]

Well, looks like I am wrong again:

andy@bark ~/tmp> cat test.c
#include <stdio.h>
#include <limits.h>

int main() {
    int x = -2147483648;
    printf("x = %d\n", -x);
    return 0;
}
andy@bark ~/tmp> zig-dev run test.c -lc
thread 27213 panic: negation of -2147483648 cannot be represented in type 'int'
/home/andy/tmp/test.c:6:24: 0x10134de in main (test.c)
    printf("x = %d\n", -x);
                       ^
???:?:?: 0x7f92c89ce27d in ??? (libc.so.6)
Unwind information for `libc.so.6:0x7f92c89ce27d` was not available, trace may be incomplete

fish: Job 1, 'zig-dev run test.c -lc' terminated by signal SIGABRT (Abort)

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.

[mlugg]

intentional UB should be noted

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?

[andrewrk]

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.

[alexrp]

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.