Aligning memory

**awsdert** · 01-06-2020

Originally Posted by Hodor

Have you tried using valgrind and static analysis?

Code:

valgrind --tool=memcheck --track-origins=yes ./alloc

==9754== Conditional jump or move depends on uninitialised value(s)
==9754==    at 0x4D00397: mmap (in /usr/lib64/libc-2.29.so)
==9754==    by 0x4014FA: alloc_std_page (alloc.c:113)
==9754==    by 0x40180E: alloc_foo_page (alloc.c:177)
==9754==    by 0x401FD9: main (alloc.c:322)
==9754==  Uninitialised value was created by a heap allocation
==9754==    at 0x483880B: malloc (vg_replace_malloc.c:309)
==9754==    by 0x4012F6: alloc_std_data (alloc.c:51)
==9754==    by 0x4013EC: alloc_std_list (alloc.c:78)
==9754==    by 0x40177A: alloc_foo_page (alloc.c:164)
==9754==    by 0x401FD9: main (alloc.c:322)
==9754==
==9754== Syscall param mmap(start) contains uninitialised byte(s)
==9754==    at 0x4D003A6: mmap (in /usr/lib64/libc-2.29.so)
==9754==    by 0x4014FA: alloc_std_page (alloc.c:113)
==9754==    by 0x40180E: alloc_foo_page (alloc.c:177)
==9754==    by 0x401FD9: main (alloc.c:322)
==9754==  Uninitialised value was created by a heap allocation
==9754==    at 0x483880B: malloc (vg_replace_malloc.c:309)
==9754==    by 0x4012F6: alloc_std_data (alloc.c:51)
==9754==    by 0x4013EC: alloc_std_list (alloc.c:78)
==9754==    by 0x40177A: alloc_foo_page (alloc.c:164)
==9754==    by 0x401FD9: main (alloc.c:322)
==9754==

Lots of messages from clang, but these are the ones that stood out, to me, as being potentially problematic

Code:

alloc.c:223:14: Semantic Issue: warning: variable 'spot' may be uninitialized when used here
alloc.c:232:28: Semantic Issue: warning: variable 'page' may be uninitialized when used here
alloc.c:272:20: Semantic Issue: warning: cast from 'uchar *' (aka 'unsigned char *') to 'std_spot *' (aka 'struct spot *') increases required alignment from 1 to 8
alloc.c:287:25: Semantic Issue: warning: cast from 'uchar *' (aka 'unsigned char *') to 'std_spot *' (aka 'struct spot *') increases required alignment from 1 to 8

Not familiar with valgrind but currently installing it and will give a try since lines don't seem to match up, probably need to do a new upload

**awsdert** · 01-06-2020

Well did help me fix a number of leaks at least, still got the problem of segfaults at when attempting to free stuff from lua, from what I can see it seems the wrong values are getting returned, which again leads me back to the previously mentioned memmove call, the 1st buffer size is exactly the same as the value display of the incorrect pointer lua attempt to use, here's the results of valgrind for the attached file:

Code:

make --no-print-directory valgrind_alloc (in directory: /mnt/DATA/github/ffxv_cheats)
cd ../lua && make --no-print-directory
make[1]: 'all' is up to date.
clang -ggdb -fPIC -fpic -ldl -lreadline -lm -I ../lua -o alloc.elf alloc.c ../lua/lapi.o ../lua/lauxlib.o ../lua/lbaselib.o ../lua/lcode.o ../lua/lcorolib.o ../lua/lctype.o ../lua/ldblib.o ../lua/ldebug.o ../lua/ldo.o ../lua/ldump.o ../lua/lfunc.o ../lua/lgc.o ../lua/linit.o ../lua/liolib.o ../lua/llex.o ../lua/lmathlib.o ../lua/lmem.o ../lua/loadlib.o ../lua/lobject.o ../lua/lopcodes.o ../lua/loslib.o ../lua/lparser.o ../lua/lstate.o ../lua/lstring.o ../lua/lstrlib.o ../lua/ltable.o ../lua/ltablib.o ../lua/ltests.o ../lua/ltm.o ../lua/lundump.o ../lua/lutf8lib.o ../lua/lvm.o ../lua/lzio.o
valgrind --tool=memcheck --leak-check=full --show-leak-kinds=all --track-origins=yes ./alloc.elf
==28705== Memcheck, a memory error detector
==28705== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==28705== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==28705== Command: ./alloc.elf
==28705==
==28705== Invalid read of size 8
==28705==    at 0x12F84D: luaS_remove (lstring.c:177)
==28705==    by 0x121FFA: freeobj (lgc.c:737)
==28705==    by 0x1234E4: deletelist (lgc.c:1393)
==28705==    by 0x123565: luaC_freeallobjects (lgc.c:1409)
==28705==    by 0x12EB04: close_state (lstate.c:303)
==28705==    by 0x12F1B0: lua_close (lstate.c:435)
==28705==    by 0x10F9B8: main (alloc.c:370)
==28705==  Address 0x10 is not stack'd, malloc'd or (recently) free'd
==28705==
==28705==
==28705== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==28705==  Access not within mapped region at address 0x10
==28705==    at 0x12F84D: luaS_remove (lstring.c:177)
==28705==    by 0x121FFA: freeobj (lgc.c:737)
==28705==    by 0x1234E4: deletelist (lgc.c:1393)
==28705==    by 0x123565: luaC_freeallobjects (lgc.c:1409)
==28705==    by 0x12EB04: close_state (lstate.c:303)
==28705==    by 0x12F1B0: lua_close (lstate.c:435)
==28705==    by 0x10F9B8: main (alloc.c:370)
==28705==  If you believe this happened as a result of a stack
==28705==  overflow in your program's main thread (unlikely but
==28705==  possible), you can try to increase the size of the
==28705==  main thread stack using the --main-stacksize= flag.
==28705==  The main thread stack size used in this run was 8388608.
Allocating Page...
Releasing Page...
Allocating memory...
Clearing memory...
Filling memory...
Hello World!
Allocating secondary memory...
Reallocating original memory...
Releasing memory...
Initiating Lua...
Setting at lua panic...
Opening lua libs...
Closing Lua...
==28705==
==28705== HEAP SUMMARY:
==28705==     in use at exit: 160 bytes in 1 blocks
==28705==   total heap usage: 5 allocs, 4 frees, 4,496 bytes allocated
==28705==
==28705== 160 bytes in 1 blocks are still reachable in loss record 1 of 1
==28705==    at 0x483AB65: calloc (vg_replace_malloc.c:762)
==28705==    by 0x10E8A7: alloc_std_data (alloc.c:61)
==28705==    by 0x10EA04: alloc_std_list (alloc.c:92)
==28705==    by 0x10ED95: alloc_foo_page (alloc.c:175)
==28705==    by 0x10F432: alloc_foo_data (alloc.c:295)
==28705==    by 0x10F5AE: foo_lua_alloc (alloc.c:317)
==28705==    by 0x12EE3D: lua_newstate (lstate.c:381)
==28705==    by 0x10F92F: main (alloc.c:362)
==28705==
==28705== LEAK SUMMARY:
==28705==    definitely lost: 0 bytes in 0 blocks
==28705==    indirectly lost: 0 bytes in 0 blocks
==28705==      possibly lost: 0 bytes in 0 blocks
==28705==    still reachable: 160 bytes in 1 blocks
==28705==         suppressed: 0 bytes in 0 blocks
==28705==
==28705== For lists of detected and suppressed errors, rerun with: -s
==28705== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
make: *** [makefile:58: valgrind_alloc] Segmentation fault (core dumped)
Compilation failed.

'bout time I got some sleep so I'll leave this topic for tonight, if I'm lucky one of you guys will figure out it out and post the fix by morning, if not then I'll just continue trying

**Hodor** · 01-06-2020

Possibly important. The lua docs say this about lua_Alloc()

The type of the memory-allocation function used by Lua states. The allocator function must provide a functionality similar to realloc, but not exactly the same. Its arguments are ud, an opaque pointer passed to lua_newstate; ptr, a pointer to the block being allocated/reallocated/freed; osize, the original size of the block; nsize, the new size of the block. ptr is NULL if and only if osize is zero. When nsize is zero, the allocator must return NULL; if osize is not zero, it should free the block pointed to by ptr. When nsize is not zero, the allocator returns NULL if and only if it cannot fill the request. When nsize is not zero and osize is zero, the allocator should behave like malloc. When nsize and osize are not zero, the allocator behaves like realloc. Lua assumes that the allocator never fails when osize >= nsize.

Note that if you add some dummy code in your foo_lua_alloc() right at the beginning of the function, like this

Code:

    int x = 1;
    if (addr == NULL && size != 0) {
        x = 0;
    }

This does nothing but you can set a breakpoint on the line x = 0. The part I highlighted in bold (ptr is NULL if and only if osize is zero) is apparently not true. Setting the breakpoint you can see that Lua is passing in NULL for addr with size (osize) not zero all the time. So I'm not sure what Lua wants you to do because the documentation seems incorrect. I suspect that your problem is this little bit of code

Code:

    if ( addr ) {
        temp.data = addr;
        temp.size = size;
    }
    else size = 0;

Because when called by luaL_openlibs() addr is NULL and size != 0 frequently. Of course it's possible that the part I wrote in bold is true and Lua is calling the alloc function blindly but I find that hard to believe. What Lua wants you to do when addr == NULL && size != 0 is a bit of a mystery though. I tried removing the else size = 0, but still segfaults.

I guess my suggestion would be to add that "dummy code", add the breakpoint (in your program addr == NULL && size != 0 is only ever true when called from Lua) and backtrace into the Lua code to see if it sheds any light on things. I'm not compiling Lua myself :P Then again, using the example alloc function from the Lua documentation works fine, so the problem is somewhere in your code probably because your code doesn't behave exactly like realloc(). Not sure

**awsdert** · 01-07-2020

Originally Posted by Hodor

Possibly important. The lua docs say this about lua_Alloc()

Note that if you add some dummy code in your foo_lua_alloc() right at the beginning of the function, like this

Code:

    int x = 1;
    if (addr == NULL && size != 0) {
        x = 0;
    }

This does nothing but you can set a breakpoint on the line x = 0. The part I highlighted in bold (ptr is NULL if and only if osize is zero) is apparently not true. Setting the breakpoint you can see that Lua is passing in NULL for addr with size (osize) not zero all the time. So I'm not sure what Lua wants you to do because the documentation seems incorrect. I suspect that your problem is this little bit of code

Code:

    if ( addr ) {
        temp.data = addr;
        temp.size = size;
    }
    else size = 0;

Because when called by luaL_openlibs() addr is NULL and size != 0 frequently. Of course it's possible that the part I wrote in bold is true and Lua is calling the alloc function blindly but I find that hard to believe. What Lua wants you to do when addr == NULL && size != 0 is a bit of a mystery though. I tried removing the else size = 0, but still segfaults.

I guess my suggestion would be to add that "dummy code", add the breakpoint (in your program addr == NULL && size != 0 is only ever true when called from Lua) and backtrace into the Lua code to see if it sheds any light on things. I'm not compiling Lua myself :P Then again, using the example alloc function from the Lua documentation works fine, so the problem is somewhere in your code probably because your code doesn't behave exactly like realloc(). Not sure

First point compiling lua was actually really easy, it was finding out where and what flags to add to make debuggable that took the extra time (btw I downloaded lua from github, not the official zip or whatever it was).

2nd point In light of yourstatments about lua's expectations I 've now change the code to this:

Code:

void* foo_lua_alloc( void *data, void *addr, size_t size, size_t want ) {
	std_data temp = {0};
	if ( want && addr ) {
		temp.data = addr;
		temp.size = size;
	}
	else if ( addr && size ) {
		temp.data = addr;
		temp.size = size;
	}
	if ( (errno = alloc_foo_data( &temp, want )) != EXIT_SUCCESS 
		|| !(temp.data) ) {
		if ( size >= want )
			return addr;
		return NULL;
	}
	if ( size < want )
		memset( &(((uchar*)(temp.data))[size]), 0, want - size );
	return temp.data;
}

Still get the same segfault though

3rd point when addr is NULL size merely refers to the type of object to be returned, normally a lua type which a plain malloc/calloc will do fine for, the rest of the time however is likely user defined types

**Hodor** · 01-07-2020

Originally Posted by awsdert

First point compiling lua was actually really easy, it was finding out where and what flags to add to make debuggable that took the extra time (btw I downloaded lua from github, not the official zip or whatever it was).

2nd point In light of yourstatments about lua's expectations I 've now change the code to this:

Code:

void* foo_lua_alloc( void *data, void *addr, size_t size, size_t want ) {
    std_data temp = {0};
    if ( want && addr ) {
        temp.data = addr;
        temp.size = size;
    }
    else if ( addr && size ) {
        temp.data = addr;
        temp.size = size;
    }
    if ( (errno = alloc_foo_data( &temp, want )) != EXIT_SUCCESS 
        || !(temp.data) ) {
        if ( size >= want )
            return addr;
        return NULL;
    }
    if ( size < want )
        memset( &(((uchar*)(temp.data))[size]), 0, want - size );
    return temp.data;
}

Still get the same segfault though

3rd point when addr is NULL size merely refers to the type of object to be returned, normally a lua type which a plain malloc/calloc will do fine for, the rest of the time however is likely user defined types

Yeah, based on the Lua example code it just wants things to behave like realloc(). Unfortunately I find it difficult to understand your code and couldn't pinpoint the problem. The only comments in your code are not helpful to me (e.g. [paraphrased] "make things segfault early" with no indication how the code is causing a segfault. Multiple early returns makes it difficult for me to reason about that state of variables and their current state: the code is not structured IMO and I guess I'm not experienced enough because a lot of it seems like following a strand of spaghetti in a bowl of spaghetti and very difficult for me to follow even in a debugger. Maybe add some assert() calls so that pre-conditions, post-conditions, current states, loop invariants and stuff like that things are easier to identify and reason about. Or restructure the code so that mere mortals like myself can conclude these things without help). I think that programmers more experienced than me need to look at it because I'm used to looking at code that is structured.

Another suggestion, if you're willing to listen, is for example:

Code:

    if ( want % size ) size *= (want / size)+1;

Don't write code like that. It's relatively opaque for a start and, worse, you can't set a breakpoint on the statement that executes when the condition is true

Code:

    if ( want % size ) 
        size *= (want / size)+1;  // now I can set a breakpoint here

There's no reason why your version is on one line. It makes debugging hard

Also a comment would not go astray even if you think it's obvious what it's doing (I know what it's doing but I still cannot set a breakpoint when the condition is true). I prefer to write code that is structured, commented, can be used in a debugger and is maintainable. But, yeah, I'm probably not experienced enough so forgive me

I'm not trying to be mean, I honestly want to help or I'd not have spent over an hour looking at your code. But, really, it's pretty convoluted. E.g.

Code:

init = (std_spot*)
                (&(((uchar*)(page->last->data.data))[page->last->upto]));

Do you really think you will be able to understand this code in a few months time?

**awsdert** · 01-07-2020

I found it! I forgot that when the alloc_foo_data determines there is not enough space in the already allocated memory it hands of to the default allocation loop, only after finding memory the loop never copy the original memory, also never did that when it takes a whole page either, and for the final kicker it didn't free memory when it gave way to the default allocation method.

**Hodor** · 01-07-2020

Originally Posted by awsdert

I found it! I forgot that when the alloc_foo_data determines there is not enough space in the already allocated memory it hands of to the default allocation loop, only after finding memory the loop never copy the original memory, also never did that when it takes a whole page either, and for the final kicker it didn't free memory when it gave way to the default allocation method.

Well, that's great news! Well done

I doubt I could have found the problem. Seriously, consider writing structured code and writing meaningful comments. I'm just trying to help, honestly. Early returns, continue, etc etc have their place and I use them regularly, but when it gets in the way of understanding the code it's time to step back and ask yourself "is there a better way to write this?". Also, I found your naming of parameters confusing. If one function uses 'data' and it calls a function that has a parameter called 'data', and those things are completely different, it's confusing. Additionally:

Code:

typedef struct list {
    struct data data;
    long used;
    long upto;
    long perN;
} std_list;

This gives me no idea what things are meant to be doing. Normally I look at a struct and can tell immediately what it's (the code) doing based on the struct's name and its member names. Is the above struct meant to represent a linked list? I have no idea. A linked list normally has pointers so my inclination is to say no it's not a linked list. So what is it? Yeah there is a member of type struct data, but that just has a void *data member and no linkage. So... I can't tell.

**awsdert** · 01-07-2020

Originally Posted by Hodor

Well, that's great news! Well done

I doubt I could have found the problem. Seriously, consider writing structured code and writing meaningful comments. I'm just trying to help, honestly. Early returns, continue, etc etc have their place and I use them regularly, but when it gets in the way of understanding the code it's time to step back and ask yourself "is there a better way to write this?". Also, I found your naming of parameters confusing. If one function uses 'data' and it calls a function that has a parameter called 'data', and those things are completely different, it's confusing. Additionally:

Code:

typedef struct list {
    struct data data;
    long used;
    long upto;
    long perN;
} std_list;

This gives me no idea what things are meant to be doing. Normally I look at a struct and can tell immediately what it's (the code) doing based on the struct's name and its member names. Is the above struct meant to represent a linked list? I have no idea. A linked list normally has pointers so my inclination is to say no it's not a linked list. So what is it? Yeah there is a member of type struct data, but that just has a void *data member and no linkage. So... I can't tell.

1st point there is still a bug somewhere, I think it's in the reallocation section because I can't see it being elsewhere
2nd point the names and structures are deliberately abstract because they are also used elsewhere, they're only meant to provide bare bone information while the functions using them aught to know more
3rd point I do agree on the whole more comments thing, I'm just bad at remembering
I think I had another point but I forgotten now, anyway the other bug is not breaking enough to be the reason for my next thread so I'll start that one now and come back to this later when I have no choice or just feel like it

**Hodor** · 01-07-2020

Originally Posted by awsdert

1st point there is still a bug somewhere, I think it's in the reallocation section because I can't see it being elsewhere
2nd point the names and structures are deliberately abstract because they are also used elsewhere, they're only meant to provide bare bone information while the functions using them aught to know more
3rd point I do agree on the whole more comments thing, I'm just bad at remembering
I think I had another point but I forgotten now, anyway the other bug is not breaking enough to be the reason for my next thread so I'll start that one now and come back to this later when I have no choice or just feel like it

The names are not meant to be abstract (the data types can, of course, can/should/could be).

As for variable names, if in function1 you call a parameter "size", then calling it "want" in another function is just confusing. Call them size in both. If in function1 you have something called "data" and that function1 calls function2 then make sure that if function2 also has a parameter called "data" that they're the same type. Doing otherwise is bound to cause confusion. Naming a parameter "what" is no help at all.

Going back to data types being abstract, you don't have to obfuscate the code to make them abstract. Is std_list an abstract data type of a linked list? I have no idea. Abstraction is generally not about providing bare bone information, to the caller, it's about providing no information at all. But in the implementation of the ADT you need to have the information and the information needs to be readable by a human. If it's not readable by a human then it cannot be maintained.

As for the early returns and stuff... I know it's tempting, and in some cases it can lead to better code. But the problem is that if you have heaps of early returns it's difficult to understand the state of variables if the program execution gets past the early returns. I.e. If you have, say, 6 early returns based on whatever conditions then it's really, really hard to reason about the state of variables and conditions that have been met if the early returns do not happen. The early returns (although they have their place) make the code unstructured and this lack of structure is what makes things hard to understand.

Edit: Just to be clear, I'm not saying this just to be mean or whatever. I'm saying it because I think if you adopt better approaches you will be a better programmer and you'll also write code that's more maintainable

Thread: Aligning memory

Thread Tools

Search Thread

Display

Similar Threads

Help With Aligning

Right aligning.

Aligning Columns

Aligning text

Tags for this Thread