Fixed warnings

[utoni]

Added -Wextra to CFLAGS. Replaced signed integer array access with an unsigned integer. Changed function signatures of threading routines to be compliant to POSIX threading architecture.

(BTW, thanks for this pretty neat tool) Signed-off-by: Toni Uhlig [email protected]

[johndoe31415]

Hey, thanks for the PR! Always appreciate improvements in code quality.

About the change in prototype for thread functions, there I think actually that the current version is correct, at least for the target I'm compiling for (Linux x86_64) -- and all the way back thinking about NPTL this is what it's always been:

       int pthread_create(pthread_t *thread, const pthread_attr_t *attr,
                          void *(*start_routine) (void *), void *arg);

So since your prototype seems to be

void (*start_routine) (void *)

instead of

void *(*start_routine) (void *)

Maybe we need to conditially compile this for different architectures? What system are you working under?

[utoni]

So since your prototype seems to be

void (*start_routine) (void *)

instead of

void *(*start_routine) (void *)

The exact opposite is my prototype. ;) At least Debian x64, Arch Linux x64 and OpenWrt/musl for Marvell Armada based platforms want it that way. To be honest, I've never seen another function signature for POSIX thread callbacks (but I might be wrong).

[johndoe31415]

Hmmmmm, wait, so which one are you seeing under Debian x86_64? Because I do have a debian installation:

Linux gula 4.9.0-8-amd64 #1 SMP Debian 4.9.144-3.1 (2019-02-19) x86_64
Last login: Sat Jun 20 17:22:53 2020 from x.x.x.x

gula [~]: man pthread_create | grep start_ro | head -n 1                                                                                                                                                          
                          void *(*start_routine) (void *), void *arg);

Could you double check this on your end please? Because likewise I only ever have seen the prototype where a void* is returned (so that the thread can actually return some value that can be retrieved by pthread_join when using non-detached threads).

I was kinda assuming you were using some BSD flavor which might not use NPTL (not sure about that), but Debian definitely does.

Also, when you say x64, you do mean x86_64 (i.e., amd64), right? But definitely not IA64? I'm like 95% sure but want to double check.

[johndoe31415]

Aaaaaaaah now I see. I read your patch wrong.

Okay, but this is entirely intentional. I.e., the start_detached_thread starts a thread in detached mode. This means it can never return any value and therefore the internal prototype that it accepts is what it is.

However, I see now that you're trying to fix the ugly cast, which of course becomes obsolete once the prototypes become compatible. I agree the function pointer cast is not ideal, but I also think that the prototype should remain identical. To solve this cleanly, we'd need a brief trampoline, something like (this is probably not going to compile, treat it as pseudocode):

static void *trampoline_fnc(void *arg) {
    struct trampoline_t *trampoline_data = (struct trampoline_t*)arg;
    arg->thread_fnc(arg->arg);
    free(arg);
    return NULL;
}

bool start_detached_thread(void (*thread_fnc)(void*), void *argument) {
   // ...
   struct trampoline_t trampoline_data = {
     .thread_fnc = thread_fnc,
     .arguent = argument,
   };
   struct trampoline_t *trampoline_heap_data = malloc_and_memcpy(&trampoline_data);
   pthread_create(... trampoline_fnc, trampoline_heap_data);   

I guess I was just lazy with the implementation there and went with the cast... hmmm at least I now understand your issue correctly, sorry for misreading the patch.

[utoni]

However, I see now that you're trying to fix the ugly cast, which of course becomes obsolete once the prototypes become compatible.

I think I'm missing some information here. Is there any reference you can point me to, so I can understand what prototype will be when compatible? As of void* (*thread_fnc)(void*) morphing to void (*thread_fnc)(void*) is completely new for me (and does not makes sense).