0

In a very known C code distribuited under GPL3 I found the following program (I just report here a minimal example and not the whole code for simplifying).

#include <stdio.h>

typedef struct {

    float *x;

} mystruct;

mystruct var_struct;

#define ALPHA       0
#define BETA        1
#define GAMMA       2
#define DELTA       3

#define MACRO1      var_struct.x[ALPHA]
#define MACRO2      var_struct.x[BETA]
#define MACRO3      var_struct.x[GAMMA]
#define MACRO4      var_struct.x[DELTA]


int main()
{
    mystruct var_struct;

    float a = 5.6f;

    MACRO1 = a;

    return 0;
}

I admit that I'm quite confused about this style of writing and using pointers, since I had preferred each separated fields for pointers in the structure itself and not a shared pointer. But this is what I found and - since I don't think that it could work - I decided to analyse the code step by step, so I can learn something new or understand something that I don't know yet.

Anyway if I run the code on the platform architecture (ARM7) it compiles and runs. If I try the same code on the gcc (GCC) 5.3.1 20151207 (Red Hat 5.3.1-2) it compiles but it doesn't run (Segmentation fault).

Question #1: To me wanted the programmer to use one shared pointer to a variable, which is going to be overwritten simply using the MACROx defined macros. But in this case I never can be sure which value as been assigned to the pointer *x, if I didn't track which macro I have already used. Or am I wrong?

Question #2: As stated here the #define macro simply replace text in the code. So even if I used MACRO1, MACRO2 or MACRO3... they simply refer to the same variable. In this case through its address in the memory. Or not?

Question #3: Is such a codestyle compatible with the most used and spreaded C techniques and C coding rules? I don't think so...but I m unsure. Maybe the programmer is using some very efficient methods for code writing, which are not very common.


UPDATE

Thanks to the many answers and comments. Why I didn't post the code?!? Because otherwise I would reminded to post just a minimal example and not the whole code. So, it is just to respect rules. I don't hide anything. So here the code. The structure I'm talking about is in nav_ukf.h declared and it is called navUkfStruct_t In the file nav_ukf.c you can read in the function navUkfInitState() how those defines are used. I couldn't find any malloc or calloc.

Question #4: Since pointers must be allocated using one of the functions above (malloc, calloc), why the minimal example works?!?

Community
  • 1
  • 1
Dave
  • 349
  • 4
  • 22
  • 3
    aren't you missing the allocation for `var_struct.x`? – Sourav Ghosh Jul 20 '16 at 13:38
  • 2) No they are not. They refer to `var_struct.x[0]` through `var_struct.x[3]` – Eugene Sh. Jul 20 '16 at 13:40
  • @SouravGhosh that's all...really.... – Dave Jul 20 '16 at 13:41
  • 3) Macros are generally discouraged if can be avoided. Macros hiding important type information like these in question are highly discouraged.. – Eugene Sh. Jul 20 '16 at 13:41
  • @EugeneSh. Clear, but in the sctructur I have only one declared pointer and not an array of pointers – Dave Jul 20 '16 at 13:42
  • As @SouravGhosh said - it's an invalid code, missing the proper allocations. – Eugene Sh. Jul 20 '16 at 13:42
  • Corrected with the missing line code – Dave Jul 20 '16 at 13:47
  • 1
    It doesn't matter. `x` is still not allocated. Your code has an undefined behavior. – Eugene Sh. Jul 20 '16 at 13:49
  • This is really what I can extract from the original code!!!! Anyway your information about the allocation is really important! – Dave Jul 20 '16 at 13:50
  • 1
    It is. Otherwise it is just broken. But I bet you can"extract" more. – Eugene Sh. Jul 20 '16 at 13:51
  • 2
    There are two possibilities here. You may have oversimplified your example, or there may be a bug in the original. It would really help if you provided a link to the original, and the name of the specific source file that you started from when developing your example. – zwol Jul 20 '16 at 13:52
  • is it possible the struct to be placed "over" something else? e.g. floar x[5]; mystruct *var_struct = &x; Also I see var_struct declared twice. – Nick Jul 20 '16 at 13:53
  • I can answer your question #3 now, though: This sort of trickery with macros **used** to be normal and unremarkable, but nowadays it is strongly discouraged. One reason it is no longer considered acceptable is that it confuses new readers of the code. Back in the day (and I'm talking 1985–2000 here) that was less important. The primary rationales for doing this sort of thing, when it was normal, were terseness (very important when all you have is an 80x25 glass tty) and poor man's encapsulation (nowadays you're better off jumping to C++ if you need more encapsulation). – zwol Jul 20 '16 at 13:54
  • If it's under GPLv3, why couldn't you "extract" more, or link to the code in question? – smrdo_prdo Jul 20 '16 at 14:05
  • 1) there is no pointer in the posted code, NOR in the linked header file, however, the posted code does say the field in the struct is a pointer and the other items say the field is a pointer to an array. 2) the posted code is not representative of the linked header file. – user3629249 Jul 21 '16 at 16:26

2 Answers2

2

In the original code, navUkfData.x is initialized before anything writes to it. You may have missed this because you were searching only for the macros, not for direct use of navUkfData.x (which demonstrates one of the several reasons why this kind of code is considered poor style). You did find the right function, though: the key lines of navUkfInitState are

navUkfData.kf = srcdkfInit(SIM_S, SIM_M, SIM_V, SIM_N, navUkfTimeUpdate);
navUkfData.x = srcdkfGetState(navUkfData.kf);

srcdkfGetState is defined here:

float *srcdkfGetState(srcdkf_t *f) {
    return f->x.pData;
}

and srcdkfInit is defined here:

srcdkf_t *srcdkfInit(int s, int m, int v, int n, SRCDKFTimeUpdate_t *timeUpdate) {
    ...
    matrixInit(&f->x, s, 1);
    ...

and matrixInit is defined here:

void matrixInit(arm_matrix_instance_f32 *m, int rows, int cols) {
    float32_t *d;

    d = (float32_t *)aqDataCalloc(rows*cols, sizeof(float32_t));

    arm_mat_init_f32(m, rows, cols, d);
    arm_fill_f32(0, d, rows*cols);
}

and arm_mat_init_f32 is defined here:

void arm_mat_init_f32(
  arm_matrix_instance_f32 * S,
  uint16_t nRows,
  uint16_t nColumns,
  float32_t * pData)
{
  /* Assign Number of Rows */
  S->numRows = nRows;

  /* Assign Number of Columns */
  S->numCols = nColumns;

  /* Assign Data pointer */
  S->pData = pData;
}

So, ultimately, navUkfData.x will be set to point to memory allocated by aqDataCalloc, whose definition I will not quote because it appears to be specific to a particular piece of embedded hardware.

So the answers to your questions are:

if I run the code on the platform architecture (ARM7) it compiles and runs. If I try the same code on the gcc (GCC) 5.3.1 20151207 (Red Hat 5.3.1-2) it compiles but it doesn't run (Segmentation fault).

If this is talking about your cut-down example: it only appears to work, by accident, on the "platform architecture". It is buggy, because it never initializes x. The macros do not avoid the need to initialize x.

If this is talking about the original program: It appears to have been written to run only on a specific piece of embedded hardware. Making it work outside that context could be man-months of effort.

To me wanted the programmer to use one shared pointer to a variable, which is going to be overwritten simply using the MACROx defined macros. But in this case I never can be sure which value as been assigned to the pointer *x, if I didn't track which macro I have already used. Or am I wrong?

I'm not sure I understand this question, but: Using the MACROx macros does not change the pointer x itself, only the elements of the array that x points to. Your confusion is perhaps another reason why these macros are poor style.

The #define macro simply replace text in the code. So even if I used MACRO1, MACRO2 or MACRO3... they simply refer to the same variable

They do all refer to the "same variable" in the sense that they all refer to x. However, x is an array, and they refer to different elements of that array.

Is such a codestyle compatible with the most used and spreaded C techniques and C coding rules? I don't think so...but I m unsure. Maybe the programmer is using some very efficient methods for code writing, which are not very common.

As I said in comments earlier: This sort of trickery with macros used to be normal and unremarkable, but nowadays it is strongly discouraged. An important reason it is no longer considered acceptable is that it confuses new readers of the code, as this question demonstrates. Back in the day (and I'm talking 1985–2000 here) that was less important.

The primary rationales for doing this sort of thing, when it was normal, were terseness (very important when all you have is an 80x25 glass tty) and poor man's encapsulation (nowadays you're better off jumping to C++ if you need more encapsulation). It is not done for efficiency.

Why I didn't post the code?!? Because otherwise I would reminded to post just a minimal example and not the whole code. So, it is just to respect rules. I don't hide anything.

It was appropriate to simplify the code in order to ask this question. However, you should also have posted a link to the original, because you knew that you didn't understand the original code, and therefore you should also have known that you might have gotten the simplification wrong. That would have saved everyone some time.

zwol
  • 135,547
  • 38
  • 252
  • 361
0

Assuming original code is OK and working, there are two possibilities:

  1. Allocation - Probably struct is allocated in memory.

    mystruct var_struct;
    var_struct.x = malloc(5 * sizeof(float));
    
    // then you can do this:
    var_struct.x[3] = 5.6f;
    
  2. Struct is placed "over" something else:

    float placeholder[5];
    
    mystruct var_struct;
    var_struct.x = &placeholder;
    
    // then you can do this:
    var_struct.x[3] = 5.6f;
    

There is third possibility - this to be C++ code and instead of pointers they to use references. Result is same as in C with pointers.

Nick
  • 9,962
  • 4
  • 42
  • 80