Strange error with preprocessor

Question

Currently I am having a very strange error with the preprocessor. I have searched and couldn't find anything relavent and therefore would like to post it here. Below is my sample code

#include <stdio.h>

#define DDR_BASEADDR  0x000000000U + 128*1024*1024

int main()
{
     uint32_t* test2 = (uint32_t*) DDR_BASEADDR;

     uint32_t whatIsThis = DDR_BASEADDR;

     uint32_t* test3 = (uint32_t*) whatIsThis;

     printf( "%x %x %x %x\n\r", DDR_BASEADDR, test2, test3, whatIsThis);

     return 0;
}

The output of this code should all be 0x8000000. However, the outputs are:8000000 20000000 8000000 8000000.

I believe it is not the datatype that caused this problem, since it also appears even if i change uint32_t* test2 = (uint32_t*) DDR_BASEADDR; to int32_t* test2 = (int32_t*) DDR_BASEADDR;

I test this both on ARM A9 from the Zedboard and with C++ online compiler and get the same result. Thank you for your time and effort.

Thang Tran

Hi Yunnosch, thank you for your answer. I tried it just now and you're right. Do you have any idea the reason behind this? — thangktran, Jun 25 '17 at 10:15
Still thinking, it was more of a gut feeling, triggered by seeing an operation in a macro, without braces. — Yunnosch, Jun 25 '17 at 10:16
To print a pointer using `printf` you should use the `"%p"` format, and its argument must be a `void *`. Mismatching formats and arguments leads to *undefined behavior*. — Some programmer dude, Jun 25 '17 at 10:18
@Yunnosch : That confused me for a moment - `{...}` are "braces", `(...)` are _parentheses_. — Clifford, Jun 25 '17 at 12:24
Don't use macros to define values in C++. Use const/consterpr. — , Jun 25 '17 at 12:30
@Clifford Thanks for pointing out (no sarcasm). Actually I was unsure. That is why I added a quote of what exactly I mean. :-) — Yunnosch, Jun 25 '17 at 13:03

score 8 · Accepted Answer · answered Jun 25 '17 at 10:17

8

The macro expansion gives you

uint32_t* test2 = (uint32_t*) 0x000000000U + 128*1024*1024;

when you probably expected

uint32_t* test2 = (uint32_t*) (0x000000000U + 128*1024*1024);

That's why it is recommended to use lots of parenthesis in macro definitions, or - even better - use a function.

answered Jun 25 '17 at 10:17

Bo Persson

90,663
31
146
203

1

He did not expect the behavior of parenthesized expression (it would be `0x2000000` in all four printouts). He wanted `0x8000000` in the case when there's no cast to pointer, which he cannot get without making `0x000000000U` a pointer inside the macro itself. – Sergey Kalinichenko Jun 25 '17 at 10:21
2

*or - even better - use a function.* You have a reason why a function is better than macro with parenthesis? – unalignedmemoryaccess Jun 25 '17 at 10:22
1

Do *you* have a reason why a macro is better than a function, @tilz0R? A function should be your default choice unless there is some reason you actually need a macro. (Note, though, that a *constant* would have worked here, too.) – Cody Gray - on strike Jun 25 '17 at 11:33
1

@CodyGray But macro is faster? – unalignedmemoryaccess Jun 25 '17 at 11:34
3

No, macros are not faster. Compilers have known how to inline functions since…I don't know, at least 20 years. @tilz0R – Cody Gray - on strike Jun 25 '17 at 11:34
1

That's correct, but don't tell me that inline function can be faster than this macro in his case? @CodyGray It can only be at the same performance, just not better in any case. – unalignedmemoryaccess Jun 25 '17 at 11:35
1

Neither is faster than the other. It isn't a speed issue. It is a "make it easy to write correct, bug-free code" issue. – Cody Gray - on strike Jun 25 '17 at 11:35
You can end up with a proliferation of parentheses; but the recommendation is not a simple as "lots of parentheses". The entire expression needs _one_ outer pair of parentheses, and for function like macros with arguments, each instance of an argument in the expression requires parentheses. Anything else is dependent of the semantics of the expression itself regardless of it being a macro or not. – Clifford Jun 25 '17 at 12:18

unalignedmemoryaccess · Answer 2 · 2017-06-25T11:30:49.733

Your macro was expandend to:

uint32_t* test2 = (uint32_t*) 0x000000000U + 128*1024*1024;

Which is the same as doing this:

//Check multiply by sizeof at the end
uint32_t* test2 = (uint32_t *) (0x000000000U + 128*1024*1024 * sizeof(*test2));

because your size of the pointer type is sizeof(uint32_t) and therefore any operation for increase/decrease multiplies by this size.

What you want is:

uint32_t* test2 = (uint32_t *) (0x000000000U + 128*1024*1024);

Which means you first do calculation of address, then you cast to the desired pointer.

Sergey Kalinichenko · Answer 3 · 2017-06-25T10:57:43.937

There is absolutely nothing strange about this: on this line

uint32_t* test2 = (uint32_t*) DDR_BASEADDR;

the cast to pointer operation is applied to 0x000000000U, after which an int of 128*1024*1024 is added to the result of the cast, i.e.

uint32_t* test2 = (uint32_t*) 0x000000000U + 128*1024*1024;
//                ^^^^^^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^^^^^
//                       Pointer                Offset

This means the pointer arithmetic is performed, so in integer terms the offset value is multiplied by sizeof(uint32_t), yielding 0x8000000.

However, on this line

uint32_t whatIsThis = DDR_BASEADDR;

there is no cast to pointer, so the addition is done entirely in integers. That is why there is no multiplication by sizeof(uint32_t), and the result is 0x20000000.

If you want all four cases printing 800000000, cast to pointer needs to be inside the macro:

#define DDR_BASEADDR  ((uint32_t*)0x000000000U + 128*1024*1024)

and a cast to int32_t needs to be on the declaration of whatIsThis:

uint32_t whatIsThis = (uint32_t)DDR_BASEADDR;

However, uint32_t is not a portable type for the pointer. Use uintptr_t instead.

*However, uint32_t is not a portable type for the pointer. Use uintptr_t instead.* It is defined in C++11 and later. — unalignedmemoryaccess, Jun 25 '17 at 11:32
The *type* `uint32_t` is defined in C++11 and later, but it isn't defined as being the same size as a pointer, @tilz0R. On lots of systems, pointers are 64 bits, and you will truncate them if you store them in a `uint32_t`. — Cody Gray - on strike, Jun 25 '17 at 11:49
@CodyGray I agree just tried to point out that it is not C89. — unalignedmemoryaccess, Jun 25 '17 at 11:50

Yunnosch · Answer 4 · 2017-06-25T10:44:18.250

With

#define DDR_BASEADDR  0x000000000U + 128*1024*1024

the code line

uint32_t* test2 = (uint32_t*) DDR_BASEADDR;

expands to

uint32_t* test2 = (uint32_t*) 0x000000000U + 128*1024*1024;

which is (because of the ways of pointer arithmetic) practically

uint32_t* test2 = (uint32_t*) (0x000000000U + (sizeof(uint32_t) *(128*1024*1024));

or

uint32_t* test2 = (uint32_t*) (0x000000000U + (4 * 0x8000000));

That gives you 20000000 instead of 8000000, both in hex format of course.
Multiplying by 4 in hex, for multiples of 4, is like multiplying by 16 (which in hex is like multiplying by 10 in decimal) and dividing by 4.
I.e. it results in 1/4 the value, shifted left by one hex-digit: 80->200.

The other code lines have the effect of a pair of braces () in some way or another.

If you use

    #define DDR_BASEADDR  (0x000000000U + 128*1024*1024)

you should be fine, and are, as you have already verified.

Using braces generously is very good practice, especially with macros.

Note: You also should be more careful with printf and using the correct representation of the values you want to print (see comments and other answers). Otherwise the resulting Undefined Behaviour is evil! All kinds of unwanted and inexplicable things can result from UB. However UB is not needed for explaining the values you get in this special case. (You got lucky.)

Strange error with preprocessor

4 Answers4