Skip to content
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

[P0] Wrapping malloc makes cake warn about -Wmissing-destructor #148

Open
iphydf opened this issue Mar 5, 2024 · 8 comments
Open

[P0] Wrapping malloc makes cake warn about -Wmissing-destructor #148

iphydf opened this issue Mar 5, 2024 · 8 comments

Comments

@iphydf
Copy link

iphydf commented Mar 5, 2024

This makes it impossible to make malloc wrappers, which are a common pattern in C. A similar issue happens when casting the malloc result to its target pointer type (required for code that should compile as both C and C++).

#include <ownership.h>

struct Foo {
    int *owner p;
};

void *owner mymalloc(int size) {
    return malloc(size);
}

struct Foo *owner foo_new(void) {
    // change mymalloc to malloc -> no warning
    struct Foo *owner foo = mymalloc(sizeof(struct Foo));

    if (!foo) {
        return 0;
    }

    foo->p = malloc(sizeof(int));

    return foo;
}

Output:

0.770 /work/c-toxcore/toxcore/test.c:19:8: warning: memory pointed by 'foo.p' was not released before assignment. [-Wmissing-destructor]
0.770  19 |    foo->p = malloc(sizeof(int));
0.770     |       ^~
@iphydf iphydf changed the title Wrapping malloc makes cake warn about -Wmissing-destructor [P0] Wrapping malloc makes cake warn about -Wmissing-destructor Mar 5, 2024
@thradams
Copy link
Owner

thradams commented Mar 5, 2024

The static analyzer consider that functions don't return uninitialized objects.

struct X* f();

It also consider that if the pointed object exists, it is initialized. The pointer return can be null.
(notnull could be useful here in function result)

then malloc is a builtin exception. malloc it understand the pointed object is initialized.
the calloc is also a builtin function where the pointed object is zero.

suggestion:
use static_debug(x) to see the "static state"
to assert an state use static_state(var, "null") etc

The sample can be fixed :

#include <ownership.h>
#include <stdlib.h>

struct Foo {
    int *owner p;
};

void *owner mymalloc(int size) {
    return malloc(size);
}

struct Foo *owner foo_new(void) {
    // change mymalloc to malloc -> no warning
    struct Foo *owner foo = mymalloc(sizeof(struct Foo));
    static_set(foo, "uninitialized");
    if (!foo) {
        return 0;
    }

    foo->p = malloc(sizeof(int));

    return foo;
}

@thradams
Copy link
Owner

thradams commented Mar 5, 2024

New idea "state cast"

struct X * p = _Uninitialized(mymalloc(sz));
struct X * p = _Zeroed(calloc(1, sz));

@iphydf
Copy link
Author

iphydf commented Mar 5, 2024

Instead of casting, can we add flags to the function signature to express these properties? If it's cast, I need to make a macro to wrap my allocator function, which is a bit uglier than just calling it. That way, malloc doesn't need to be magic and the analyser can look at the "returns uninitialised" flag, similar to "owner".

(MyStruct *)malloc(sizeof(MyStruct)) gives an error (correctly), saying that you're casting away the owner flag.

(MyStruct *owner)malloc(sizeof(MyStruct)) is correct. If we add uninitialized, maybe it'll be:

void *owner uninitialized malloc(size_t);

...
{
    MyStruct *owner uninitialized ms = (MyStruct *owner uninitialized)malloc(sizeof(MyStruct));
    ...
}

With the same semantics as the magic malloc today?

Just some thoughts. Maybe you have better thoughts on how much annotation is necessary for the semantics to work (e.g. maybe the cast and local variable don't need the flags and can infer it from the malloc call?).

@thradams
Copy link
Owner

thradams commented Mar 5, 2024

not the case o malloc, calloc, but the problem is when the meaning is dynamic

int main()
{
   mtx_t mtx;
   if (mtx_init(&mtx, mtx_plain) == thrd_success)
   {
      static_set(mtx, "not-null");
      mtx_destroy(&mtx);
   }
}

realloc is also a sample.
Microsoft SAL has a lot of semantics for this . I am trying to avoid too much annotations.. but design is open

static_set(mtx, "not-null"); works always the problem it needs to be repeated on the caller side.

@thradams
Copy link
Owner

thradams commented Mar 5, 2024

For uninitialized _Out could be used then it the same of arguments.

_Out void * _Owner malloc(int size);

it means the pointed object (whatever it is) is uninitialized. (just like argument _Out)

@iphydf
Copy link
Author

iphydf commented Mar 5, 2024

Hmm true, if it's returned through an out-parameter, it gets trickier. Not sure how to design that yet.

@iphydf
Copy link
Author

iphydf commented Mar 5, 2024

I'll try with the manual static_set for now. I'll see how far I get.

Btw I'm trying to make https://github.com/TokTok/c-toxcore work with cake. It's a 65kloc C library, and my goal is to have 0 ownership warnings from cake. It'll be a good test for your analyser :).

@thradams
Copy link
Owner

thradams commented Mar 5, 2024

suggestion..start without -fanalyser.
This will ask you to add qualifiers and this is similar of "fixing const"
then having 0 warning without -fanalyser you can turn it on.

#pragma cake diagnostic disabled "W-" 

also may help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants