-
malloc to struct pointer = hard fault. What am I doing wrong?
Posted by DavidAlfa on 29 Jul, 2021 00:45 -
I'm working on dinamically allocating the screens in the stm32 fw, so I can reduce the ram footprint.
A rough description is: Theres a "widget" struct. It holds some data like position and other things, and also a pointer to the content.
The content is like a "sub-widget", can be a button, editable, display...
So what I'm doing is to first allocate the widget, then allocate the sub-widget depending on the type.
However when accessing the subwidget, it causes hard fault with the combo type.
This worked perfectly fine before, using static allocations. It started to happen after using malloc. sizeof reports the correct sizes. mallinfo also reports the correct size being allocated (+8 bytes per malloc).
It's not running out of heap or stack, it happens with the very first allocation, it's just with the combo type. I can allocate anything else correctly.Code: [Select]struct selectable_widget_t {
widgetStateType state;
widgetStateType previous_state;
uint8_t tab;
int (*processInput)(widget_t*, RE_Rotation_t, RE_State_t *);
int (*longPressAction)(widget_t*);
};Code: [Select]struct comboBox_widget_t { //<-- void *content addresses this
uint8_t currentScroll;
const uint8_t* font;
comboBox_item_t *first;
comboBox_item_t *currentItem;
selectable_widget_t selectable;
};Code: [Select]struct widget_t
{
widgetType type;
widgetRefreshType refresh;
widgetFrameType frameType;
uint8_t posX;
uint8_t posY;
uint8_t width;
uint8_t enabled;
int8_t radius;
widget_t *next_widget;
struct screen_t *parent;
void (*draw)(widget_t*);
void (*update)(widget_t*);
void *content; //<-- void *content
};
Well, it looks a lot but it's not. In widget_t, use void *content to allow any type of pointer.
comboBox_widget_t includes a selectable_widget_t, And pointers to item, that are added when configuring it.
The allocating function is pretty simple. Of course the code is stripped down to show only what matters:Code: [Select]widget_t *newWidget(widgetType type){
widget_t *w=malloc(sizeof(widget_t));
if(!w) Error_Handler();
switch(type){
case widget_combo:
w->content = malloc(sizeof(comboBox_widget_t));
break;
default:
Error_Handler();
}
if(!w->content) Error_Handler();
widgetDefaultsInit(w, type);
return w;
}
The part that causes the hard fault is in widgetDefaultsInit:Code: [Select]void widgetDefaultsInit(widget_t *w, widgetType type){
if(type==widget_combo){
comboBox_widget_t *combo = (comboBox_widget_t*)w->content;
w->frameType = frame_combo;
w->draw = &comboBoxDraw;
w->parent->current_widget = w;
# These cause a hard fault! But doesn't with static allocation (Manually declaring each widget and sub-widget)
combo->first = NULL;
combo->currentItem = NULL;
combo->selectable.processInput = &comboBoxProcessInput;
combo->font = default_font;
combo->currentScroll = 0;
}
} -
Error handler better be terminal or this will cause an uninitialized access after default case falls through:
That was it! Not exactly that, but yes, I over-simplified the function and it was missing an important step.
if(!w->content) Error_Handler();
Is parent initialized before this?
w->parent->current_widget = w;
I kept looking at the malloc thing,"there must be something wrong with a pointer or a cast! ", but didn't made sense because it looked correct.
Yes, Error_Handler is terminal. It shows the file/line where the problem happened and dies in a forever loop.
Thanks!
-
The allocating function is pretty simple. Of course the code is stripped down to show only what matters:
Code: [Select]widget_t *w=malloc(sizeof(widget_t));
What programming language is that? The above code is invalid in both C and C++.
In C++ you would be required to explicity cast the result of `malloc` to `widget_t *`.
In C you would have to refer to your type as `struct widget_t`.
-
Error handler better be terminal or this will cause an uninitialized access after default case falls through:
That was it! Not exactly that, but yes, I over-simplified the function and it was missing an important step.
if(!w->content) Error_Handler();
Is parent initialized before this?
w->parent->current_widget = w;
I kept looking at the malloc thing,"there must be something wrong with a pointer or a cast! ", but didn't made sense because it looked correct.
I've noticed many people tend to run into the same issue as you did when debugging a problem (not just with code!): a tendency to focus on something they are not very confident in, rather than taking a look at the whole picture and using a methodic approach. In other words, they let prejudice rule over method.
Don't feel bad though, it's actually very common, and I've seen many threads in the same vein, with a lot of replies just making wild guesses all over the place, most of them unrelated to the issue (for lack of information), but the discussion itself is often enough for the OP to eventually find the culprit. Sharing a problem often helps us solve it by ourselves!
-
I do that a lot! It happens all the time, something right in front of your eyes and you don't see it!
It actually was very simple. Previously there was a secondary function that was called before widgetDefaultsInit which initialized the parent pointer.
I packed everything in a single funcion, but missed that part. Everything else was fine!
I decided to ask here after few hours of code checking, testing, debugging and smashing the table.
Specially smashing the table and screaming things that should never said in presence of children
-
Nope, this is syntactically correct C code.The allocating function is pretty simple. Of course the code is stripped down to show only what matters:
Code: [Select]widget_t *w=malloc(sizeof(widget_t));
What programming language is that? The above code is invalid in both C and C++.QuoteIn C you would have to refer to your type as `struct widget_t`.
You may want to learn about the "typedef" keyword.
-
Yes, there's also:Code: [Select]
typedef struct widget_t widget_t;
I also get a little confused about that.
This is two examples are same thing, right?Code: [Select]struct data_t{
uint8_t data;
uint8_t size;
};
typedef struct data_t data_t;Code: [Select]typedef struct{
uint8_t data;
uint8_t size;
}data_t;
Then:Code: [Select]data_t Mydata; -
This is two examples are same thing, right?
Yes. You can even doCode: [Select]struct data_t{
uint8_t data;
uint8_t size;
};
typedef struct data_t data_t;Code: [Select]typedef struct{
uint8_t data;
uint8_t size;
}data_t;
Then:Code: [Select]data_t Mydata;Code: [Select]typedef struct data_struct data_t;This is actually a common pattern, because then you can use data_t in the definition of struct data_struct. While the above will work fine if you replace data_struct with data_t, it can confuse some programmers (especially those coming from C++, where the struct and typedef namespaces are the same); and it will not work in C++ (again, because struct and typedef use the same namespace).
struct data_struct {
uint8_t data;
uint8_t size;
};
Unless you use packed structures, you really should reorder your structures so that the fields with largest alignment requirements are listed first. That way, the compiler needs to insert the least padding in the structure, and your structures are no larger than they need to be.
You could also design your widget types as unions within a structure containing the common fields:Code: [Select]struct widget {This struct widget would be the last defined, and contain all the widget types. The label type could be
struct widget *next;
struct screen *parent;
void (*draw)(struct widget *);
void (*update)(struct widget *);
int (*focus)(struct widget *);
int (*select)(struct widget *);
int16_t x;
int16_t y;
int16_t width;
int16_t height;
int16_t radius;
widgetStateType state; /* Visible, hidden, focused, disabled, ... */
widgetRefreshType refresh;
widgetFrameType frame;
widgetType type; /* Defines which of the unions is valid */
union {
struct anyLabel label;
struct comboBox combo;
};
};Code: [Select]struct anyLabel {where the icon and the text parts are optional.
const int8_t *icon; // Optional button icon
const int8_t *font; // Font for the text label
char *text;
int16_t iconX; // Relative coordinates for the icon
int16_t iconY;
int16_t textX; // Relative coordinates for the text label
int16_t textY;
};
Combobox select() method opens and closes the option list. Let's say we use a list of anyLabels, each with a defined size, and the list scrollable:Code: [Select]struct listItem {The comboBox-specific select() method is only called when one of the list items is chosen. The state, refresh, and frame refer to the comboBox list area.
struct listItem *next;
struct anyLabel label;
int16_t width;
int16_t height;
};
struct comboBox {
int (*select)(struct *widget, struct listItem *);
struct simpleButton label;
int16_t scroll;
widgetStateType state;
widgetRefreshType refresh;
widgetFrameType frame;
struct listItem *list;
}
Before implementing any of this in a microcontroller, I personally would DEFINITELY simulate and test the UI first, and design the widget data types based on the actual needs around that. For the simulator, I believe the Cairo library would be most appropriate; it too is written in pure C. You can either use the GTK+ widget toolkit (in Linux, MacOS, or Windows), or you can write the simulator in pure C with Cairo under Xlib (i.e., X-Windows, either in Linux, or in MacOS or Windows using an X-Windows emulation).
As you can see from the Cairo X11/Xlib example, it creates a simple window of the desired size as a Cairo surface, and the standard X11/Xlib event loop acquires pointer location and clicks, as well as keypresses (and -releases). It should fit well the microcontroller model.
(Instead of drawing to a memory buffer and sending the changes to a display, one draws to the Cairo surface, and updates the visible window with the surface contents. Different C code, yes, but logically very similar; so the control flow and data types should be almost the same in the simulator, as needed on the MCU.)
The idea of the simulator is twofold: One, it lets you experiment (in a desktop environment, thus much easier and faster than a MCU development cycle) with the actual user interface. Two, you can experiment and develop the actual drawing primitives and data types you need on the MCU to describe that user interface efficiently.
Me, I like to find unsuspecting victims to test my UIs without documentation, to see if they can find a feature by navigating the UI if they are only given a desired end result/effect. If not, the UI is not good enough. Some users are stuck in specific expectations ("I want X on the bottom", "The icons are ugly", "I don't like the colors"), so not all of their input is actionable, but observing (statistics on) what and how they traverse through the UI, tells you how end users too would try to use the UI.
Which gives one the perfect opportunity to optimize the UI to work for the expected use cases with as little effort as possible.
During the development of such UI simulators, I usually end up revamping my data structures completely. What I imagine I need at the design phase, is often what I need at the implementation phase; and since I firmly believe reality beats theory every time, I adapt my design to work for what I need it to.
I do prefer to do this stuff in the Linux environment, because it is both closest to the MCU environment (same compiler and compiler options, at least, even if a different hardware backend), and is least effort (see Cairo + Xlib above; you ensure the dev libs for these two packages are available, and if you have a desktop environment, you're ready to go). In Windows, a large problem is that Microsoft only provides a C++ compiler, not a real C one, so things you expect from a C compiler no longer apply there. You can use MingW or other GCC ports or even Intel CC, but AFAIK interfacing to the Windows GUI from C is nontrivial. On MacOS, you do need an interface layer, something like XQuartz or XDarwin. On Linux, Wayland users do need e.g. XWayland, but because of the number of X11 applications, it should already be running, unless one uses a self-built highly customized Linux distro.
(XQuartz, XWayland, XDarwin, etc. all provide a "rootless X server". It means X11 applications are drawn as standard application windows for that operating system, but their contents are updated through the X server application. This helper X server does not need to run with elevated privileges, only with the current user privileges, so it really is more like a translator application. WSL includes an automatic layer for this, so you won't even need to run the rootless X server yourself.)
If you want, I could probably whip up a small example, "simulating" say a 128×64 B/W (OLED) or 320×240 15-bit color (BuyDisplay IPS) display, on X11 and Cairo, showing how the various events would be caught by the simulator and forwarded to some basic widget types. If you intend to use a touchscreen, then mouse/pointer works, although there is no "hover" above anything, only touch/push. If you intend to use physical buttons, use the keyboard for those.
Perhaps as a separate topic, in the hopes of others creating small appliances and widgets (no pun intended) with displays would do this too. We already have enough bad user interfaces, and doing better isn't hard; in fact, it can be fun and very much worth the effort. -
Unless you use packed structures, you really should reorder your structures so that the fields with largest alignment requirements are listed first. That way, the compiler needs to insert the least padding in the structure, and your structures are no larger than they need to be.
Reminds me of Torvald’s rant on people caring about reordering fields in structs.
But I can’t completely disagree with him there. While I myself have a habit of ordering fields like that in structures, I would not recommend going insane about that. The moment you put any effort into thinking about their order, you are likely wasting your time. Or worse: falling into premature optimization. The idea seems brilliant and reasonable, until one confronts it with reality.
For platforms larger than microcontrollers, with gigabytes of RAM, it will usually bring no measurable gains. That is partially because the gains, if any, are microscopic compared to other factors, partially because of how memory allocation works. For 8-bit microcontrollers there is normally no alignment requirements and padding between fields is unexpected, no matter the order. For more-bits microcontrollers the amount of RAM is usually enough to not have to care about such a detail. Exceptions include large arrays of objects, deep stacks, pushing the limits of a microcontroller, cache optimization. But that’s not what most programming is.
Keeping track of field sizes is close to impossible for portable software. That is less relevant to microcontrollers. In particular if you an afford to write code for exactly one, specific device. But in general it diminishes the importance of fields order.
It’s not without drawbacks too. With that approach, if you extend a structure, you must insert fields between other things. That kills binary compatibility. And it breaks it in a nasty manner: silently, often not giving any obvious symptoms until you discover some of your data gets corrupted. Not an issue if the structure is used only internally by a module, but a horrible trap if it’s a part of either the public API or is exposed for caller-side optimization.
To sum it up, you are likely to spend your time on something that normally will provide no noticeable gains and may cause problems. And don’t get me wrong. I am not saying it has no uses. No, it’s a great optimization technique! But, like any other optimization, it should be used when actually needed.
Adding a bit to that topic, there is an extension of that idea in which combine multiples (typically pairs) of data instances into a single structure to decrease waste from padding. Depending on the size of the data and circumstances, that may be either using arrays of fields…Code: [Select]struct Data2 {… or gluing structures back-to-back…
double foo[2];
uint_fast64_t bar[2];
void* ptr[2];
char ch[4][2];
};Code: [Select]struct Data2 {The latter makes sense for larger structures, if locality of reference is important. The cost, aside from more expensive maintenance and higher chances of introducing bugs, is larger code, which by itself has negative impact on both memory usage and performance.
double foo1;
uint_fast64_t bar1;
void* ptr1;
char ch1[4];
char ch2[4];
void* ptr2;
uint_fast64_t bar2
double foo2;
};
-
The moment you put any effort into thinking about their order, you are likely wasting your time.
False, at least when using unions or polymorphic structures, which is the case here.
I do agree that the moment you use only a rule of thumb to reorder the fields of your structure, you are wasting time.
However, the order of structure members is important, no matter what anyone thinks. The C compilers are not allowed to reorder the fields, nor add padding preceding the very first member, and the standards specify quite strict rules on the padding between members (that really depend on the necessary alignment, which was not available prior to C11 _Alignof operator came along).
I can show several real-world examples exactly why it matters. Of course, the entire picture cannot be condensed down to any simple rules of thumb, because there are competing interests, but it definitely matters. Doing it right is not waste of time.
My classic example is binary trees. Just about every example you see puts the payload first. That is the stupid thing to do. If you put the common structure pointers first, you can use a single set of functions to support basically any type:Code: [Select]/* Tell the compiler we'll be declaring struct node later on. */Because the int_data and double_data structures have common initial members, C99 6.5.2.3p5 says those initial members can be accessed via any of the unions, as long as at least one of the unions is valid. (The any node structure is not necessary at all, but is useful for us humans to use when we don't know or don't care about the actual node type; it makes code using these structures easier to read for us humans. The compiler does not care if we use any or i or d.)
struct node;
typedef signed char node_type;
/* Enumerated constants for node_type. */
enum {
TYPE_DEAD = -1,
TYPE_NONE = 0,
TYPE_INT,
TYPE_DOUBLE,
};
struct int_node {
struct node *le;
struct node *gt;
node_type type; /* = TYPE_INT */
unsigned char flag;
int value;
};
struct double_node {
struct node *le;
struct node *gt;
node_type type; /* = TYPE_DOUBLE */
unsigned char flag;
double value;
};
/* Completely optional, only useful to help humans write easier code */
struct any_node {
struct node *le;
struct node *gt;
signed char type;
unsigned char flag;
};
struct node {
union {
struct any_node any;
struct int_node i;
struct double_node d;
};
};
For example, the following functions will always find the smallest and largest nodes, respectively, in a binary search tree, regardless of the actual node types:Code: [Select]struct node *smallest_node(struct node *root)The type indicates the type of a particular node. I like to use a signed integer type for this, with negative values indicating various errors. Since I rarely have 127 or more node types, I often make it a signed char or int8_t. On most architectures, this means that at least the next byte will be padding, so I like to use that for an internal flag, used for example to detect loops when emitting the tree structure in Graphviz Dot language.
{
while (root->any.le)
root = root->any.le;
return root;
}
struct node *largest_node(struct node *root)
{
while (root->any.gt)
root = root->any.gt;
return root;
}
Here is an example (untested) for how to emit the above tree in Dot:Code: [Select]unsigned char flag = 0; /* Assigned at initialization */
static int dot_tree_recurse(struct node *node, FILE *out)
{
int ret = 0;
/* Already visited? */
if (node->any.flag == flag)
return 0;
else
node->any.flag = flag;
fprintf(out, "\t\"%p\" [ label=\"", (void *)node);
switch (node->any.type) {
case NODE_NONE:
fprintf(out, "(None)");
break;
case NODE_INT:
fprintf(out, "int: %d", node->i.value);
break;
case NODE_DOUBLE:
fprintf(out, "double: %.6f", node->d.value);
break;
default:
fprintf(out, "(Bad type: %d)\" ];\n", node->any.type);
return node->any.type;
}
fprintf(out, "\" ];\n");
if (node->any.le) {
int temp = dot_tree_recurse(node->any.le, out);
if (!ret)
ret = temp;
fprintf(out, "\t\"%p\" -> \"%p\" [ taillabel=\"<=\" ];\n", (void *)node, (void *)(node->any.le);
}
if (node->any.gt) {
int temp = dot_tree_recurse(node->any.gt, out);
if (!ret)
ret = temp;
fprintf(out, "\t\"%p\" -> \"%p\" [ taillabel=\">\" ];\n", (void *)node, (void *)(node->any.gt);
}
return ret;
}
int dot_tree(struct node *root, FILE *out)
{
int ret = 0;
fprintf(out, "digraph {\n");
if (root) {
++flag;
ret = dot_tree_recursive(root, out);
}
fprintf(out, "}\n");
fflush(out);
if (ret)
return ret;
if (ferror(out))
return -1;
return 0;
}
Now, in the above struct node structure, there is likely padding between flag and value, and that's fine. The important bit is that the initial members of the structures of the union have to match. Among these initially shared members, we do want to put the pointers first, because they need the biggest alignment anyway, and are the most used members. (The address of the first element in a structure is always the address of the structure; there is no leading padding allowed in C; and so on.)
So, the size or alignment of the structure fields should not be something you should spend much time considering; but considering the order of those fields, especially if shared in an union, the order of initially matching fields, can make a big difference in how much code you need to write.
However, even if you do not use an union now, are you sure the structure won't be polymorphic in the future? I say it is better to adopt a routine structure member ordering scheme, where you group the members based on their logical use cases, and within the group, prefer to put pointers first, then other members in approximate field size order. When a pointer points to an array with its size (count) in a separate member, I like to put the size/count next to the pointer, since it makes it easier for us humans to see they're associated. That logical reason overrides any alignment/padding gains, in my opinion.
Elsewhere, I've shown how to use the preprocessor to simulate simple C++ templates, so that you can use the same source file to define functions that only differ by the types of certain fields or variables. For the above, for any defined numeric value types, for example. The fact that it is possible, does not make it that useful, especially on embedded architectures, where the associated code bloat can be a hindrance. The neglible speed and node size differences may make it useful in some corner cases, but that's part of final optimization rounds, not something to worry about during main development.
Copy-pasting the same code and tweaking it just a bit to match the need is a horrible practice, because if there is a bug in the code you copied from – and there likely is, as we're only human – you need to check and fix it in all derivatives; and usually at least one is forgotten. And the next dev that comes along, starts wondering why the similarly-looking code differs in that exact spot, with no hint whether the difference is intentional or a bug. Adding a nice quirk in one variant does not magically appear in the others, so such copy-paste code usually diverges and becomes hellishly annoying to maintain: because of their similarities, we humans tend to feel they should be the same, and will have problems if they aren't. We are pattern detectors at the core, after all.
I still like my simulator approach better than implementing this stuff directly on the target MCU.
When implementing the UI on the simulator, all of the above tends to pop out during the development anyway; and if you aren't already worrying about getting it to just work damnit, it is much easier to rewrite the data structures to work better in a comfortable, easy, fast simulator environment. -
I've tested that in the past, might depend on being Big or small endian?
In stm32, putting the smaller first is what saves space. -
Nominal Animal: I was responding to your earlier post and I have quoted the relevant fragment. That was about reordering for the purpose of saving space. Creating compatible types is an unrelated concept, which I was not even touching.
DavidAlfa: endinaness determines byte order within a multi-byte integer. I never worked with STM32, but padding at the begining would be at least weird. It would entail that pointer arithmetic works upside-down, including strings being stored from the last to the first byte. How have you came to your conclusion? Any specific struct example?
-
I've tested that in the past, might depend on being Big or small endian?
In stm32, putting the smaller first is what saves space.
I suppose you're replying to the idea of ordering struct members to minimize padding?
I don't see how endianness could play any role here? And anyway, almost all common targets these days are little endian.
The reason for what Nominal Animal said is simple. It's akin to filling a container with stones of various sizes. You'll get the most stones in if you fill it with the bigger ones first. The smaller ones will then be able to get through. Not sure this image fits the problem completely, but I like it.
A small example, let's imagine you have 4 struct members like this:Code: [Select]uint32_t n1;
uint8_t n2;
uint16_t n3;
uint32_t n4;
Let's assume a 32-bit integer must be 32-bit aligned, a 16-bit integer 16-bit aligned, and 8-bit integer 8-bit aligned.
Ordered in the above way, the offsets will be the following:
n1: 0
n2: 4
n3: 6
n4: 8
=> struct size = 12 bytes
Now let's reorder with the larger alignments first:Code: [Select]uint32_t n1;
uint32_t n4;
uint16_t n3;
uint8_t n2;
The offsets will now be the following:
n1: 0
n4: 4
n3: 8
n2: 10
=> struct size = 11 bytes
Now let's reorder with the smaller alignments first:Code: [Select]uint8_t n2;
uint16_t n3;
uint32_t n1;
uint32_t n4;
The offsets will now be the following:
n2: 0
n3: 2
n1: 4
n4: 8
=> struct size = 12 bytes
The larger the number of members, the larger the size difference will be.