[Toybox] [PATCH] in seq.c rename itoa to toybox_itoa, to avoid clashes

[Rob Landley]

On 2/17/22 12:05 PM, enh via Toybox wrote:
> On Thu, Feb 17, 2022 at 5:23 AM John wrote:
> > This is a simple patch for seq.c to rename itoa to toybox_itoa, to avoid
> > clashes with functions in the toolchain.
>
> > As Piraty mentioned on IIRC there are 2 different implementations of itoa in
> > musl alone, so this should avoid any clashes people have with
> > implementations in their toolchain.
>
> the two musl itoa functions are both static, so there's no possibility of a
> clash (otherwise musl itself would have a problem internally!).
> 
> what was the actual issue you were seeing?

I've been waiting for the reply to Elliott's question myself. I'm happy to apply
a fix for an actual bug, but not a purely theoretical "maybe someday" bug.

But even if it did happen (which seems especially unlikely in musl since Rich is
scrupulous about namespace pollution), in theory a static function declared
earlier in the same compilation unit is going to bind before anything outside
even if some library does have its own version of it. Which means all I'd really
have to worry about is header namespace polution (#including a header with the
conflicting declaration):

$ cat boing.c
#include <stdio.h>

int exit(char *str)
{
  return printf("string was %s\n", str);
}

int main(int argc, char *argv[])
{
  int x = exit("potato");

  printf("and it returned %d\n", x);

  return 0;
}
$ gcc boing.c
boing.c:3:5: warning: conflicting types for built-in function ‘exit’
[-Wbuiltin-declaration-mismatch]
 int exit(char *str)
     ^~~~
$ ./a.out
string was potato
and it returned 18

In practice modern compilers are polluting their own namespaces all over the
place, but the option to tell it to stop is "-fno-builtin". and even without
adding that the gcc version works fine.

Of course the Android NDK version calls my builtin exit() twice and then segfaults:

$ ./a.out
string was potato
and it returned 18
string was (null)
error: TLS segment alignment in " is not a power of 2: 1

libc: error: TLS segment alignment in " is not a power of 2: 1

Aborted

But I'm guessing that's because bionic's post-main() code that gets returned to
tries to call exit() out of libc and apparently binds to mine if I redefine it.
(Seriously, I break everything. And I point out it wasn't MY code deviating from
expected behavior, it was android's... I want to say crtend.a but they don't
seem to have one? Only .so versions. Eh, it's successfully linking and running
as a static binary (which I still have to do because I haven't installed bionic
on the host) and I'm not running it under strace to try to find out where it's
pulling this stuff from...)

However, if I make my new exit() static and feed the compiler -fno-builtin, then
the NDK behaves the same way gcc did. (And oddly enough for a _static_ function
clang both works properly and doesn't warn even without -fno-builtin, while gcc
still warns about the static function not matching its expectations. Go figure.)

Anyway, the point is that even in this contrived case (where I'm fighting
compiler builtins, which itoa() is not likely to become any time soon), the
static function defined before the other function should work fine. Yes even in
the presence of a library that has its own function with the same name I use in
my code.

This is part of the reason the compiler puts -lc at the end. I had to get that
right back when I had a wrapper to rewrite the gcc command line to start with
--nostdinc --notstdlib and then add everything _back_ to link against uClibc
portability in a compiler that you could extract into an arbitrary directory and
add to the $PATH from there, defeating gcc's horrible "all my resources live at
an absolute path determined at compile time" logic:

https://github.com/landley/aboriginal/blob/master/sources/toys/ccwrap.c

The -lc goes after all the .o files to ensure local references are satisfied
first, but stuff resolved in the same compilation unit should be fine either way.

Rob

P.S. I was extremely annoyed as a teenager when I upgraded my turbo c++ version
and suddenly the function throw() in the bbs software I'd written stopped
working because the language committee had decided to make that a keyword. Since
then I maintained the uClibc toolchain using the above wrapper for a decade or
so, maintained my own tinycc fork for 3 years, helped get dynamic linking
working on a new Linux port (qualcomm heaxagon: they had static linking working
already but building x11 with static linking was just painful: yes you CAN stick
debug printfs into uClibc's dynamic loader before it's relocated itself, if by
"printf" you mean a macro that uses a char array on the stack as the buffer it's
passing to an open coded write() system call), learned all SORTS of fun nommu
weirdness about ... These days toybox has a header that #includes all the other
standard headers out of posix and and common bsd ancestry, and then if some
build environment gets weird I hit it with a rock via #ifdefs and #defines in
portability.h. Anything CAN break, at any time. The way to deal with sharp edges
is to find them and file them off.

P.P.S. "theoretically correct" and "works properly" are two different things,
which sometimes require years of repeatedly explaining at maintainers in
extensive detail:

https://lore.kernel.org/qemu-devel/4B73F8CC.6010406@suse.de/t/#md050a8a3df649c375e0ebcd74273f72ff2bf9653