0

I am running cppcheck (c++11) against a library that contains many casts similar to the below:

// Allocates various buffers
int*   i_buffer = (int*)   calloc (500, sizeof (int));
float* f_buffer = (float*) i_buffer;

For these casts, I see the following message:

"portability","invalidPointerCast","Casting between integer* and float* which have an incompatible binary data representation."

What is the correct way to perform the type of cast shown above ? What is the potential consequence of casting the pointer the way it is shown above ?

didjek
  • 393
  • 5
  • 16
  • 1
    use c++ casts. 'reinterpret_cast' in this case – BЈовић Jan 29 '20 at 16:18
  • 2
    The messages from compiler seems perfectly legit. Could you give some context on why one would want to do this? This seems pretty bad code to me (but maybe there is some reason to write this?) – kebs Jan 29 '20 at 16:20
  • 1
    @BЈовић it is a common misunderstanding (not excluding myself) that `reinterpret_cast` lets you cast between arbitrary types, but in fact there is only a rather limited list of cases where `reinterpret_cast` won't send you straight into undefined behaviour land – 463035818_is_not_an_ai Jan 29 '20 at 16:20
  • Don't use `calloc` in C++ to begin with. Fix that first. – Jesper Juhl Jan 29 '20 at 16:20
  • I'm afraid its somebody else's quite elderly code who is no longer available for questioning. – didjek Jan 29 '20 at 16:21
  • @didjek Can you read and understand what the code is trying to do then? – Yakk - Adam Nevraumont Jan 29 '20 at 16:22
  • 6
    There isn't. First `(int*) calloc (500, sizeof (int));` will not generate an array on integers. It just allocates memory. Using it will be undefined behavior. If you are using `float* f_buffer = (float*) i_buffer;` so that you can run through those non-existing integers as if they were floats, that's also illegal as it violates the strict aliasing rules. No amount of casting will fix any of that. This is C code (besides the calloc cast) and should not be used in C++ code. – NathanOliver Jan 29 '20 at 16:22
  • @NathanOliver I don't think using memory allocated by `calloc` to hold `int`s is undefined behaviour. – user253751 Jan 29 '20 at 16:29
  • @user253751 In C++, it is. Per [this](https://timsong-cpp.github.io/cppwp/intro.object#1.sentence-2) `calloc` does not create an object. If you try to use an object that doesn't exist, you have undefined behavior. – NathanOliver Jan 29 '20 at 16:30
  • @NathanOliver Per [this](https://timsong-cpp.github.io/cppwp/basic.life#1) the lifetime of an object starts once storage is obtained and initialization (if any) is complete. `int` objects have no initialization, so their lifetime begins as soon as storage is obtained. – user253751 Jan 29 '20 at 16:40
  • @user253751 That's true. But per my link, you don't even have an object. The lifetime of an object that doesn't exist can't begin. You need to have a valid objectc first, and `calloc` does not give you a valid object, just raw memory. You need to placement new over it (but array new is broken so don't use it) is order to have an object and start its life. – NathanOliver Jan 29 '20 at 16:42
  • @formerlyknownas_463035818 Any link? I know about it, but can not find any info. My comment just explains how to "fix" the warning from cppcheck, not dealing with million other problems related to that piece of code. – BЈовић Jan 29 '20 at 16:44
  • @BЈовић https://en.cppreference.com/w/cpp/language/reinterpret_cast – 463035818_is_not_an_ai Jan 29 '20 at 16:45
  • @NathanOliver What exactly is the difference between "an object existing" and "an object's lifetime having begun"? It seems to me that you are saying that no object can ever be created because at the time of its creation, it does not exist yet. – user253751 Jan 29 '20 at 16:49
  • 2
    @user253751 existing is a compile time notion, lifetime is a run time notion. At compile time, you have create an object via a declaration, new expression, changing the active union member or creating a temporary. Then after that and at run time the lifetime starts after the storage has been acquired and initialization is complete. If you don't have an object at compile time (like the OP's example) then there is no object at run time to become alive. – NathanOliver Jan 29 '20 at 16:54
  • @NathanOliver I'll be sure to tell Ulrich Drepper that it's illegal to access the strings pointed to by argv in C++. Maybe he'll ban C++ programs from using the C standard library. – user253751 Jan 29 '20 at 17:02
  • @user253751 `argv` is defined as a parameter of `main`. It's a valid object. Sorry if you don't beleive me, but the standard is quite clear on how the `alloc` family behaves. – NathanOliver Jan 29 '20 at 17:06
  • @NathanOliver `argv` is, but what about the thing `argv` points to? If you re-read my previous comment you will see I was not talking about `argv` itself. – user253751 Jan 29 '20 at 17:10
  • @NathanOliver are you saying the example [here](https://en.cppreference.com/w/cpp/memory/c/calloc) has undefined behavior? – Kevin Jan 29 '20 at 17:21
  • @Kevin Yes. I've not seen a compiler where the expected behavior doesn't happen, but per the abstract machine it is UB. There is actually a paper asking for this corner of the language to be fixed since so many people think it is okay as-is: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0593r2.html – NathanOliver Jan 29 '20 at 17:47

3 Answers3

2

How to cast int pointer to float pointer

Using reinterpret cast.

But don't try to do that, because the reinterpreted pointer won't be useful.

What is the correct way to perform the type of cast shown above ?

The shown cast is already well defined by itself; it's just not useful to do such cast.

If you need to allocate an array of floats, you can use the following instead:

std::vector<float>(500);
eerorika
  • 232,697
  • 12
  • 197
  • 326
  • reinterpret_cast passes the compiler alright (as does the existing code). For cppcheck, I get the same warning message, however. – didjek Jan 29 '20 at 16:32
  • @didjek Yes. It passes the compiler because the program is well-formed. But cppcheck inspects the program further and recognises that the cast isn't useful. – eerorika Jan 29 '20 at 16:33
1

Strictly speaking the behaviour of your code is undefined due to a strict aliasing violation.

In practice, if int and float are the same size (they are on many desktop platforms although there is some push to move int to 64 bit), then the code will run without error. Although to re-iterate, from the perspective of standard C++ there is absolutely no guarantee of this.

But you should still fix it. Allocating the float array directly is the sensible thing to do:

float* f_buffer = (float*) calloc (500, sizeof (float));
Bathsheba
  • 231,907
  • 34
  • 361
  • 483
  • 1
    Why keep `calloc`? This still doesn't actually generate an array an using it as if it did would be UB. – NathanOliver Jan 29 '20 at 16:31
  • 1
    @NathanOliver: Because I don't want to burden the OP with having to hunt down all the `free` calls. – Bathsheba Jan 29 '20 at 16:31
  • Actually, several solutions proposed by other posters also worked, in the sense that they meant that the cppcheck message went away! I chose this one as the answer because it is closer to the original code in spirit. I also dislike hunting for free calls. No doubt it is debatable as to whether this is really the correct c++ way to do it. The "new" syntax would seem to be the most correct from a pure c++ point of view. – didjek Jan 29 '20 at 16:39
  • @didjek: If you can go down the C++ route then `std::vector(500);` would be hard to beat, although perhaps then you'd need to change your code structure in order to keep that vector in scope, plus you'll need to find all the `free` calls. Not all of us (me included) have the luxury of having a codebase consisting of perfect C++17 code - mine is a mash of things being slowly fixed up to C++11 standard. Which is why I like folk to learn C99 first so they have a sense of evolution. – Bathsheba Jan 29 '20 at 16:46
  • All: @NathanOliver does raise an important point here - technically the behaviour is still undefined in C++ (`calloc` is not a way to create an *object*) although the fashion is to regard that as a defect in the C++ standard. Commercially if I had a C++ compiler that puked on it, then I'd switch compiler vendor. – Bathsheba Jan 30 '20 at 11:36
0

It's also legitim to use an old style C array by new (which is C++ standard):

float* f_buffer = new float[500];

Be aware to adapt the releasing of that memory to:

delete [] f_buffer;

Type casts should always be avoided to keep a clean code and to allow the compiler to verify correctness of your code.

Nick
  • 472
  • 3
  • 18
  • The `new` syntax is C++, not C – Anonymous Anonymous Jan 29 '20 at 16:30
  • @AnonymousAnonymous: Indeed it is, and the language tag is C++. But this answer should point out that you must not use `free` to release memory allocated with `new`: so moving away from `calloc` might introduce bugs, unless you hunt all the corresponding `free` calls down like rabbits. – Bathsheba Jan 29 '20 at 16:30
  • @Bathsheba Yeah, I know - just wanted to point out, that this is no `old style C array`. The phrasing could confuse people – Anonymous Anonymous Jan 29 '20 at 16:33
  • Hi, thanks for your proposal too, it also works. Seem my comment relating to Bathsheba's proposal. – didjek Jan 29 '20 at 16:43
  • You're right, but by "old style" I mean the C-style array, not the new syntax. For the releasing by delete I added a note. – Nick Jan 30 '20 at 10:43