Re: [PATCH] bind.2, mount_setattr.2, openat2.2, perf_event_open.2, pidfd_send_signal.2, recvmmsg.2, seccomp_unotify.2, select_tut.2, sendmmsg.2, set_thread_area.2, sysctl.2, bzero.3, getaddrinfo.3, getaddrinfo_a.3, getutent.3, mbrtowc.3, mbsinit.3, rti...

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Wilco,

On 1/6/23 16:53, Wilco Dijkstra wrote:
Hi Alex,

Which C libraries never supported bzero(3)?  It was in POSIX once, so I guess
it's supported everywhere in Unix(-like) systems (you can see that I don't care
at all about other systems).  Even if only for backwards compatibility, the
removal from POSIX will have not affected the portability of bzero(3) in source
code (even where libc has removed it, the compiler will provide support).

These functions have caused portability issues since many UNIX systems didn't
support them, neither did Windows nor most of the embedded world. So they
always required extra work - if you do the codesearch on bzero you will find
many examples of those hacks in existing code.

You may not care about anything outside Linux,

I do care about non-Linux.  What I don't care about is non-POSIX.

but many libcs that support
bzero are not optimized. Even GLIBC used a slow C implementation for bzero
until we changed it to call memset. I have no idea what all other libs do (and
given bzero is dead, it doesn't even matter), but bad performance is also a
portability issue.

So, I don't think that's a real problem yet.  We're not yet (or I believe so) in
a point where bzero(3) is non-portable in source code.

It never was portable or well optimized, which were reasons to deprecate it.

Okay, I guess I'll drop the bzero(3) patch for the man-pages, and maybe write an alternative patch documenting at least some of this discussion.


Even simpler: it is unconditionally defined to memcpy() + len in a macro.
The reason (I guess) is that they didn't even know that mempcpy() exists.

But that means it will never use mempcpy - not in the source, not in the binary.
So they have done the right thing, and there is no argument that adding or
optimizing mempcpy in C libraries will improve nginx.

Actually, gcc optimizes differently.  When you call mempcpy(3), since it knows
it exists, it calls it or replaces it by memcpy(3)+len, depending on
optimization flags.  When you call memcpy(3)+len, since it doesn't know if
mempcpy(3) is available, it keeps the memcpy(3) call always.

I don't care what -O0 does, what matters is that in almost all cases mempcpy
gets optimized into memcpy, and that is the right thing to do.

src/nxt_h1proto.c:2290:    p = nxt_cpymem(p, " HTTP/1.1\r\n", 11);
src/nxt_h1proto.c:2291:    p = nxt_cpymem(p, "Connection: close\r\n", 19);

Sure but one could equally argue that returning src + len is useful too:

p = memscpy (dest1, p, size1);
p = memscpy (dest2, p, size2);

Be honest, how much do you think this would be used? ;)


Or return the size so you can easily keep track of how much bytes were copied:

copied += memncpy (dest1, src1, size1);
copied += memncpy (dest2, src2, size2);

This kind of interface is error-prone. In memcpy(3) it's not a problem, because there's not truncation, but in other functions it has been the cause of bugs.

snprintf(3) has that interface, and that one causes bugs. p += snprintf() is always wrong. If there's truncation (and you can't know it before-hand, or you'd be calling sprintf(3)), 'p' will overflow, and the behavior is undefined.

That's why I proposed stpeprintf() recently, to prevent such issues.

Having more interfaces promoting that kind of return value is wrong. I'd rather remove all interfaces that return the length of the resulting string, when a pointer can be returned.


And there are lots of other possibilities.

Actually not many.

For an interface that copies n bytes from s to d, the possible return values are (ignoring the possibility of returning the result of arc4random(3) :P):

-  (void)
-  s
-  d
-  s + n
-  d + n
-  n

Ignoring void for obvious reasons...

s or s+n are useless, because memcpy(3) is way more frequently used to create a new string by catenation of many source strings, compared to the few times it is used to copy a single source string into many destination strings.

n interfaces are error-prone, and difficult to use; I wouldn't recommend adding yet another one.

d is useless, because that's a value that's already there before the call, and most calls to memcpy(3) that don't ignore the return value are going to add the length to it. Isn't that a good hint that the good choice would have been d+n from the beginning?

So who is to say that mempcpy is better
than all these options?

I do. And considering that there are projects that reimplement mempcpy(), but I haven't seen any that implements 'memscpy()' or 'memncpy()' (as suggested above), I'd say some other programmers also do.


 From a source code point of view, they let programmers write better/simpler
source code than memcpy(3) or memset(3).  That's sugar... yes.  IMO, it's worth it.

Exactly, it's an opinion/personal preference. As I showed, there are other
possible return values, so should we add all of these interfaces just because
some people might like them?

I'd first wait to see where are those people :)


Having it in libc rather than an external library has the benefit that it will
have support from the compiler (better warnings and optimizations).

No. Compiler and libc support are totally different things. If your library is
deemed useful and used in lots of projects, it may be reasonable to add the
headers to GLIBC. But this will not affect compiler optimization - it would
use the same header and produce the same code.

GCC can see patterns in code and replace them by the function, but only if that function is standard (being in glibc is not enough). If the function is in an external library, such optimizations can't be performed.


Cheers,
Wilco

Cheers,

Alex

P.S.: To compensate for the time I'm taking from you, I'm preparing a patch to remove rawmemchr(3) from glibc. I'll send it when it's "ready". Although since I haven't sent many patches to glibc, I guess it will still be far from ready ;)

--
<http://www.alejandro-colomar.es/>

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Kernel Documentation]     [Netdev]     [Linux Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux