4

I implement a custom memory allocator. All my main code for this inside file memory.c I create a main function in this file to test function. All works fine. But when I move those testing code to another file (calling main.c and run it. I meet Segmentation fault.

int main (int argc, char *argv []) {
   allocator_init();
   char* ptr = (char*) allocate(4096);   // csutom function. that on `memory.c` 
   strcpy(ptr, "this is the test one");// segmentation fault here
   printf("print: %s\n", ptr);
   deallocate(ptr);
}

Here is the main code :

volatile Memory memory;


/* allocated memory to memory variable by assign /dev/zero to memory */
void allocator_init() {
    fd = open("/dev/zero", O_RDWR);
    if(fd == -1) {
        perror("File open failed");
        exit(0);
    }

    // page size can different on different platform. customize again to optimize
    PAGE_SIZE = getPageSize();

    // fd = open("ZEROES", O_RDWR);

    if(fd == -1) {
        perror("File open failed");
        exit(0);
    }

    // Initialize the region list
    memory.region = NULL;

    int i;
    /// Initialize the caches
    /// size of each cache is 16 * 2^i => 16, 32, 64, 128, 256, 512, 1024, 2048
    for (i=0; i<8; i++) {
        memory.cache[i].size = 16<<i;
        memory.cache[i].S = NULL;
    }
    return;
}

    void *allocate_region (unsigned int size) {

      Region *region, *temp;

      temp = memory.region; 
      void *mapped_addr = mmap(NULL, size + REGION_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);

      if(mapped_addr == MAP_FAILED) {
         perror("Mapping failed");
         exit(0);
       }

      /* create region from mapped address */
      region = mapped_addr;
      region->size = size;
      region->addr = mapped_addr + REGION_SIZE;
      printf("allocated region: %p\n", region->addr);

      /* assign this region to memory */
      memory.region = region;

      /* just simple append algorithm in linked list. so new region will be appended to head of linked list */
      region->next = temp;

      return region->addr;

    }

/* allocate : if size < 2048 : allocate cache. else allocate region */
void *allocate(unsigned int size) {
    size = ALIGN(size);
    return allocate_region(size);
}

Here is memory.h define all struct that i have used:

#ifndef MEMORY_H
#define MEMORY_H

#define MAX_SIZE (1024*1024)
#define REGION_SIZE sizeof(Region)
#define SLAB_SIZE sizeof(Slab)
#define WORD_SIZE 32
#define ALIGNMENT 8
/* rounds up to the nearest multiple of ALIGNMENT */
#define ALIGN(size) (((size) + (ALIGNMENT-1)) & ~0x7)
#define SIZE_T_SIZE (ALIGN(sizeof(size_t)))
#define TRUE 1
#define FALSE 0
#define MAX_SIZE 10000

// Caches are:
// 16, 32, 64, 128, 256, 512, 1024, 2048


#include "bits.h"

int PAGE_SIZE;
// File descriptor for zeroes file
int fd;

void allocator_init();
int deallocate_cache(void* ptr);
void *allocate(unsigned int size);


typedef struct __Region {
  void *addr;               /// started address can use for caller (can calc by allocated address + Region size)
  int size;                 /// size of this allocated Region (not include size Region)
  struct __Region *next;
} Region;

/// size of Slab equals to size of System Page
typedef struct __Slab {
  void *addr;               /// address of this slab (exclude header)
  char *bitmap;             /// started address can use for caller (can calc by allocated address + slab size)
  int size;
  int slots;                /// number of slots in maximum has been used
  int cache_size;            /// size of cache that contains this slab
  int bitmap_size;          /// size of bitmap. so, can count address bit by bitmap + bitmap_size
  int currentSize;          /// current allocated elements of this slab. currentSize = 0. deallocated this slab
  int bit;                  /// bit to marked which part of slab has been used
  struct __Slab * next;
} Slab;



typedef struct __Cache {
  int size;
  Slab *S;
} Cache;


typedef struct __Memory {
  Region *region;
  Cache cache[8];
} Memory;
#endif // MEMORY_H

above code will works fine, allocate function return an address. I just meet error when move to another file. in my memory.c, I have some global variable to control allocated address and memory. Does it affect when I move code to new file ? I cannot explain this.

Thanks :)

Trần Kim Dự
  • 5,872
  • 12
  • 55
  • 107
  • Are you linking/including the files correctly? Try removing the cast, if that makes the compiler complain it probably means it can't see the declaration/definition of `allocate`. Don't forget to compile with `-Wall` – Kninnug Apr 05 '14 at 18:51
  • yes. I have tried to remove cast, but still meet this error. on `memory.c` contains a global variable to control memory that I have allocated. Does it affect my code, when move outside ? Thanks :) – Trần Kim Dự Apr 05 '14 at 18:58
  • As long as that global variable is only used *inside* `memory.c` it shouldn't matter. You did not answer my question about whether you're including the prototype of `allocate` and if you're linking against `memory.c`. – Kninnug Apr 05 '14 at 19:01
  • @Kninnug yes. I have a `memory.h` file, but it just contains some struct. I don't include `allocate` or `deallocate` function to header. So, now, I try add those function to header as prototype but I still meet above error. thanks :) – Trần Kim Dự Apr 05 '14 at 19:14
  • @Kninnug I have debugged and see that address when return from allcate() function is marked as "out of bound" on watch window. Does this make strange ? thanks ) – Trần Kim Dự Apr 05 '14 at 21:05
  • That would indicate a problem in the `allocate` function. Why this would manifest only when it is moved to its own translation unit cannot be said without seeing it. You should include the definition of `allocate` in the question. – Kninnug Apr 05 '14 at 21:21
  • @Kninnug I have added a main code of allocate function. Please see that exist something wrong ? thanks :) – Trần Kim Dự Apr 05 '14 at 21:26
  • For completeness' sake you should also include the definitions of `Memory` and `Region`. – Kninnug Apr 05 '14 at 21:37
  • @Kninnug I have edited again. I have add file `memory.h` add `alocator_init()` that I will call first on main function . Please review for me. thanks :) – Trần Kim Dự Apr 05 '14 at 21:43
  • I'd guess you are missing some function declaration, which is thus getting wrong arguments. Compile wit warnings cranked up, and check them all. – vonbrand Apr 05 '14 at 21:46
  • @vonbrand I have added flag `-Wall -Wextra` and compile, I see no warning or error. Moreover, it just goes wrong when I move to new file. Before that, it works smoothly. thanks :) – Trần Kim Dự Apr 05 '14 at 21:49

1 Answers1

5

There are some observations worth making:

  1. Your header should only declare what the consumer of your code needs to know. That's three functions: allocator_init(), allocate() and deallocate_cache(). Everything else in the file is an implementation detail that should be hidden from the consumer of your code.
  2. Headers should not define variables — not even tentatively. If you aren't writing extern in front of a variable in a header, it should be prefixed with static and you'd better have a very good reason for it. (See How do I share a variable between source files in C for a complete disquisition.)
  3. Names starting with _ are reserved for the implementation to use. That's a slight over-simplification, but it covers all the bases and is easier to remember than the formally correct rules. Names starting with __ are definitely off-limits (100%; no simplification involved). Don't define such names for yourself, and be extremely cautious about using such names in your code (they're usually types you probably shouldn't be using). I simply removed the leading underscores from the structure tags; it works happily.
  4. Your header included "bits.h", but fortunately your code didn't use anything from it, so commenting it out worked fine.
  5. Your code uses getPageSize() but doesn't define it. Probably excusable; I used 4096 instead. And tracking this down showed that PAGE_SIZE is a variable defined in the header, leading to the start of this diatribe polemic explaining what to to. It should be a static variable in the memory.c file. The only things visible from the outside of the source file should be those functions (and perhaps global variables) that consumers need to use. Everything else should be static so it is invisible — no name conflicts, no possibility of misuse or abuse.
  6. When it is made into a static variable, the compiler complains that PAGE_SIZE is unused. That, incidentally, is another advantage of making everything that can be static into a static definition — when the code could be accessed outside the source because it is not static, the compiler can't warn about unused variables or functions.
  7. Your header defined two different values for MAX_SIZE; don't!
  8. Your header defined unused values, such as #define TRUE 1; again, don't. (MAX_SIZE wasn't used, either, nor FALSE, nor SLAB_SIZE, …)
  9. Your ALIGN macro is faulty:

     #define ALIGN(size) (((size) + (ALIGNMENT - 1)) & ~0x7)
    

    As it stands, ALIGNMENT is 8, and the macro works. But suppose I change ALIGNMENT to 16; then the mask is wrong. It should probably be & ~(ALIGNMENT - 1). Beware of apparent parameterization that is incomplete.

  10. If you exit because of an error, you should not exit with status 0; that means success. Either use 1 or EXIT_FAILURE — or follow some other locally relevant scheme.
  11. Your header declared deallocate_cache() but your main program calls deallocate() and the code for neither was given. The naming inconsistency is problematic. I used a dummy implementation.

Obviously, the necessary headers had to be added, too. OK; with that lot fixed, the code compiled and linked. Then I ran into:

Mapping failed: Operation not supported by device

This was Mac OS X 10.9.2 Mavericks. Just an FYI. I worked around it by creating an empty file ./dev.zero and referencing that. Beware portability assumptions.

Then it crashed with a bus error, signal 10. It didn't print the allocated region message. That limits the damage to 3 lines of code.

You cannot do arithmetic on void * types in standard C. GCC does allow you to do it; don't take advantage of that, though, in code that has any pretensions to portability.

When I created an empty file, the program crashed. When I used dd if=/dev/zero of=dev.zero bs=1k count=1024 to initialize the file to all zeros, the program no longer crashed. I'd added a bunch of debug printing code.

I suggest not using /dev/zero for your mapping file.

Mapping succeeded: 0x10ca74000
region = 0x10ca74000
size = 4096
allocated region: 0x10ca74018
Memory: allocate_region (0x10ca40080)
Region: Base (0x10ca74000)
Address: 0x10ca74018, size:   4096, next = 0x0
Cache: line 0 (0x10ca40088)
Size: 16
Cache: line 1 (0x10ca40098)
Size: 32
Cache: line 2 (0x10ca400a8)
Size: 64
Cache: line 3 (0x10ca400b8)
Size: 128
Cache: line 4 (0x10ca400c8)
Size: 256
Cache: line 5 (0x10ca400d8)
Size: 512
Cache: line 6 (0x10ca400e8)
Size: 1024
Cache: line 7 (0x10ca400f8)
Size: 2048
ptr = 0x10ca74018
print: this is the test one
deallocate called for 0x10ca74018: unimplemented

Code

memory.h

#ifndef MEMORY_H
#define MEMORY_H

extern void  allocator_init(void);
extern void  deallocate(void *ptr);
extern void *allocate(unsigned int size);

#endif // MEMORY_H

main.c

#include <stdio.h>
#include <string.h>
#include "memory.h"

int main(void)
{
    allocator_init();
    char *ptr = (char *) allocate(4096); // custom function. that on `memory.c`
    printf("ptr = %p\n", ptr);
    strcpy(ptr, "this is the test one"); // segmentation fault here
    printf("print: %s\n", ptr);
    deallocate(ptr);
}

memory.c

#include "memory.h"
#include <assert.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <unistd.h>

#define REGION_SIZE sizeof(Region)

#define ALIGNMENT   8
#define ALIGN(size) (((size) + (ALIGNMENT - 1)) & ~(ALIGNMENT - 1))

#if defined(DEV_ZERO_MEMORY_MAPPING)
#define DEV_ZERO "/dev/zero"
#else
#define DEV_ZERO "./dev.zero"
#endif

enum { NUM_CACHES = 8 };

static int fd = -1;

typedef struct Region
{
    void *addr;
    int size;
    struct Region *next;
} Region;

typedef struct Slab
{
    void *addr;
    char *bitmap;
    int size;
    int slots;
    int cache_size;
    int bitmap_size;
    int currentSize;
    int bit;
    struct Slab *next;
} Slab;

typedef struct Cache
{
    int size;
    Slab *S;
} Cache;

typedef struct Memory
{
    Region *region;
    Cache cache[NUM_CACHES];
} Memory;

static Memory memory = { 0 };

static void dump_slab(FILE *fp, const char *tag, const Slab *slab)
{
    fprintf(fp, "Slab: %s (%p)\n", tag, slab);
    if (slab != 0)
    {
        fprintf(fp, "addr: %p, ", slab->addr);
        fprintf(fp, "bitmap: %p, ", (void *)slab->bitmap);
        fprintf(fp, "size: %6d, slots %3d, ", slab->size, slab->slots);
        /*
        int cache_size;
        int bitmap_size;
        int currentSize;
        int bit;
        struct Slab *next;
        */
    }
}

static void dump_cache(FILE *fp, const char *tag, const Cache *cache)
{
    fprintf(fp, "Cache: %s (%p)\n", tag, cache);
    if (cache != 0)
    {
        fprintf(fp, "Size: %d\n", cache->size);
        Slab *slab = cache->S;
        while (slab != 0)
        {
            dump_slab(fp, "", slab);
            slab = slab->next;
        }
    }
}

static void dump_region(FILE *fp, const char *tag, const Region *reg)
{
    fprintf(fp, "Region: %s (%p)\n", tag, reg);
    if (reg != 0)
    {
        fprintf(fp, "Address: %p, size: %6d, next = %p\n",
                reg->addr, reg->size, reg->next);
    }
}

static void dump_memory(FILE *fp, const char *tag, const Memory *mem)
{
    fprintf(fp, "Memory: %s (%p)\n", tag, mem);
    if (mem != 0)
    {
        Region *reg = mem->region;
        dump_region(fp, "Base", reg);
        while (reg->next != 0)
        {
            dump_region(fp, "Next", reg->next);
            reg = reg->next;
        }
        for (int i = 0; i < NUM_CACHES; i++)
        {
            char line[32];
            snprintf(line, sizeof(line), "line %d", i);
            dump_cache(fp, line, &memory.cache[i]);
        }
    }
}

void allocator_init(void)
{
    fd = open(DEV_ZERO, O_RDWR|O_CREAT, 0600);
    if (fd == -1)
    {
        perror("File open failed");
        exit(0);
    }

    if (fd == -1)
    {
        perror("File open failed");
        exit(0);
    }

    memory.region = NULL;

    for (int i = 0; i < NUM_CACHES; i++)
    {
        memory.cache[i].size = 16 << i;
        memory.cache[i].S = NULL;
    }
}

static void *allocate_region(unsigned int size)
{
    assert(fd != -1);

    Region *temp = memory.region;
    void *mapped_addr = mmap(NULL, size + REGION_SIZE, PROT_READ | PROT_WRITE,
                             MAP_PRIVATE, fd, 0);

    if (mapped_addr == MAP_FAILED)
    {
        perror("Mapping failed");
        exit(0);
    }
    printf("Mapping succeeded: %p\n", mapped_addr);

    Region *region = mapped_addr;
    printf("region = %p\n", region);
    region->size = size;
    printf("size = %d\n", region->size);
    region->addr = (char *)mapped_addr + REGION_SIZE;
    printf("allocated region: %p\n", region->addr);

    memory.region = region;
    region->next = temp;

    dump_memory(stderr, __func__, &memory);

    return region->addr;
}

void *allocate(unsigned int size)
{
    size = ALIGN(size);
    return allocate_region(size);
}

void deallocate(void *ptr)
{
    fprintf(stderr, "%s called for %p: unimplemented\n", __func__, ptr);
}

I strongly recommend making functions to dump complex structures along the dump_memory(), dump_region(), dump_cache(), dump_slab() functions shown; they're often very helpful, though they were actually something of a red herring in debugging this.

Community
  • 1
  • 1
Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
  • I have created new project, and use all your code. when I run, here is output : `Mapping succeed : 0x7f65b022000 region = 0x7f65b022000 Bus error (core dumped)` Is there any mistake on my machine ? thanks :) – Trần Kim Dự Apr 06 '14 at 01:58
  • "Bus error" often meaning an alignment issue. Figure out which line is causing this error. – M.M Apr 06 '14 at 02:01
  • I got that core dump (bus error) when the mapped file (`./dev.zero`) was empty — on my Mac. When I created a non-empty file (as mentioned in the answer), it worked correctly. It might help if you identify the platform you're working on. – Jonathan Leffler Apr 06 '14 at 02:03
  • I just compiled the code on an Ubuntu 13.10 VM and it crashed as described when `./dev.zero` was empty and worked when it was not empty (no recompilation needed). – Jonathan Leffler Apr 06 '14 at 02:10
  • @JonathanLeffler I have add `define DEV_ZERO_MEMORY_MAPPING` and code running well. I use Ubuntu 13.10, too I have compare your source code with mine, and do as your guideline, now, everything works fine. – Trần Kim Dự Apr 06 '14 at 06:02
  • Thanks very much for your help. Not only you help me on concrete problem, you have read my code line by line and figure out which point is bad (both in logical, and in convention, style ...) and give me many well information :D thanks thanks so much :D – Trần Kim Dự Apr 06 '14 at 06:04
  • I forgot to check that, but yes, the `/dev/zero` mapping seems to work OK on Ubuntu. Note that POSIX says: _The [`mmap()`](http://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html) function shall be supported for the following memory objects: * Regular files * [SHM] [Option Start] Shared memory objects [Option End] * [TYM] [Option Start] Typed memory objects [Option End] Support for any other type of file is unspecified._ Using `/dev/zero` is in the realm of behaviour that is 'unspecified by POSIX'. It works fine on Linux; it may not work anywhere else. – Jonathan Leffler Apr 06 '14 at 06:20