C: Returning an array from a function into a variable

[noname4me]

Hi

I am trying to return an array from a function that is reading from a memory location in the flash memory.

The data is read as a uint32_t, but is chunked in uint8_t (it is storing some numbers).

I am not able to read the numbers from the flash, so I thought that reading it as a uint32_t, then making a union of that with a uint8_t and reading it off using that was a work around.

The function that reads the memory is working, as I can see the data in the uint8_t form.

I am now trying to pass that information back to my calling function:

Code: [Select]

uint8_t * read_data_8(uint16_t idx, uint16_t len)
{

union d8a_t temp;
uint8_t outfiles[32];
__disable_irq();
EE_Reads(idx, len, &temp.bigdata);
__enable_irq();

memcpy(outfiles, temp.chunks, (strlen(temp.chunks)));

return outfiles;
}
This appears to work as far as outfiles contains the numbers I expect.


My receiving code looks like this:

Code: [Select]

uint8_t * TESTMENOW;
TESTMENOW =  read_data_8(BT_NAME,3);

int t1 = TESTMENOW;
int t2 = TESTMENOW +1;
int t3 = TESTMENOW +2;

and TESTMENOW gives me a 0x0 in my debugger (Atollic).


t1 = 0
t1 = 1
and t2 = 2


I am sure I'm doing something stupid with the pointers, but I tried similar code on https://www.tutorialspoint.com/compile_c_online.php and I was able to return the array.

Code: [Select]

#include <stdio.h>

/* function to generate and return random numbers */
int * getRandom( ) {

static int  temp[10] = {0};
temp[1] = 'A';

temp[1] = 'A';

temp[2] = 'B';

temp[3] = 'C';

temp[4] = 'D';


   return temp;
}

/* main function to call above defined function */
int main () {

   /* a pointer to an int */
   int *p;
   int i;

   p = getRandom();

   for ( i = 0; i < 10; i++ ) {
      printf( "*(p + %d) : %d\n", i, *(p + i));
   }

   return 0;
}

Can anyone please give me a clue on what I'm doing wrong?

Thanks

[magic]

You are returning pointer to a local stack variable which ceases to exist when the function returns.

Technically, the bits still remain on the stack so you have some chance of getting them as shown by your second sample, but they can be erased at any time by the calling function making some new stack allocation or by an interrupt.

It seems that your debugger helpfully zeros out the stack after returning to catch exactly that kind of bugs.

You need to accept a pointer to a buffer allocated by the calling function, like EE_Reads does, or return those bytes by value.

[GromBeestje]

You are returning outfiles, which is a local declaration in the read_data_8. A pointer to this array is no longer valid when you the function goes out of scope. A possible solution is to make it static

static uint8_t outfiles[32];

This means, there will be a fixed memory block allocated for the array that will always be reserved for this function, so it won't get overwritten any time the function returns. Note that this memory will always be allocated, and that this also means that calling the multiple times, it will always return the same pointer, and it will contain the data of the last call.

[magic]

And if you store that pointer somewhere and call the function again, the value pointed by the old pointer will change. That's asking for some debugging adventure.

[noname4me]

Ahh I see...this makes sense

I am not on the PC now, but I wanted to ask:

If I cleared the outfiles array at the start of the function (by filling with NULL or NULL terminating each time data is entered into this array) then would this be a "safe" way to use this function?

Safe in the sense that I'm not likely to get hard faults...

I wi try this in a couple of hours when I can get to the PC.

Thanks

EDIT: I meant after setting it to static...

Sent from my SM-N960F using Tapatalk

[T3sl4co1l]

Right, it needs to be allocated elsewhere.

To do this in OOP+GC, you simply return the object and forget about it, no problem.  C is too simplistic for that method however.

The hidden steps of that operation, are: allocating the object on the heap (C's malloc), returning the pointer to the object, and later disposing of the object (free) when it falls out of scope.

If you ever forget to free the object, it gets stuck on the heap.  This is called a memory leak.  If you ever free an object then reuse its pointer, you'll be reading garbage, and randomly get anything from protection faults to possible code execution vulnerability (use-after-free).

So while you could malloc inside the function, return a pointer, and free once it's used, it's probably not a good idea.  Better not to have functions with side effects (namely, modifying the heap).

So the better way is to pass in a pointer to available memory.  In other languages, this is a pass-by-reference.  You'll need,

Code: [Select]

void read_data_8(uint16_t idx, uint16_t len, uint8_t * buf) { ... }

...

read_data_8(idx, len, &buf);

You could also break it into multiple accesses, which is ugly since those would be multiple different function calls and you might as well inline it or something.  But sometimes that's an okay way to go.

Tim

[Mechatrommer]

If I cleared the outfiles array at the start of the function (by filling with NULL or NULL terminating each time data is entered into this array) then would this be a "safe" way to use this function?
Safe in the sense that I'm not likely to get hard faults...

no, the point is outfiles is only temporary allocated during the life of read_data_8 function, once the program goes out (exit) of read_data_8, the outfiles will be technically terminated (invalid) you are lucky your compiler is clever enough to zero the returned value, otherwise as other said, debugging adventure because the why your program crashed on another part of your code, or that out of boundary memory exploit hack that everybody slag about C/C++. the only solution is to expand the context or lifetime of the returned pointer (outfiles), ie by declaring it global or static as above poster suggested, or use pointer returned by malloc (if your compiler ever support it) but then thats going to be fun tracing alive memory to manually terminate them.

[noname4me]

Code: [Select]

uint8_t TESTMENOW[32];
read_data_8(BT_NAME, TESTMENOW,3);

int t1 = TESTMENOW[0];
int t2 = TESTMENOW[1];
int t3 = TESTMENOW[2];


and

Code: [Select]

uint8_t * read_data_8(uint16_t idx, uint8_t outfiles[], uint16_t len)
{

union d8a_t temp;
//uint8_t outfiles[32];
__disable_irq();
EE_Reads(idx, len, &temp.bigdata);
__enable_irq();

memcpy(outfiles, temp.chunks, (strlen(temp.chunks)));

return outfiles;
}
now returns my numbers back to me

Thanks for all your help - very appreciated

[Mechatrommer]

whats the purpose of returning outfiles then on the latest code? if its already manipulated in the function which will already take effect when exiting function. its just a waste one line of code, remove that and make the function of type void to avoid further waste on stack allocation (in case compiler is not smart enough). there are few more discussions for improvement such as reusability, but since you think you fixed it, so i think this one is concluded, ymmv and have a nice day.

[noname4me]

Lol... sorry... it does nothing because I don't store it anywhere as a returned value.

It can be deleted and the code works the same

Btw: any tips you can give would be welcome,  I'm always up for improvement

Sent from my SM-N960F using Tapatalk

[NorthGuy]

Code: [Select]

uint8_t * read_data_8(uint16_t idx, uint8_t outfiles[], uint16_t len)
{

union d8a_t temp;
//uint8_t outfiles[32];
__disable_irq();
EE_Reads(idx, len, &temp.bigdata);
__enable_irq();

memcpy(outfiles, temp.chunks, (strlen(temp.chunks)));

return outfiles;
}

Why not simply:

Code: [Select]

uint8_t * read_data_8(uint16_t idx, uint8_t outfiles[], uint16_t len)
{
__disable_irq();
EE_Reads(idx, len, outfiles);
__enable_irq();
return outfiles;
}
?

[obiwanjacobi]

A general read pattern like this usually returns the number of bytes placed in the buffer (array) passed into the function. In scenarios where you read from a long stream for instance, you can transfer the data in chunks (buffers/arrays) of a certain length. When you reach the end of the stream, the passed in buffer may not be filled all the way, which can be detected by the number of bytes returned from the function.

(in your case source can be omitted)
int read([void* source], byte* destination, int destMaxLength, int numberOfBytesToRead)

[2c]

[T3sl4co1l]

Why not simply:

Code: [Select]

uint8_t * read_data_8(uint16_t idx, uint8_t outfiles[], uint16_t len)
{
__disable_irq();
EE_Reads(idx, len, outfiles);
__enable_irq();
return outfiles;
}
?

And for that matter, why not simply inline it, or use a #define as shortcut?

Code: [Select]

#define read_data_8(idx, outfiles, len) \
__disable_irq(); \
EE_Reads((idx), (len), (outfiles)); \
__enable_irq();

(Or to cover more pathological cases of syntax, maybe those statements should be comma-separated, inside of parenthesis, with a final void statement to remove a return value.)

Which goes back to what I mentioned earlier, "you might as well inline it".

If there's more to the function than what we see here -- this is just a small part for example purposes -- keeping it in a function would be justified of course.

By the way, at -O3 and -Os optimization, automatic function inlining will be attempted, and you don't have to actually write it inline yourself; the compiler will figure out the best way.

Tim

[Mechatrommer]

And for that matter, why not simply inline it, or use a #define as shortcut?

Code: [Select]

#define read_data_8(idx, outfiles, len) \
__disable_irq(); \
EE_Reads((idx), (len), (outfiles)); \
__enable_irq();


imho that is one archaic way of coding, harder to maintain even to re-type / edit. and some debugger will not bother to step into each line and function within the #define body making debugging work even harder. as you mentioned, -O3 and -Os option already can figure it out, or at least explicitly use "inline" keyword in the function if we want to highlight (to other programmers) that the function should be better inlined (even if compiler choose not to). but then we dont know how many times the OP will make calls to this function, too many calls will increase compiled code by few bytes, not good if space is critical, but otherwise can be a good thing for performance when space is abundant. i did like what you've suggested once, but it had very specific purpose, more like construct codes or shorcuts, but mere inlining and maintainability are not among the purposes.

[Nominal Animal]

Given that the data is actually an array of uint32_t's, you'll want to write the function as

Code: [Select]

static inline void read_data(uint16_t  idx,
                             uint32_t *data,
                             uint16_t  bytes)
{
    __disable_irq();
    EE_Reads(idx, bytes, (uint8_t *)data);
    __enable_irq();
}
so that to read 5 values from index idx to a buffer buffer[], you use

Code: [Select]

        uint32_t  buffer[5];

        read_data(idx, buffer, sizeof buffer);
If you wanted to read buffer[1] and buffer[2] only, use read_data(idx, buffer + 1, 2 * sizeof buffer[0]).

Here are the reasons:

• Functions marked static inline are embedded in their call sites (if possible), making them as fast as preprocessor macros.
  
• On most architectures, __disable_irq() and __enable_irq() compile to a single machine instruction.  As the function is just a call to another function, embedding (more properly, inlining) makes sense code size-wise too.
  
• Instead of a temporary buffer, you can just cast the pointer to the desired type, because the other type is effectively a pointer to unsigned char.  (While this is a form of type punning, that cast is actually used to refer to the storage representation of the data you're interested in.  If you wonder about the standards conformance of this, see why and how memcpy() can be used to copy the storage representation of any data type.)
  
• By using proper function parameter types, you help the programmer at compile time, as the types of the parameters are checked.
• When specifying arrays by their names, they will automatically decay to a pointer to the first element of the array.  This means that &(buffer[1]) and buffer+1 evaluate to the exact same thing, to a pointer to the second element in the buffer array.
  
• The Principle of Least Surprise.  This interface is similar to other read()-like functions, making it familiar to programmers; so, they're less likely to be surprised by its behaviour and accidentally write buggy code due to wrong assumptions.
  

[langwadt]

Why not simply:

Code: [Select]

uint8_t * read_data_8(uint16_t idx, uint8_t outfiles[], uint16_t len)
{
__disable_irq();
EE_Reads(idx, len, outfiles);
__enable_irq();
return outfiles;
}
?

And for that matter, why not simply inline it, or use a #define as shortcut?

Code: [Select]

#define read_data_8(idx, outfiles, len) \
__disable_irq(); \
EE_Reads((idx), (len), (outfiles)); \
__enable_irq();


just don't call it anywhere interrupts are not enabled

[golden_labels]

Actually: don’t call it at all. In some contexts it will introduce syntax error or change program flow in a non-obvious way — including executing code that should never been executed according to what a human sees in the code.

Macros are not functions. This is a literal text replacement engine, that has no idea about C syntax. The macro should be at least defined like that:

Code: [Select]

#define RAD_DATA_8(idx, outfiles, len) do {\
        __disable_irq();
EE_Reads((idx), (len), (outfiles));
__enable_irq();
    } while (false)However, while in this particular case it will work, beware: this is still a horrible trap in more complex situations. For example you are not allowed to ever reference a parameter more than once — because that may lead to multiple invocations of code with side effects. Unless it is documented as a name or a part of a name, which implies only referencing a variable or function. But that exception has exceptions to itself: for examle volatile variables (performance issues, which are not obvious from what the invocation suggests; inconsistent value of a variable within the macro expansion), names of functions with side effects or accessing volatile variables themselves (value inconsistencies; multiple side-effects of seemingly single invocation) etc. And I bet that this exception to exception has exceptions itself, perhaps with exception with exceptions to exceptions. And some additional gotchas.

Since a macro is just replacing text, the source code effectively contains multiple copies of similar pieces of code. The compiler may have hard time optimizing that, leading to performance or memory footprint problems.

tl;dr: just use proper C abstractions and avoid text replacement as much as possible. You will save yourself a lot of ugly problems. Unless you understand what are the actual outcomes of the changes you make — that includes at least ability to properly profile code read raw assembly.

[T3sl4co1l]

just don't call it anywhere interrupts are not enabled

Ah yes, quite true; true of the original function, too.

That's always been one annoying thing about atomic operations, on many platforms -- it's often a big annoyance to save the interrupt flag(s), especially when you just need some teeny operation.

Speaking of teeny, depending on how long EE_Reads() takes, you might not want to disable interrupts at all -- the greatly (and randomly) increased interrupt latency may cause problems elsewhere.  Or, find a better way of handling it altogether (say, an asynchronous model: request a read operation, when complete fire an interrupt and execute callback function).

Actually: don’t call it at all. In some contexts it will introduce syntax error or change program flow in a non-obvious way — including executing code that should never been executed according to what a human sees in the code.

Yes, quite, as I alluded to:

(Or to cover more pathological cases of syntax, maybe those statements should be comma-separated, inside of parenthesis, with a final void statement to remove a return value.)

Wrapping in a do-statement is the most common patch for this (which I forgot at the time).

Basically, avoid making pathological expressions and statements in general, and especially when using macros.  Easily avoided.

Long expressions aren't optimized either -- also worth keeping in mind.  My most recent example of this was a bit-shifting operation (specifically, converting from packed 5-6-5 RGB <> 24-bit RGB color): easy enough to write a one-liner of, but the result is basically unoptimized (>100 instructions I think it was).  Written in a more suggestive manner (using local variables in a loop structure) it was still pretty ugly, but significantly shorter and faster.

Tim

[IDEngineer]

It's terrible form to disable interrupts around a function call:

__disable_irq();
EE_Reads((idx), (len), (outfiles));
__enable_irq();

...for several reasons, not the least of which is that you're including all the function call overhead in the disable period. Even if it "works" today, the risk is high that someone will modify the interrupt system in the future, not realizing that interrupts can be off for a lengthy period of time. Yes, I know others have spoken of inlining functions, but it's almost certain you'd still be including code within the "interrupts off" periods that doesn't need that protection. You should work aggressively to minimize the clock cycles during which interrupts are turned off. Usually it's just one or two lines of code that really matter, and not everything that your (inlined) function does.

[SiliconWizard]

It's terrible form to disable interrupts around a function call:

__disable_irq();
EE_Reads((idx), (len), (outfiles));
__enable_irq();

...for several reasons, not the least of which is that you're including all the function call overhead in the disable period. Even if it "works" today, the risk is high that someone will modify the interrupt system in the future, not realizing that interrupts can be off for a lengthy period of time. Yes, I know others have spoken of inlining functions, but it's almost certain you'd still be including code within the "interrupts off" periods that doesn't need that protection. You should work aggressively to minimize the clock cycles during which interrupts are turned off. Usually it's just one or two lines of code that really matter, and not everything that your (inlined) function does.

True enough. And if you really have "no choice" doing that, I'd suggest at least adding a #warning directive (or similar), so that everytime you'd compile code calling that, the compiler would spit out a warning suggesting to be extra careful.

[mfro]

... To do this in OOP+GC, you simply return the object and forget about it, no problem.  C is too simplistic for that method however.

It's not.

Technically, you can do the exact same thing in C. Putting any efficiency concerns aside (returning a compound object from a function on the stack isn't exactly efficient), you just need to return a struct that wraps the array:

Code: [Select]

struct arr_struct {
    char array[20];
};

struct arr_struct return_an_array(void)
{
    struct arr_struct s = { .array = { 'a', 'b', 'c', '\0' } };

    return s;
}


[IDEngineer]

Technically, you can do the exact same thing in C. Putting any efficiency concerns aside (returning a compound object from a function on the stack isn't exactly efficient), you just need to return a struct that wraps the array....

Or, since it has to be a non-local object to pass between functions anyway, just use a pointer. Since the OP mentions "flash" this is likely an embedded environment, so minimizing stack space is a larger concern than it might otherwise be. And as you note, in any environment pushing/popping entire data structures is horribly inefficient compared to a pointer.

[Mechatrommer]

It's terrible form to disable interrupts around a function call:

__disable_irq();
EE_Reads((idx), (len), (outfiles));
__enable_irq();

then  where you suggest to put disable/enable interrupt calls? only in main() function? that will certainly put program in high constraint and hence difficult to expand anyway. if you extra taboo about it you may create your own protectied or guarded functions such as....

Code: [Select]

int my_disable_irq_count = 0; // global

my_disable_irq() {
if (!my_disable_irq_count) __disable_irq();
my_disable_irq_count++;
}

my_enable_irq() {
if (my_disable_irq_count == 1) __enable_irq();
if (my_disable_irq_count > 0) my_disable_irq_count--;
}
or vice versa, hence...

my_disable_irq();
EE_Reads((idx), (len), (outfiles));
my_enable_irq();

i heard some people said about performance penalty/overhead? well you asked for it, performance or safety, pick one. tell me any programming language that is as fast as C/C++/assembly yet as safe as ADA/Phyton/Java? there is no free lunch and dinner...

[NorthGuy]

tell me any programming language that is as fast as C/C++/assembly yet as safe as ADA/Phyton/Java? there is no free lunch and dinner...

As Benjamin Franklin said: "The one who agrees to trade Liberty for Safety, doesn't deserve neither Liberty nor Safety".

[langwadt]

It's terrible form to disable interrupts around a function call:

__disable_irq();
EE_Reads((idx), (len), (outfiles));
__enable_irq();

then  where you suggest to put disable/enable interrupt calls? only in main() function? that will certainly put program in high constraint and hence difficult to expand anyway. if you extra taboo about it you may create your own protectied or guarded functions such as....

inside the EE_Reads function around the lines that actually need it, no need to have disable during the
overhead for calling and returning from the function