[RFC] Arithmetic on void pointers

[Pekka Enberg]

Hello,

It seems like clang doesn't really care much for "-Wno-pointer-arith"
for C++11 code:

[penberg@localhost tmp]$ cat pointer-arith.cpp
#include <cstdio>

int main()
{
void *p = reinterpret_cast<void*>(0xdeadbeef);

printf("%p\n", p + 8);
}
[penberg@localhost tmp]$ g++ -std=gnu++11 -Wall -Wno-pointer-arith
pointer-arith.cpp
[penberg@localhost tmp]$ clang++ -std=gnu++11 -Wall -Wno-pointer-arith
pointer-arith.cpp
pointer-arith.cpp:7:22: error: arithmetic on a pointer to void
printf("%p\n", p + 8);
~ ^
1 error generated.

For C code, the option works as expected.

What do we want to do in OSv code? Follow the more pedantic style
enforced by Clang or file a bug report and hope for the best?

Pekka

[Nadav Har'El]

On Fri, Oct 25, 2013 at 11:54 AM, Pekka Enberg <pen...@cloudius-systems.com> wrote:

Hello,

It seems like clang doesn't really care much for "-Wno-pointer-arith"
for C++11 code:

int main()
{
    void *p = reinterpret_cast<void*>(0xdeadbeef);

    printf("%p\n", p + 8);
}

[penberg@localhost tmp]$ clang++ -std=gnu++11 -Wall -Wno-pointer-arith
pointer-arith.cpp
pointer-arith.cpp:7:22: error: arithmetic on a pointer to void
    printf("%p\n", p + 8);
                   ~ ^
1 error generated.

Here I think clang is correct, and we should fix it. I was actually surprised the first time I saw void pointer arithmetic in OSv, but since the compiler didn't complain, I didn't either ;-)

In C (and C++) pointer arithmetic means the following: If you have

    type *p;

    int i;

    p+i;

Then the pointer is moved i*sizeof(p) bytes forward.

Since sizeof(void) is undefined, then so is arithmetic with void pointers...

If you want a pointer which you can move forward in *bytes* (char), you need a char*, not a void*.

So personally, I think we should switch to using char*, not void*, where we want to do byte-based pointer increments.

I think the way we use void* for this is wrong.

 

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

--

Nadav Har'El

n...@cloudius-systems.com

[Wink Saville]

Would it be better to use u8/uint8_t ... rather than char?

Is there a style guide?

-- Wink

[Nadav Har'El]

On Sat, Oct 26, 2013 at 3:58 AM, Wink Saville <wi...@saville.com> wrote:

Would it be better to use u8/uint8_t ... rather than char?

Is there a style guide?

The C and C++ books go to great lengths to explain the ridiculous concept that while sizeof(char)=1, it doesn't mean that char is always 1 byte (yeah... right... have you ever seen such a machine???). Rather what it means is that while "char" may be 2 bytes (say), C guarantees that any other size is a multiple of that size, e.g., "int" may be 2 "char"s, etc., but not one and a half. This will mean that any offset we ever use into a structure is a multiple of char size, so it makes sense to use char *. For example:

struct {

   int a;

   int b;

} s;

char *p = &s;  // p points into s.a.

p += sizeof(int); // now p points into s.b;

If we used u8*, not char*, the above code would not be correct.

However, looking at percpu.hh, we have

    T *addr(void* base = percpu_base) {
        size_t offset = reinterpret_cast<size_t>(&_var);
        return reinterpret_cast<T*>(base + offset);
    }

This will not not be correct with char*base if char isn't 1 byte, because it's not clear (to me) what the reinterpret_cast does. We don't need to use it - we need to use pointer arithemetic there too: something like

    constexpr char *elf_percpu_base = 0;

    T *addr(char* base = percpu_base) {
        size_t offset = reinterpret_cast<char *>(&_var) - elf_percpu_base;

        return reinterpret_cast<T*>(base + offset);
    }

[Wink Saville]

Personally I lean towards a style where size'd variables are used for things like hardware registers and such, so I'd probably define s as:

struct {

  s32 a;

  s32 b;

} s;

In any case, you might add a compile time check for size of char just to be safe.

Is there a style guide for the project?

-- Wink

[Avi Kivity]

On 10/25/2013 10:08 AM, Nadav Har'El wrote:

On Fri, Oct 25, 2013 at 11:54 AM, Pekka Enberg <pen...@cloudius-systems.com> wrote:

Hello,

It seems like clang doesn't really care much for "-Wno-pointer-arith"
for C++11 code:

int main()
{
    void *p = reinterpret_cast<void*>(0xdeadbeef);

    printf("%p\n", p + 8);
}
[penberg@localhost tmp]$ clang++ -std=gnu++11 -Wall -Wno-pointer-arith
pointer-arith.cpp
pointer-arith.cpp:7:22: error: arithmetic on a pointer to void
    printf("%p\n", p + 8);
                   ~ ^
1 error generated.

Here I think clang is correct, and we should fix it. I was actually surprised the first time I saw void pointer arithmetic in OSv, but since the compiler didn't complain, I didn't either ;-)

In C (and C++) pointer arithmetic means the following: If you have

    type *p;

    int i;

    p+i;

Then the pointer is moved i*sizeof(p) bytes forward.

Since sizeof(void) is undefined, then so is arithmetic with void pointers...

If you want a pointer which you can move forward in *bytes* (char), you need a char*, not a void*.

So personally, I think we should switch to using char*, not void*, where we want to do byte-based pointer increments.

I think the way we use void* for this is wrong.

I am really used to void* but agree that we should follow the standard.

Even more, we should avoid untyped pointers. But of course in some cases it is unavoidable.

[Avi Kivity]

On 10/26/2013 01:58 AM, Wink Saville wrote:

Would it be better to use u8/uint8_t ... rather than char?

Is there a style guide?

char* is more correct since C says that char* may alias to anything. This warns the compiler not to do strict aliasing analysis on the pointer in question.