2

In order to avoid memory fragmentation on very large datasets, I have implemented a doubly linked list that avoid calling malloc twice: one malloc for the data and another one for the prev and next nodes. Instead, it allocates the required space in one shot using alignof to get the offset of the struct containing the prev and next nodes.

The implementation is here but extracting the relevant part:

#include <stdlib.h>
#include <stdint.h>
#include <stdalign.h>

struct node
{
    struct node *prev;
    struct node *next;
};

typedef struct
{
    struct node *head;
    struct node *tail;
    size_t offset;
    size_t size;
} klist;

klist *klist_create(size_t size)
{
    klist *list = calloc(1, sizeof *list);

    if (list != NULL)
    {
        size_t align = alignof(struct node);

        // Round size up to nearest multiple of alignof(struct node)
        list->offset = (size + (align - 1)) / align * align;
    }
    return list;
}

#define klist_node(list, data) ((void *)((uintptr_t)(const void *)data + list->offset))
#define klist_data(list, node) ((void *)((uintptr_t)(const void *)node - list->offset))

void *klist_push_head(klist *list)
{
    void *data = calloc(1, list->offset + sizeof(struct node));

    if (data == NULL)
    {
        return NULL;
    }

    struct node *node = klist_node(list, data);

    if (list->head != NULL)
    {
        list->head->prev = node;
        node->next = list->head;
    }
    else
    {
        list->tail = node;
    }
    list->head = node;
    list->size++;
    return data;
}

void *klist_head(const klist *list)
{
    if (list->head != NULL)
    {
        return klist_data(list, list->head);
    }
    return NULL;
}

...

Then, in main:

struct data
{
    int key;
    char *value;
};

klist *list = klist_create(sizeof(struct data));
struct data *data = klist_push_head(list);

data->key = 1;
data->value = "one";

where data can be a pointer to any primitive or composite type.

The thing is that not being the typical packaged structure containing all the members involved:

struct node
{
    void *data;
    struct node *prev;
    struct node *next;
};

I am concerned about the effective type rule:

If a value is stored into an object having no declared type through an lvalue having a type that is not a character type, then the type of the lvalue becomes the effective type of the object for that access and for subsequent accesses that do not modify the stored value.

How does this rule affect the implementation of the list?

Is it legal/portable code?

David Ranieri
  • 39,972
  • 7
  • 52
  • 94
  • `(uintptr_t)(const void *)data + list->offset` assumes conversion of a pointer to an integer followed with integer math makes sense. A integer from a pointer is not specified to have this property - it is more like an unsequenced ID, than an ordered sequence of values. Instead consider using `char *`. – chux - Reinstate Monica Oct 12 '21 at 10:51
  • @chux-ReinstateMonica sorry but I dont get you, [a conversion to `void *` is required before converting a pointer to `uintptr_t` and vice versa](https://stackoverflow.com/questions/45352226/is-a-conversion-to-void-required-before-converting-a-pointer-to-uintptr-t). In this part I'm just taking the address of the packed `struct node`, then this address is casted to `void *` – David Ranieri Oct 12 '21 at 11:05
  • @chux-ReinstateMonica, furthermore, it seems than I am not allowed to use `char *` due to: _When a pointer to an object is converted to a pointer to a character type, the result points to the lowest addressed byte of the object. Successive increments of the result, up to the size of the object, yield pointers to the remaining bytes of the object._. Based on Lundin's answer to this related question: https://stackoverflow.com/questions/65897127/pack-two-objects-using-alignof , it is UB to inspect `data` through a `char*` and go beyond `sizeof(data)` bytes – David Ranieri Oct 12 '21 at 11:05
  • Do not do math on a integer from `(uintptr_t) some_void_star`. Nothing is C requires with `char buf[2];` that `(uintptr_t)(void *)&buf[1] - (uintptr_t)(void *)&buf[0] == 1`. – chux - Reinstate Monica Oct 12 '21 at 11:11
  • @chux-ReinstateMonica how is this? woow, I thought that arrays or an area reserved by `malloc` was required to be in contiguous areas, where can I read about this subject? – David Ranieri Oct 12 '21 at 11:20
  • 1
    Yes, arrays are in contiguous areas. The incorrect assumption is that the integer conversion of pointers to elements of the arrray is continuous. The addresses of the array elements are continuous. – chux - Reinstate Monica Oct 12 '21 at 11:29
  • 1
    AFAIK the standard only says that a valid void pointer can be converted to uintptr_t and back. What happens when you add something to uintptr_t before going back to void pointer isn't described. – Support Ukraine Oct 12 '21 at 11:38
  • @chux-ReinstateMonica I'm sure you're right, but I can't figure out why an integer type (`uintptr_t`) that is the result of subtracting/adding two contiguous and well aligned areas would cause some kind of problem, I'll investigate, thanks for the hint :) – David Ranieri Oct 12 '21 at 11:38
  • @4386427 oh I see ... thanks for the input., this seems one of those cases where "because the standard says so" must be applied, isn't it? – David Ranieri Oct 12 '21 at 11:41
  • Then, how can I pack them together and then access each section without breaking any rule? I don't want to use `max_align_t` because it can leave big gaps between the data and the node, I'm also aware of the `containerof` approach used in the linux kernel lists, any suggestion is welcomed ;) – David Ranieri Oct 12 '21 at 11:48
  • @DavidRanieri Which code lines is it that makes you worry about effective type? – Support Ukraine Oct 12 '21 at 12:01
  • @4386427 this one: `struct node *node = klist_node(list, data);` and `return klist_data(list, list->head);` – David Ranieri Oct 12 '21 at 12:05
  • Maybe I'm misunderstanding this but that line doesn't store a value into an object having no declared type... does it? – Support Ukraine Oct 12 '21 at 12:07
  • I'm not sure, thats why I am asking ;) is `struct node` a declared type? I mean, I'm reserving space with `sizeof(data) + required bytes for the alignment + sizeof(struct node)` – David Ranieri Oct 12 '21 at 12:16
  • As far as I can see it doesn't write to the `calloc`ed memory. It calculates a new void pointer. So I don't see how the effective type rule is relevant for that code. That said I would probably use a char pointer for the calculation. – Support Ukraine Oct 12 '21 at 12:23
  • Thanks @4386427, _I would probably use a char pointer for the calculation._ but then it will break this other rule: _it is UB to inspect data through a `char*` and go beyond `sizeof(data)` bytes_ isn't it? (I can not found a quotation to the rule, is just what @Lundin answered here: https://stackoverflow.com/a/65900928/1606345) – David Ranieri Oct 12 '21 at 12:25
  • 1
    @DavidRanieri To me the standard isn't 100% clear so I can't provide quotes. I may be mistaken but my understanding is: 1) Inspecting data through a `char*` is only UB on systems having trap representations. If that's a concern you can use `unsigned char*` as `unsigned char` can't have trap representations. That said... as far as I can see, your code doesn't inspect data so it's not a concern. 2) To me the concern is whether the pointer arithmetic is valid, i.e. a) casting to (unsigned) char pointer b) doing pointer arithmetic by adding/subtracting an offset and ...... cont.... – Support Ukraine Oct 13 '21 at 07:57
  • 1
    cont... c) casting back to void pointer. Is that a valid operation? I think it is because the resulting pointer will always be within the block obtained in a single `calloc`. But just to repeat... I'm not 100% sure as I can't give a clear quote from the standard. – Support Ukraine Oct 13 '21 at 07:59

1 Answers1

1

I do not see clearly all aspects of OP's approach short-comings, yet certain parts like addition of integers pointers via (uintptr_t)(void*) is not specified to work to form the desired final pointer.


An alternative is to use a flexible member array which will handle padding issues too.

Something like the below.

// Error checking omitted for brevity.   

struct node {
  struct node *prev;
  struct node *next;
  max_align_t data[]; // FMA member at worst case alignment.
};

typedef struct {
  struct node *head;
  struct node *tail;
  size_t data_size;
  size_t size;
} klist;

klist* klist_create(size_t data_size) {
  klist *list = calloc(1, sizeof *list);
  list->data_size = data_size;
  return list;
}

struct node* klist_push_head(klist *list) {
  struct node *nd = calloc(1, sizeof *nd + list->data_size);
  if (list->head) {
    list->head->prev = nd;
    nd->next = list->head;
  } else {
    list->tail = nd;
  }
  list->head = nd;
  list->size++;
  return nd;
}

#define klist_data(/* struct node* */ nd) ((void *)&((nd)->data))
chux - Reinstate Monica
  • 143,097
  • 13
  • 135
  • 256
  • Thank you chux, I have used your aproach (`max_align_t`): https://github.com/davranfor/c/blob/master/klist/klist.c, there is only one thing that I don't like, since I call `malloc` in one shot and `data` is placed after the `struct node`, now `free(data);` must be `free((char *)data - offsetof(struct node, data));`, it seems prone to errors to me but at least the implementation haves well defined behaviour :) – David Ranieri Oct 12 '21 at 18:29
  • 1
    @DavidRanieri As the allocation was done with a helper function `klist_push_head()`, the free-ing should be done via some `klist_free(klist *list, struct node*)` helper too - not a raw `free()`. Thus any error prone-ness is managed. Look forward to reviewing code on [code review](https://codereview.stackexchange.com/). – chux - Reinstate Monica Oct 12 '21 at 18:40
  • @DavidRanieri Concerning "In order to avoid memory fragmentation on very large datasets", I do not see this approach as _certainly_ improving things. Profiling would help back-up your view. I'd campaign for first coding a clear, no tricks code (2 allocations) that works and then look for verified improvements in speed and memory usage. – chux - Reinstate Monica Oct 12 '21 at 18:48
  • yes, is done by a helper: https://github.com/davranfor/c/blob/master/klist/klist.c#L261 but I mean is easy to forget that the data must be freed with `kfree(data)` instead of `free(data);`. Thanks for the link ;) – David Ranieri Oct 12 '21 at 18:48
  • Well, having a large dataset ,say 1 million records requires one million calls to `malloc` instead of 2 millions, but yeah, I need to profile. – David Ranieri Oct 12 '21 at 18:50
  • @DavidRanieri `free(NULL)` is OK. `klist_free(NULL)` is not. Better to add `NULL` check within. Allowing `klist_free(NULL)` simplifies errant code handlers. – chux - Reinstate Monica Oct 12 '21 at 18:51
  • ooops, you are right!!!! O my god I need a break :) Thanks for your help chux – David Ranieri Oct 12 '21 at 18:52