Bad pointer math

cancel
Showing results for 
Search instead for 
Did you mean: 

Bad pointer math

Jump to solution
1,311 Views
Suudy
Contributor I

I've got some code that calculates a pointer.  In one case I always get an offset from 0 (NULL), but the other always works correctly.  Let me post the code and I'll explain the problem:

 

 

typedef struct
{
  unsigned char pop_index;  ///< Pop index
  unsigned char push_index; ///< Push index
  void          *start;     ///< Pointer to underlying storage
  unsigned char reclen;     ///< Record size
  unsigned char used;       ///< Number of entries used
  unsigned char mask;       ///< Mask of the number of records
} queue_t;

queue_status_t queue_pop(queue_t *q, void *p){                         const unsigned char *src;  if ( q->used == 0 )  {    return Q_EMPTY;  }  if ( p != NULL )  {    src=(const unsigned char *)(q->start) + ((q->pop_index & q->mask) * q->reclen);     (void)memcpy(p, src, q->reclen);  }  q->pop_index++;  q->used--;  return Q_OK;}queue_status_t queue_push(queue_t *q, const void *p){  unsigned char *dst;  if ( q->used == q->mask )  {    return Q_FULL;  }  dst = (unsigned char *)(q->start) + ((q->push_index & q->mask) * q->reclen);   (void)memcpy(dst, p, q->reclen);  q->push_index++;  q->used++;  return Q_OK;}

 

Now, the problem is in queue_pop().  Every time I run this, the pointer 'src' is always offset from 0, regardless of the value of q->start.  But queue_push() works fine.  Now they have the exact same pointer math, they just use different values for the offsets.

 

However, if I change queue_push() to the following:

 

 

queue_status_t queue_pop(queue_t *q, void *p){                         const unsigned char *src=(const unsigned char *)(q->start);  if ( q->used == 0 )  {    return Q_EMPTY;  }  if ( p != NULL )  {    src += ((q->pop_index & q->mask) * q->reclen);     (void)memcpy(p, src, q->reclen);  }  q->pop_index++;  q->used--;  return Q_OK;}

 

It works fine.  Note that q->start is not NULL when I come into the function.

 

Any ideas what is occuring here?  I've had a few co-workers look over the code, and they can't seem to find anything in the code that would cause this to occur.

Labels (1)
Tags (1)
0 Kudos
1 Solution
174 Views
CompilerGuru
NXP Employee
NXP Employee

Yes, apears to be a compiler issue :smileysad:.

 

Please report it using the service request so you are in the loop when it gets fixed.

I did report it internally too as MTWX38498, you can mention that as reference.

 

As far as -or is concerned, I would not use this option for a S08. It makes sence to keep stuff in register for cores with tons of registers, but that's not the case for a S08. For such a small chip it often creates more (spill) code due register collisions than without it. Also as general rule, if in doubt do not specify an option.

 

Out of your options,you probably need only " -Ccs08  -OnCstVar -Tue1", the others are default or

only matter for C++ (-OnPMNC or are you using C++?).

-Cc has no effect for ELF (which is default but also choosen with -F2).

 

Daniel

View solution in original post

0 Kudos
9 Replies
174 Views
Suudy
Contributor I

Ok, attached is a project exhibiting the problem.  I've run this both on the target hardware and in the simulator.  On the hardware, it reads from address 0 (or an offset from it).  In the simulator, it fails within queue_pop() on an unitialized memory access during the computation of the 'src' pointer.

 

I used the volatile pointer (const unsigned char * volatile src) and indeed the compiler did not optimize away 'src' so I can see it in the debugger.  And it indeed holds NULL.

 

Note that I ran this code initially with the default compiler options as setup by the new project wizard, and the code ran just fine.  But then I put in the settings we are using on our project, and recompile, it then fails.  Here's what we are using for our project (copied from our makefile):

 

# Default compilation flags
#   -F2       : Use ELF/Dwarf output
#   -Ccs08    : Generate code for HCS08
#   -Cc       : Allocate constant objects into ROM
#   -Ms       : Small memory model
#   -Os       : Optimize for size
#   -Ou       : Optimize dead assignments
#   -Of       : Create sub-functions for common code
#   -OnCstVar : Disable const variable by constant replacement
#   -OnPMNC   : Disable code generation for NULL point to member check
#   -Tue1     : Make enums a single unsigned byte
#
# It isn't clear from the documentation whether this is allocate local variables
# into registers or register optimization.  This "-Or" is discussed on p. 246
# and p. 401 of the compiler documentation, and one says it causes code bloat
# (register allocation) and the other says it reduces code size (register
# optimization).
#   -Or       : Allocate local variables into registers
CFLAGS = -F2 -Cs08 -Cc -Ms -Os -Ou -Of -OnCstVar -OnPMNC -Or -Tue1

 

A couple notes.  First, we use a command line based build using GNU make.  We do not use the IDE.  Second, we invoke the debugger directly on the command line.  However, the sample project I've attached does use the IDE, and exhibits the same problem.

 

Finally, I started tweaking options.  The problem seems to be the "-Or" option.  When I remove this option, the code runs just fine.  Now, we are unclear as to what this option really does (see the above note).  Looking at the help file (Compiler_HC08.chm) it says two (seemingly contradictory) things about this option.

 

 


-Or: Allocate Local Variables into Registers

 

Allocate local variables (char or int) in registers. The number of local variables allocated in registers depends on the number of available registers. Use this option when using variables as loop counters or switch selectors or when the processor requires register operands for multiple operations (e.g., RISC processors). Compiling with this option may increase your code size (spill and merge code).

 



However it also says:

 


-Or: Register Optimization

 

When accessing pointer fields, this option prevents the compiler from reloading the pointer address for each access. An index register holds the pointer value across statements when possible.


 

We aren't sure which actually is the case.  Similar language is found in the PDF documentation (Compiler_HC08.pdf) on p. 246 and p. 401 (in the same respective order as above).

 

Any insight?  Are we misusing options, or does this appear to be a compiler issue?

 

Thanks for the help,

Pete

 

Queue.zip

Message Edited by t.dowe on 2009-10-26 01:30 PM
0 Kudos
175 Views
CompilerGuru
NXP Employee
NXP Employee

Yes, apears to be a compiler issue :smileysad:.

 

Please report it using the service request so you are in the loop when it gets fixed.

I did report it internally too as MTWX38498, you can mention that as reference.

 

As far as -or is concerned, I would not use this option for a S08. It makes sence to keep stuff in register for cores with tons of registers, but that's not the case for a S08. For such a small chip it often creates more (spill) code due register collisions than without it. Also as general rule, if in doubt do not specify an option.

 

Out of your options,you probably need only " -Ccs08  -OnCstVar -Tue1", the others are default or

only matter for C++ (-OnPMNC or are you using C++?).

-Cc has no effect for ELF (which is default but also choosen with -F2).

 

Daniel

View solution in original post

0 Kudos
174 Views
Suudy
Contributor I

CompilerGuru wrote:

Yes, apears to be a compiler issue :smileysad:.

 

Please report it using the service request so you are in the loop when it gets fixed.

I did report it internally too as MTWX38498, you can mention that as reference.


Thanks for the note.  I will submit service request.  For now I have a workaround, but I'm not sure this kind of problem isn't popping up elsewhere.  But then again, eliminating "-Or" seems to have fixed it, and since we will take your advice and remove it, perhaps this isn't a problem.

 


CompilerGuru wrote:

As far as -or is concerned, I would not use this option for a S08. It makes sence to keep stuff in register for cores with tons of registers, but that's not the case for a S08. For such a small chip it often creates more (spill) code due register collisions than without it. Also as general rule, if in doubt do not specify an option.



So which is "-Or"?  Register allocation or register optimization?  The documentation says one causes code bloat (which you mention above) and the other says it reduces code size.  It sounds like "-Or" means register allocation?

 


CompilerGuru wrote:

 

Out of your options,you probably need only " -Ccs08  -OnCstVar -Tue1", the others are default or

only matter for C++ (-OnPMNC or are you using C++?).

-Cc has no effect for ELF (which is default but also choosen with -F2).

 


Ok, we'll limit our options.  We are going to use:  "-F2 -Cs08 -Ms -OnCstVar -Tue1" and keep it there.


Thanks,

Pete

 

 

0 Kudos
174 Views
Suudy
Contributor I

I'm working on a standalone project to demonstrate the problem.  I'll post it when I've created something.

 

In the meantime, I'd like to point out a few things.

 

  1. It is not the debugger reporting a different value.  The code does not work, and passes an NULL (or some offset of NULL) to memcpy.  I've stepped into memcpy, and I've single stepped through the generated assembly.  A NULL pointer gets passed to memcpy.
  2. Further on the first point, I see the data at address 0 and on being copied into my output buffer (pointed to by 'p').  So I'm even more convinced it is not a debugger issu.
  3. I've tried the volatile, but the debugger does not show 'src'.  But as CompilerGuru pointed out, I did not put the volatile after the '*'.  I'm going to try that first.
  4. And when I mention that the code "works fine" I mean that the data coming out of the queue is correct.  It is not correct in the first case, but is correct in the second.  And this isn't inside the function, but in the caller to queue_pop().

As for target and tool information:

 

MC13213 (HCS08)

Small memory model

CW v6.2 (chc08 5.0.26 build 8120)

0 Kudos
174 Views
CompilerGuru
NXP Employee
NXP Employee

What cpu are you targeting, what version of the tools are using?

 

Also is the code not working properly or is the debugger just showing a wrong value for src?

 

I would also suggest to file a service request for this kind of issues. Best if you could provide a

full project showing the problem.

 

Daniel

0 Kudos
174 Views
Lundin
Senior Contributor IV
The problem could indeed be the debugger showing the wrong value, because in the latter case

const unsigned char *src=(const unsigned char *)(q->start);

you probably trick the optimizer to stack that variable, while in the first case it could be using only a CPU index register. If that's the case, you should also receive the correct value if you do like this:

if(p != NULL)
{
volatile const unsigned char *src=(volatile const unsigned char *)(q->start);
src += ((q->pop_index & q->mask) * q->reclen);
}

If that works, yeah then the error is the debugger hiding away the value in the 1st example.
0 Kudos
174 Views
CompilerGuru
NXP Employee
NXP Employee

To check if it is a display error I would suggest to step into memcpy and see where the data

gets read from.

 

When using volatile, to get the compiler not to optimize the pointer value,

the volatile should be after the *. Before the * it means the value the pointer points to is volatile, and 

as this code is not accessing it, it does not matter.

E.g.

const unsigned char * volatile src=(const unsigned char * volatile)(q->start);
 

I tried to simulate the queue_pop method with CW 4.7 and the code behaved properly as far as I

noticed. The src variable however was not shown in the debugger.

 

Please note which chip you are targeting and which tool versions you are using. I tried with a S12, the 

OP might have problems with a S08 or something else. Don't forget to note the memory model or

anything else which might affect the setup.

 

Also when reducing the issue so far it is beneficial to go the last step and create a self contained, compilable sample showing the problem,

in this case the definition of the return type and some test values were all that was missing Smiley Happy

 

Daniel

 

 

 

#include <stdio.h>typedef struct{unsigned char pop_index; ///< Pop indexunsigned char push_index; ///< Push indexvoid *start; ///< Pointer to underlying storageunsigned char reclen; ///< Record sizeunsigned char used; ///< Number of entries usedunsigned char mask; ///< Mask of the number of records} queue_t;typedef enum  {   Q_EMPTY, Q_OK} queue_status_t;  queue_status_t queue_pop(queue_t *q, void *p){                         const unsigned char *src;  if ( q->used == 0 )  {    return Q_EMPTY;  }  if ( p != NULL )  {    src=(const unsigned char *)(q->start) + ((q->pop_index & q->mask) * q->reclen);     (void)memcpy(p, src, q->reclen);  }  q->pop_index++;  q->used--;  return Q_OK;}unsigned char buf[64] = {  0, 1,2,3,4,5,6,7,8,9};queue_t q = {  1,0, buf, 4, 1, 15};void main(void) {  unsigned char buf2[4];  volatile queue_status_t r = queue_pop(&q, buf2);}     

 

 

 

0 Kudos
174 Views
Lundin
Senior Contributor IV
I suspect that the optimization was done by putting the pointer in an index register, and then to avoid that it would make sense to write volatile*. To be 100% sure, write

volatile const unsigned char * volatile src;
0 Kudos
174 Views
Suudy
Contributor I

Lundin wrote:
I suspect that the optimization was done by putting the pointer in an index register, and then to avoid that it would make sense to write volatile*. To be 100% sure, write

volatile const unsigned char * volatile src;

Indeed that did fix it.  Both cases did.  Either "const unsigned char * volatile" or "volatile const unsigned char * volatile" fixed the problem.

 

0 Kudos