warning: integer overflow in expression 1

[WebDrake]

Hello all,

I've got a warning coming up in some code that I don't understand (the subject line should tell you what it is).

The lines in question are implementations of the following macros that I've written as a very simple mechanism for code to exit gracefully in the event of a memory allocation error. Here are the macros:

#define MALLOC(vector,vectorsize,vectortype,errmessage) { vectortype *temp = (vectortype *)malloc((unsigned) (vectorsize)*sizeof(vectortype));	if(temp!=NULL) vector = temp;	else error_output_quit(errmessage); }

#define CALLOC(vector,vectorsize,vectortype,errmessage) { vectortype *temp = (vectortype *)calloc((unsigned) (vectorsize), (unsigned) sizeof(vectortype));	if(temp!=NULL) vector = temp;	else error_output_quit(errmessage); }

#define REALLOC(vector,oldvector,vectorsize,vectortype,errmessage) { vectortype *temp = (vectortype *)realloc(oldvector,(unsigned) (vectorsize)*sizeof(vectortype));	if(temp!=NULL) vector = temp;	else error_output_quit(errmessage); }

So, for example, in the program one would type,

MALLOC(x,100,double,"If this error message is produced, malloc'ing x failed");

These macros appear to work fine and it's only on one occasion when the warning message is generated: when the "vectorsize" entry is a const int set equal to 1000000.

I really, really would like this int to be 1000000. So, what's the issue? Should I set it to long int? Or is there something else I should be aware of?

Many thanks,

-- Joe


P.S. if the macros are useful, feel free to use. If they're crap, advice on what would be good is appreciated.

 

[WebDrake]

I really, really would like this int to be 1000000. So, what's the issue? Should I set it to long int? Or is there something else I should be aware of?

Trying out a few possibilities, it appears it's the const int bit the compiler objects to.

Using just int, or a #define statement, doesn't produce the problems. However, one can preserve the const int if one makes it instead, unsigned const int.

It appears the (unsigned) conversion in the macros is what causes the trouble.

 

[DaveSinkula]

Why use macros?

If you have a good reason, wrapping in [blue][tt]do ... while(0)[/tt][/blue] could be better.
If C only, why the casts?
If C++ only, why malloc, etc.?
If C/C++, yeech.

Why cast the vectorsize to [tt]unsigned[/tt]? The [tt]size_t[/tt] it is multiplied by will generally promote the vectorsize unsigned integral type that [tt]sizeof[/tt] returns.

What is the range of [tt]int[/tt] and [tt]unsigned[/tt] on the platform the produces the diagnostic?

 

[WebDrake]

I'm using macros simply to get a nice, elegant notation in the course of programming, whose meaning can be revised easily without going through all the code. I would in many ways much prefer writing them as functions instead of macros, but I wasn't sure that I could pass something like vectortype to a function.

If you have a good reason, wrapping in do ... while(0) could be better.

Can you clarify? I'm not at all a development programmer and I'm trying to put in place some simple things to give alerts when memory allocation fails. As a first approximation, I'm making the program simply exit with an error message, but I would like to expand it to something which gives the program a few chances before deciding that a given allocation just isn't going to happen.

If C only, why the casts?
If C++ only, why malloc, etc.?
If C/C++, yeech.

I want to clarify what the issues are here---I think we've talked about this before. I write exclusively in C at the moment, but would like to be able to use the modules I've created now in a C++ application at some point without rewriting them.

A side benefit of the macro is that one of the nastier possibilities, having a cast with different type from the allocation, e.g. (double *)malloc(10*sizeof(int)) , isn't going to happen. Also, since the macro assumes the *temp pointer to be of the same type as the "vector" passed to it, warnings can still come up if there is a clash.

Why cast the vectorsize to unsigned? The size_t it is multiplied by will generally promote the vectorsize unsigned integral type that sizeof returns.

Casting the size to unsigned is something I was taught to do and it made sense to me since one can't realistically assign a negative amount of memory. I don't completely understand your objection so if you can explain at length I'd be very grateful.

One thing I do remember---and I think it was in a conversation with you---is it being pointed out that using casts like this destroys the ability of the compiler to help flush out errors. I can certainly appreciate the possibility here. Better, perhaps, to remove the (unsigned) cast: if the program falls over because of a negative value being used, it's a much bigger clue than if it falls over because its absolute value was wrong.

Thanks very much for the useful thoughts.

 

[DaveSinkula]

I'm using macros simply to get a nice, elegant notation in the course of programming, whose meaning can be revised easily without going through all the code. I would in many ways much prefer writing them as functions instead of macros, but I wasn't sure that I could pass something like vectortype to a function.

Consider how [tt]malloc[/tt] et al do it for a bunch of types in the first place: [tt]void*[/tt]. But as a one-liner, probably not with a function. Hm.

Do what you like. I find debugging macros 10 times as difficult as debugging code.

If you have a good reason, wrapping in do ... while(0) could be better.

Can you clarify? I'm not at all a development programmer and I'm trying to put in place some simple things to give alerts when memory allocation fails. As a first approximation, I'm making the program simply exit with an error message, but I would like to expand it to something which gives the program a few chances before deciding that a given allocation just isn't going to happen.

http://c-faq.com/cpp/multistmt.html

I want to clarify what the issues are here---I think we've talked about this before. I write exclusively in C at the moment, but would like to be able to use the modules I've created now in a C++ application at some point without rewriting them.

Keep them as C. Compile them as C. Modify the headers to coexist with C++.

32.4 Mixing C and C++

A side benefit of the macro is that one of the nastier possibilities, having a cast with different type from the allocation, e.g. (double *)malloc(10*sizeof(int)) , isn't going to happen. Also, since the macro assumes the *temp pointer to be of the same type as the "vector" passed to it, warnings can still come up if there is a clash.

Use the form:

x = malloc(N * sizeof *x);

Casting [tt]malloc[/tt]

Casting the size to unsigned is something I was taught to do and it made sense to me since one can't realistically assign a negative amount of memory. I don't completely understand your objection so if you can explain at length I'd be very grateful.

The sign is already lost in the promotion to [tt]size_t[/tt]. So it only buys you potential error hiding.

 

[WebDrake]

Have I told you before that you're a real star? :-D

I agree in general about macros and debugging, which is why I hesitated for so long before doing this. But these ones at least are relatively simple and, once done, they're done, and one can get a lot of use out of them.

I guess that one could create a function along the following lines:

void *MALLOC(int vectorsize, int sizeofvectype, char *errmessage) {
    void *temp = malloc(vectorsize*sizeofvectype);
    if(temp!=NULL)
        return temp;
    else
        error_output_quit(errmessage);
}

which one could call as follows:

{
    /* Deep in some dark and dismal function ... */

    int *x;

    x = MALLOC(100,sizeof(int),"x was not malloc'd properly!!");
}

I don't know to what extent this might introduce a speed hit. In my own code I'd rather the compilation take longer and the program be quicker, but I guess in other development situations the speed tradeoff might be worthwhile. I'll test out the above and see how it goes.

If there's any interest, I'm happy to keep the boards up to date both with the macro and function versions of what I'm aiming for here.

 

[DaveSinkula]

That's about what I was coming up with. Mine was like this.

#include <stdio.h>
#include <stdlib.h>

void error_output_quit(const char *errmessage)
{
   puts(errmessage);
   exit(EXIT_FAILURE);
}

void *mymalloc(size_t elements, size_t size, const char *msg)
{
   void *dynmem = malloc(elements * size);
   if ( dynmem == NULL )
   {
      error_output_quit(msg);
   }
   return dynmem;
}

void *mycalloc(size_t elements, size_t size, const char *msg)
{
   void *dynmem = calloc(elements, size);
   if ( dynmem == NULL )
   {
      error_output_quit(msg);
   }
   return dynmem;
}

void *myrealloc(void *original, size_t elements, size_t size, const char *msg)
{
   void *dynmem = realloc(original, elements * size);
   if ( dynmem == NULL )
   {
      free(original);
      error_output_quit(msg);
   }
   return dynmem;
}

With usage something like this.

int main()
{
   int i, count = 100;
   int *x, *y;
   [blue]x = mymalloc(count, sizeof *x, "x was not malloc'd properly!!");[/blue]
   for ( i = 0; i < count; ++i )
   {
      x[i] = i;
      printf("x[%d] = %d\n", i, x[i]);
   }
   count = 10;
   [blue]x = myrealloc(x, count, sizeof *x, "x was not realloc'd properly!!");[/blue]
   for ( i = 0; i < count; ++i )
   {
      x[i] = i * 10;
      printf("x[%d] = %d\n", i, x[i]);
   }
   count = 25;
   [blue]y = mycalloc(count, sizeof *y, "y was not calloc'd properly!!");[/blue]
   for ( i = 0; i < count; ++i )
   {
      y[i] = i * 2;
      printf("y[%d] = %d\n", i, y[i]);
   }
   free(x);
   free(y);
   return 0;
}

I don't know to what extent this might introduce a speed hit.

The macro being simple text substitution, virtually the same code is there. (Take a look at the preprocessor output to see what you are actually presenting to the compiler.) So outside of the overhead of a function call and return, I don't see much difference. By the way, given this (line wrap due to the site's formatting):

#include <stdio.h>
#include <stdlib.h>

#define MALLOC(vector,vectorsize,vectortype,errmessage) { vectortype *temp = (vectortype *)malloc((unsigned) (vectorsize)*sizeof(vectortype));    if(temp!=NULL) vector = temp;    else error_output_quit(errmessage); }

void error_output_quit(const char *errmessage)
{
   puts(errmessage);
   exit(EXIT_FAILURE);
}

int main()
{
   int *x;
   MALLOC(x, 100, int, "x was not malloc'd properly!!");
   return 0;
}

Here is a prepro output I get.

void error_output_quit(const char *errmessage)
{
puts(errmessage);
exit(1);
}

int main()
{
int *x;
{ int *temp = (int *)malloc((unsigned) (100)*sizeof(int)); if(temp!=0) x = temp; else error_output_quit("x was not malloc'd properly!!"); };
return 0;
}

In a system that actually supports dynamic memory, I wouldn't look at function call overhead as being a primary bottleneck (tiny embedded systems being a place where [tt]malloc[/tt] may not exist and function call overhead can be an issue).


Another possible addition might be to use macros in conjunction with these functions.

[blue]#define MALLOC(var, elem, msg)  var = mymalloc(elem, sizeof *var, #var msg)
#define CALLOC(var, elem, msg)  var = mycalloc(elem, sizeof *var, #var msg)
#define REALLOC(var, elem, msg) var = myrealloc(var, elem, sizeof *var, #var msg)[/blue]

int main()
{
   int i, count = 100;
   int *x, *y;
   [blue]MALLOC(x, count, " was not malloc'd properly!!");[/blue]
   /* ... */
   count = 10;
   [blue]REALLOC(x, count, " was not realloc'd properly!!");[/blue]
   /* ... */
   count = 25;
   [blue]CALLOC(y, count, " was not calloc'd properly!!");[/blue]
   /* ... */
   free(x);
   free(y);
   return 0;
}

And yet another variation.

#include <stdio.h>
#include <stdlib.h>

void error_output_quit(const char *var[blue], const char *function[/blue])
{
   printf("%s -- %s failure\n", var, function);
   exit(EXIT_FAILURE);
}

void *mymalloc(size_t elements, size_t size, const char *name)
{
   void *dynmem = malloc(elements * size);
   if ( dynmem == NULL )
   {
      error_output_quit(name[blue], "malloc"[/blue]);
   }
   return dynmem;
}

void *mycalloc(size_t elements, size_t size, const char *name)
{
   void *dynmem = calloc(elements, size);
   if ( dynmem == NULL )
   {
      error_output_quit(name[blue], "calloc"[/blue]);
   }
   return dynmem;
}

void *myrealloc(void *original, size_t elements, size_t size, const char *name)
{
   void *dynmem = realloc(original, elements * size);
   if ( dynmem == NULL )
   {
      free(original);
      error_output_quit(name[blue], "realloc"[/blue]);
   }
   return dynmem;
}

#define MALLOC(var, elem)  var = mymalloc(elem, sizeof *var, [blue]#var[/blue])
#define CALLOC(var, elem)  var = mycalloc(elem, sizeof *var, [blue]#var[/blue])
#define REALLOC(var, elem) var = myrealloc(var, elem, sizeof *var, [blue]#var[/blue])

int main()
{
   int count = 100;
   int *x, *y;
   [blue]MALLOC(x, count);[/blue]
   /* ... */
   count = 10;
   [blue]REALLOC(x, count);[/blue]
   /* ... */
   count = 25;
   [blue]CALLOC(y, count);[/blue]
   /* ... */
   free(x);
   free(y);
   return 0;
}

[purple]/* preprocessor output of main
int main()
{
int count = 100;
int *x, *y;
x = mymalloc(count, sizeof *x, "x");

count = 10;
x = myrealloc(x, count, sizeof *x, "x");

count = 25;
y = mycalloc(count, sizeof *y, "y");

free(x);
free(y);
return 0;
}
*/[/purple]