1

I am trying to implement the memmove() function for a certain quiz, so I dynamically allocated a block of memory with the ptr buffer as temp holder in the swap operation and the swap is done in one loop. When I submit this code it gives me one wrong answer out of 3 test cases. When I separate the swap in two for loops, all cases succeed. So what is the difference ?

  • swap in the same loop (One Case Wrong):

    uint8_t *my_memmove(uint8_t *src, uint8_t *dst, size_t length) {
    uint8_t *buffer;
    buffer = (uint8_t*)reserve_words(length);
    for (uint8_t i = 0; i < length; i++) {
        *(buffer+i) = *(src+i);
        *(dst+i) = *(buffer+i);   
    }
    return dst;
    free_words((uint32_t*)buffer);
}

  • swap in two loops (All Cases Succeed):

    uint8_t *my_memmove(uint8_t *src, uint8_t *dst, size_t length) {
    uint8_t *buffer;
    buffer = (uint8_t*)reserve_words(length);
    for (uint8_t i = 0; i < length; i++) {
        *(buffer+i) = *(src+i);
    }

    for (uint8_t i = 0; i < length; i++) {
        *(dst+i) = *(buffer+i);   
    }

    return dst;
    free_words((uint32_t*)buffer);
}

  • reserve_words function used (user_defined) :

    int32_t *reserve_words(size_t length) {
    int32_t *ptr;
    ptr = (int32_t *)malloc(length * sizeof(int32_t));
    return ptr;
}
klutt
  • 30,332
  • 17
  • 55
  • 95
Mohamed Wehba
  • 31
  • 4
  • Hi, perhaps the memory location is overwritten in the first case before the swap? – IronMan Oct 05 '20 at 22:12
  • 3
    Consider what happens if `src` and `dst` have overlapping regions. – kaylum Oct 05 '20 at 22:12
  • 2
    Note that you leak memory because your `free()` is after your `return`. You also allocate four times as much memory as necessary because you multiply the length by `sizeof(int32_t)`. You should check that the memory allocation succeeds before using the allocated memory. – Jonathan Leffler Oct 05 '20 at 22:13
  • 1
    Why do you have the `reserve_words` function instead of a simple `malloc` call? – klutt Oct 05 '20 at 22:13
  • You can't do it in the same loop, that has the same problem as trying the copy directly from source to destination. – Barmar Oct 05 '20 at 22:22
  • Note that the C11 standard [§7.24.2.2 The `memmove` function](http://port70.net/~nsz/c/c11/n1570.html#7.24.2.2) says: "Copying takes place _as if_ the n characters from the object pointed to by `s2` are first copied into a temporary array of `n` characters that does not overlap the objects pointed to by `s1` and `s2`, and then the `n` characters from the temporary array are copied into the object pointed to by `s1`." Only simple-minded implementations do it by an actual double copy. You can copy head-to-tail if the areas don't overlap or `s1` < `s2`, and tail-to-head otherwise. – Jonathan Leffler Oct 05 '20 at 22:22
  • 1
    @JonathanLeffler There's no portable way to tell if they overlap, because you can't compare pointers between different arrays. The real implementation of `memmove()` isn't limited like this. – Barmar Oct 05 '20 at 22:23
  • Note `reserve_words()` allocates excess memory. Suggest `ptr = malloc((length+3)/4 * sizeof *ptr);` or the like. – chux - Reinstate Monica Oct 05 '20 at 22:26
  • 1
    @Barmar — yes, and that's why the standard goes through the shenanigans with the spare memory, but a library for a particular implementation can know whether you can portably compare pointers despite the restrictions the standard has, and can therefore make that determination. That's why it is a standard library function — the standard library is tailored to a particular implementation. On most machines, you can make that comparison — possibly by converting addresses to `uintptr_t` values — but it is not guaranteed by the standard to be portable (but the implementation knows that it is safe). – Jonathan Leffler Oct 05 '20 at 22:26
  • 1
    @JonathanLeffler Right, but since he's trying to implement his own `memmove`, and presumably it should be portable, none of that helps him. – Barmar Oct 05 '20 at 22:28
  • @Barmar — It depends on what the rules of engagement are. There's nothing in the question that says it must be portable. However, the relevant points have now been made. Simple minded but portable code will work somewhat as shown; efficient code will not. – Jonathan Leffler Oct 05 '20 at 22:29
  • @Barmar: equality checking is well defined between any two objects of the same type, and that's sufficient to check for overlap (if you do it in a loop). Of course, the loop is inefficient, but it's less work than an unnecessary copy, and there's an off-chance that the compiler will figure it out. – rici Oct 06 '20 at 04:05
  • @rici I realized that, but I didn't think anyone could be seriously suggesting equality checking between all the array elements. – Barmar Oct 06 '20 at 04:28
  • @Barmar: If you're prepared to malloc an temporary buffer and copy to it, then the equality checking is entirely reasonable. (You only have to know if the last byte in src is somewhere in dst, so it's O(n). If the overlap goes the other way, it's still safe to do the copy left to right.) – rici Oct 06 '20 at 04:30

3 Answers3

1

The problem here is that memmove is guaranteed to work with overlapping memory blocks. If you want to see a case where your first version would fail, try this:

char str[20]="Hello, World!";
my_memmove(&str[0], &str[1], 13);
puts(str);

The output is:

HHHHHHHHHHHHHH

It's easy to realize why. Every iteration you will write to the byte you're going to read from next iteration.

So your first version of my_memmove should really be called my_memcpy instead, because memcpy is not guaranteed to work with overlapping memory areas, which memmove is.

klutt
  • 30,332
  • 17
  • 55
  • 95
1
  • The first version copies from the source to the destination as it copies to the temporary array: if the source and destination arrays overlap and src < dst you will overwrite parts of the source array before copying them. This is exactly where the difficulty lies when implementing memmove()
  • The second version allocated a temporary array, copies from the source to it, then copies from the temporary array to destination: no problem, except you free the temporary array after the return statement, so there is a memory leak, and you are probably not expected to allocate memory for this assignment.
  • There is a bug in all copying loops: i should be defined as a size_t, not a uint8_t. As coded, the function will fail for blocks longer than 255 bytes.
  • there is no need to allocate length words to save length bytes.

The trick for memmove() is to test whether dst points inside the source array and copy from the end of the source array to the beginning if such is the case. There is no fully portable way to do this, but for most current systems with a flat address space, the test if (dst > src && dst < src + length) works fine.

Here is a simplistic implementation (note that your prototype has the src and dst pointers transposed from the standard memmove() function):

uint8_t *my_memmove(const uint8_t *src, uint8_t *dst, size_t length) {
    if (dst > src && dst < src + length) {
        for (size_t i = length; i-- > 0;)
            dst[i] = src[i];
    } else {
        for (size_t i = 0; i < length; i++)
            dst[i] = src[i];
    }
    return dst;
}
chqrlie
  • 131,814
  • 10
  • 121
  • 189
  • in memmove use of `const` is disputable – 0___________ Oct 05 '20 at 22:44
  • 1
    @P__J__: Why is that? `src` is not used to write to the source array, hence a `const` qualified pointer can be passed to `memmove` and the corresponding argument must be `const`-qualified too. The fact that the source and destination arrays can overlap means `src` an `dst` cannot be `restrict` qualified, which is a different matter. – chqrlie Oct 05 '20 at 23:47
  • As I wrote disputable. IMO if there is any potential path of execution which writes this oblect it should not be declared as `const`. `restrict` is a different issue – 0___________ Oct 06 '20 at 00:14
  • @P__J__: if `memmove` writes the object though the `src` pointer, I agree. If memory modified through the `dst` pointer happens to overlap memory read through the `src` pointer, it is not a reason to `const` qualify `*src`, but it is the very reason to not `restrict` qualify either pointer, contrary to `memcpy` where the requirement is that these areas be disjoint. The specification for `restrict` is not perfect but applies precisely to the case in question. – chqrlie Oct 06 '20 at 08:30
1
  1. Memory leak
    return dst;
    free_words((uint32_t*)buffer);

You will never call free_words as it will return earlier.

  1. When you copy in one loop and memory locations overlap your algorithm will overwrite the data and the function will not work.

  2. This buffer completelly useless and it is an example of bad programming

  3. function should get and return void * pointers.

  4. You do not have to cast the pointers if one side of the assignment is (void *)

void *mymemmove(void *dest, void *src, size_t size)
{
    void *saveddest = dest;
    unsigned char *ucdest = dest, *ucsrc = src;
    if(dest && src && size)
    {
        if(dest < src)
        {
            while(size--)
            {
                *ucdest++ = *ucsrc++;
            }
        }
        else if(src < dest) //this if to avoid copy if dest == src (not needed)
        {
            ucdest += size - 1; ucsrc += size - 1;
            while(size--)
            {
                *ucdest-- = *ucsrc--;
            }
        }
    }
    return saveddest;
}
0___________
  • 60,014
  • 4
  • 34
  • 74