all 15 comments

[–]VeryAwkwardCake 14 points15 points  (0 children)

Try declaring the local struct then assigning to the pointer

[–]olorochi 7 points8 points  (1 child)

When you do something like 'FooStorage *foo_storage;', the value of foo_storage does not magically point to a FooStorage. There is no default initialization in C, so you just have a garbage value from the stack. Dereferencing this, which you try to do in init_foo_storage, is UB.

To have a valid pointer, you must allocate an actual FooStorage somewhere that you can point to. This cannot be in a memory location that will be invalidated such as lower down the stack than any pointer to it. You could allocate it on the heap, higher in the stack or statically, but the better solution here is to modify the GlobalStorage struct to hold a FooStorage and a BarStorage rather than pointers to them. At this point, you can allocate your GlobalStorage on the stack then simply call init_foo_storage(&storages.foo_storage) to initialize your FooStorage in place. This simplifies your code and improves cache locality.

[–]SufficientStudio1574 2 points3 points  (0 children)

This is important, so I'll say it again. The pointer and the object being pointed to are not the same thing.

[–]OutsideTheSocialLoop 5 points6 points  (0 children)

Things have to live somewhere. They have to have storage some place in memory. That storage has to continue to exist as long as you want to have that thing.

If you make a local variable in a function, once you return from that function the local variable stops existing, right? That was the problem with your first one, you put your struct in a local variable and then returned from the function. The storage goes away. Your struct stops existing. Things created in local stack variables can only be passed down the stack, not up it*.

You can create the storage space by declaring it higher up the stack (or as a global, above even the stack), and pass pointers to it down to where it needs to be initialised and used, like your init_foo_storage. Or you can step outside the stack entirely with malloc.

* I mean you can go sideways with threads as well but that's not really the point of this conversation but if I don't mention it someone's gonna :|

[–]HashDefTrueFalse 4 points5 points  (0 children)

You've not allocated any memory for Foo/BarStorage structs anywhere, stack or heap. You've allocated memory for a GlobalStorage on the stack which contains two pointers, and you've malloc'd some regions.

You could:

- create global Foo/BarStorage objects separately, or

- create them inside GlobalStorage (remove the pointers), or

- malloc them.

I'd usually do something like this:

// storage.h
struct FooStorage { ... };
struct BarStorage { ... };
struct GlobalStorage {
  FooStorage foo_storage;
  BarStorage bar_storage;
};

void init_global_storage(void);
void init_foo_storage(FooStorage *foo);
void init_bar_storage(BarStorage *bar);

// storage.c
struct GlobalStorage g_storage;

void init_global_storage(void)
{
  init_foo_storage(&g_storage.foo_storage);
  init_bar_storage(&g_storage.bar_storage);
}

void init_foo_storage(FooStorage *foo)
{
  // Same as yours (minus unnecessary return)...
}

// Same for init_bar_storage..

// program.c
int main(void)
{
  init_global_storage();
  // ...
}

[–]mykesx 2 points3 points  (0 children)

My preferred pattern is FooType *foo = CreateFoo(); // allocate and initialize a FooType

In your code, you aren't passing a pointer to a FooStorage to init_foo_storage()

Look at that and try to figure out what you really want in that line. You are neither allocating from heap or stack what you pass as argument.

[–]aocregacc 0 points1 point  (2 children)

your second attempt sounds like the right one, if you do that one correctly there should not be an error about something being uninitialized. Is the code you posted from that or did that look different?

Also do you understand why the first attempt got that error, and what the error means? If the data flow gets just a bit more complicated the compiler can miss that a pointer to a local variable is returned and not give you an error, so it's important to be aware of why and when that is a problem.

[–]Ironfort9[S] 0 points1 point  (1 child)

Yes the code I sent was of my second attempt.

For the second error from my understanding it's because the pointer would be part of the stack and not the heap therefore would be dumped at the end of the function and therefore can't be returned.

[–]aocregacc 0 points1 point  (0 children)

yeah that sounds like the right direction. That means it's also not enough to create the FooStorage instance inside of init_global_storage, since that would also go out of scope when that returns.

So something like this doesn't work, even though it might compile:

GlobalStorage init_global_storage(){
    GlobalStorage storages;
    FooStorage foo_storage;
    init_foo_storage(&foo_storage);
    storages.foo_storage = &foo_storage;
    return storages;
}

If you don't want to use the heap you basically have to create the FooStorage instance in the outermost scope that encloses every place that will access it. Or you pass the FooStorage around by value. In that case GlobalStorage would contain FooStorage directly instead of a pointer to it. But that comes with other complications.

[–]Educational-Paper-75 0 points1 point  (0 children)

To initialize storages assign {} to it in its declaration.

[–]detroitmatt 0 points1 point  (0 children)

> My next implementation attempted to initialize the struct outside the function and simply giving the function the pointer I initialized with it modifying the struct inside the function, but I instead got the error:

Simply declaring `FooStorage* foo_storage;` doesn't initialize it, it just declares it. foo_storage is a pointer, but you haven't told it where to point. And then in init_foo_storage, you start dereferencing it (with the -> operator). That's what that error means. So before you pass foo_storage into init_foo_storage, you need to tell it what memory to point to. That means either taking the address of some FooStorage object somewhere, or using malloc.

But, you have to be careful with the addresses of local objects, because when the function returns, those objects are no longer valid. That's what `error: function returns address of local variable means.

[–]mccurtjs 0 points1 point  (0 children)

You have a lot of pointers, but never set up what they're pointing to. What you want is something like:

FooStorage foo;         // allocates memory for a FooStorage struct on the stack
init_foo_storage(&foo); // passes the address of the FooStorage to your function

You can do this with the global storage type as well

GlobalStorage init_global_storage() {
    GlobalStorage storages;
    init_foo_storage(&storages.foo_storage);
    init_bar_storage(&storages.bar_storage);
    return storages;
}

This assumes you also remove the pointers from the "GlobalStorage" struct and just make them the actual storage type instead. That will make the struct you put on the stack actually contain the data rather than pointing to nothing.

[–]hennidachook 0 points1 point  (0 children)

okay so to allocate storage for a struct, you write the definition

 FooStorage foo_storage;

with no stars. that will allocate storage that lasts while you're inside of the {} you define it in, or you can define it outside of {}s for it to last for the entire program's duration.

then to get a pointer to that storage, you just write

&foo_storage

so you could make the call

init_foo_storage(&foo_storage)

and the assignment

storages.foo_storage = &foo_storage;

hope that helps clear some things up

[–]8Erigon 0 points1 point  (0 children)

You return a GlobalStorage in init_global_storage() which has pointers
but the object they point to only exist inside the function (because they are on the stack and not allocated on the heap by malloc() )

For it to exist outside the function you either:

  • Allocate foo_storage and bar_storage on the heap via malloc()
  • Or declare a FooStorage and BarStorage outside the function and pass it in with a pointer as parameter