Non-executing code block freezes, but works if block is in { } braces

[Jeff Rowberg]

I am running into a problem which seems to have to do with the code execution pointer, and the behavior makes zero sense at all. There is a conditional statement which doesn't match, and since the code it controls is only one line, I omitted the surrounding { } braces for brevity. This is a pattern that I have reused many other times all throughout this fairly complex project without any observable bad consequences.

However, when I let the code run this way, execution apparently stops at that line. Here is the code block:

Code: [Select]


/**
 * @brief iWRAP "NAME" event handler
 * @param[in] mac MAC address of remote device
 * @param[in] friendly_name Null-ternimated device name string
 */
void my_iwrap_evt_name(const iwrap_address_t *mac, const char *friendly_name) {
    uint8_t name_len = 0;
    if (friendly_name) name_len = strlen(friendly_name);

    uint8_t payload[13 + name_len];
    payload[0] = mac -> address[5];
    payload[1] = mac -> address[4];
    payload[2] = mac -> address[3];
    payload[3] = mac -> address[2];
    payload[4] = mac -> address[1];
    payload[5] = mac -> address[0];
    payload[6] = 0;
    payload[7] = 0;
    payload[8] = 0;
    payload[9] = 0; // RSSI
    payload[10] = 4; // inquiry status
    payload[11] = find_pairing_from_mac(mac);
    if (name_len) memcpy(payload + 13, friendly_name, name_len);

    // send kg_evt_bluetooth_inquiry_response(...) event
    skipPacket = 0;
    if (kg_evt_bluetooth_inquiry_response) skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);
    if (!skipPacket) send_keyglove_packet(KG_PACKET_TYPE_EVENT, 13 + name_len, KG_PACKET_CLASS_BLUETOOTH, KG_PACKET_ID_EVT_BLUETOOTH_INQUIRY_RESPONSE, payload);
}


The offending line of code is this:

Code: [Select]


    if (kg_evt_bluetooth_inquiry_response) skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);


Note that "kg_evt_bluetooth_inquiry_response" function pointer which is currently NULL, so this condition doesn't match. Now, if I modify this one line of code to include braces:

Code: [Select]


    if (kg_evt_bluetooth_inquiry_response) {
        skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);
    }


Suddenly, everything works. There is NO reason I can think of why this should be occurring. Is there some weird Arduino IDE Java -> C++ conversion that is messing with the code blocks before compiling? Can I check this somehow?

I have also noticed that if I add a log output command (not shown here) earlier inside the function, in a way that is logically unrelated (and separated by many lines of code) from the condition shown here, the problem also goes away even if I don't add braces. I suspect some weird memory leak in my code or a buffer overflow or something, but I have no way of doing low-level debugging with breakpoints, watches, etc.

I am using Arduino v1.0.6 with a Teensy++ 2.0 (AT90USB1286) and Teensyduino v1.20 (latest as of this moment).

[AWOL]

Condition or assignment?
Did you mean ==?

[econjack]

In your statement:

Code: [Select]


 if (kg_evt_bluetooth_inquiry_response) {
        skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);
    }


what do you think the function call name in the sub-expression:

Code: [Select]


  if(kg_evt_bluetooth_inquiry_response)


does?

[Jeff Rowberg]

Condition or assignment?
Did you mean ==?

The "skipPacket" variable should be assigned if the condition matches and the function is executed, but in this case there should be only the test for a non-null function pointer, which should fail since the function pointer is null.

In your statement:

Code: [Select]


 if (kg_evt_bluetooth_inquiry_response) {
        skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);
    }


what do you think the function call name in the sub-expression:

Code: [Select]


  if(kg_evt_bluetooth_inquiry_response)


does?

The condition is meant to check whether the function pointer has been assigned to an actual function or not. The function pointer is declared like this, but not initialized:

Code: [Select]


uint8_t (*kg_evt_bluetooth_inquiry_response)(uint8_t *address, uint8_t *cod, int8_t rssi, uint8_t status, uint8_t index, uint8_t name_len, uint8_t *name_data);


In this circumstance, the non-dereferenced "kg_evt_bluetooth_inquiry_response" pointer is null, so "if (null)" evaluates to false (or it should). This kind of conditional check for assigned vs. unassigned function pointers as optionally defined callbacks is present all over the place in the code, and they appear to work fine--with this one exception.

[AWOL]

The "skipPacket" variable should be assigned if the condition matches

Which condition?

Code: [Select]

if (kg_evt_bluetooth_inquiry_response) skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);
Here, "kg_evt_bluetooth_inquiry_response" is called unconditionally, the return value assigned to "skipPacket" and the result of that assignment tested.
However the semicolon at the end means no decision is taken as a result of that test.

[Jeff Rowberg]

Which condition?

This one:

Code: [Select]


if (kg_evt_bluetooth_inquiry_response) skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);


Translated into English:

"If the kg_evt_bluetooth_inquiry_response function pointer is non-null--in other words, if it has been set to point to a real function instead--then run the kg_evt_bluetooth_inquiry_response function with the supplied arguments, and store the return value in skipPacket."

The skipPacket variable is declared and initialized to 0 one line above this, so it starts with a known value. But it should not be modified regardless, since the "if (kg_evt_bluetooth_inquiry_response)" condition should evaluate as false, and the line of code the subsequently would call that function should not be executed.

My understanding of function pointers (and the rest of the code and other libraries I have written) suggests that if you have a function pointer declared like this:

Code: [Select]


uint8_t (*my_function_pointer)(uint8_t arg1, uint8_t arg2);


...and then you use only the pointer variable name without a parameter list like this:

Code: [Select]


if (my_function_pointer != null) { ... }


...it tests the pointer's assigned memory address only, and does not actually call the function (which is what I want here). On the other hand, if you include the parameter list:

Code: [Select]


if (my_function_pointer(10, 20) != null) { ... }


...then it will dereference the pointer and execute the real function that it is pointing to, with the supplied arguments, and then the conditional statement will evaluate the return value rather than the function pointer's memory address.

[AWOL]

Translated into English:

"If the kg_evt_bluetooth_inquiry_response function pointer is non-null--in other words, if it has been set to point to a real function instead--then run the kg_evt_bluetooth_inquiry_response function with the supplied arguments, and store the return value in skipPacket."

That's not how to translate that code into English.
You're calling the function whether or not the pointer to it is NULL.

Code: [Select]

if (fn && (result = fn(paramList)))
{
  doThis();
}
Tests the function pointer and if it is not NULL, calls the function and assigns the value returned by the function to the variable "result".
If the value of "result" is non-zero, the function "doThis" will be executed.

[Vaclav]

Until "notify me" is fixed.
I know this should not matter, but have you actually tried to compare to null?
I am not questioning you, but does the process stops at the first if(...) or executes the next if(!...)?
Just asking.

[PaulS]

I think it's time for you to post ALL of your code, so that we can try to reproduce the problem.

[KenF]

I'm wondering if you expressly initialised kg_evt_bluetooth_inquiry_response to be null when you declared it.  Such as  void (*kg_evt_bluetooth_inquiry_response)()=NULL; 

But we could all make guesses until the cows come home.  Unless you actually post your code, we're all just fumbling around in the dark.

[Jeff Rowberg]

You're calling the function whether or not the pointer to it is NULL.

I follow the example you give, but this statement above doesn't make sense in light of my code or your example. If that were true given the syntax I have in my code, then I would have a gigantic mess of runtime errors all over the place in this codebase and others where attempts to check for function pointer assignment are in fact dereferencing null function pointers and trying to call them. But that is demonstrably not what happens.

These examples from elsewhere online follow my understanding:

• http://www.newty.de/fpt/fpt.html#compare
• http://stackoverflow.com/questions/142789/what-is-a-callback-in-c-and-how-are-they-implemented#comment45692_142809

In short:

• Using a function pointer without a parameter list gives you the pointer address.
• Using a function pointer with a parameter list executes the function.

If my code were actually calling the function pointer in the conditional statement, then your code would be doing so twice. In your example (emphasis mine):

Code: [Select]

if (fn && (result = fn(paramList)))
{
  doThis();
}
Tests the function pointer and if it is not NULL, calls the function and assigns the value returned by the function to the variable "result". If the value of "result" is non-zero, the function "doThis" will be executed.

That's exactly what I'm saying. The condition "fn && (result = fn(paramList))" will first check to make sure the pointer is not null, and then if it is not null the second half of the logically AND'ed condition will be evaluated (i.e. "fn(paramList)" will run and have its return value stored in the "result" variable).

But I don't want to evaluate the return value inside my if statement's condition; I want it to be run as a result of the condition. In contrast, my code is equivalent to this mutation of your example:

Code: [Select]

if (fn)
{
  result = fn(paramList);
}

Presumably, this should test the function pointer to ensure it is not null first; then, if it is not null, call the function and store the return value. This is what I am going for. Visually rearranging my code above, it's the same:

Code: [Select]

if (kg_evt_bluetooth_inquiry_response)
{
  skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);
}

Except, I left the braces off because it's one line:

Code: [Select]

if (kg_evt_bluetooth_inquiry_response)
    skipPacket = kg_evt_bluetooth_inquiry_response(payload + 0, payload + 6, payload[9], payload[10], payload[11], payload[12], name_len ? payload + 13 : 0);

Is there anything wrong with this?

I know this should not matter, but have you actually tried to compare to null?

Good question; I have not tried this yet, though it has not been necessary in other places. Using "if (kg_evt_bluetooth_inquiry_response)" should be logically the same as "if (kg_evt_bluetooth_inquiry_response != 0)" as far as I know, but it is worth trying just to be sure.

The challenge here, and why I suspect a memory leak or problem somewhere else in my code, is that seemingly irrelevant modifications suddenly cause this to work again (e.g. adding a debug output line). This is why I don't think it's necessarily due to this specific block of code, but at the same time I don't know where to look.

I think it's time for you to post ALL of your code, so that we can try to reproduce the problem.

No problem as far as I'm concerned, but it's a large file in a project with dozens of others, some of which rely on additional hardware outside the Teensy++ 2.0 board (specifically a Bluegiga WT12 module in this particular case). Reproducing the behavior externally may be difficult. The source file in question is here, on Github:

• https://github.com/jrowberg/keyglove/blob/master/keyglove/support_bluetooth2_iwrap.h#L1105

The full project is on the same Github repo:

• https://github.com/jrowberg/keyglove/tree/master/keyglove

I'm wondering if you expressly initialised kg_evt_bluetooth_inquiry_response to be null when you declared it.  Such as  void (*kg_evt_bluetooth_inquiry_response)()=NULL;

I did not, but I can try that as well.

However, the seemingly inexplicable fact remains that the exact same code works with braces around the (non-matching!) conditional code block, and doesn't work when the braces are gone, despite the conditional code being exactly one line.

And, similarly, the same code works (without braces in both cases) when a debug output line is present a dozen lines up, while it locks up without the debug line present.

[AWOL]

If my code were actually calling the function pointer in the conditional statement, then your code would be doing so twice.

Nope.

Code: [Select]

if (fn)
does not call the function, even once, it merely tests the function pointer to decide if it is or is not null.

Code: [Select]

if (fn && fn (parameterList))
does not call the function iff "fn" is null.
It doesn't check to see if "fn" is a valid pointer, only that it is or is not null.
The function can only be called once if the pointer is not null

[Jeff Rowberg]

Code: [Select]

if (fn)
does not call the function, even once, it merely tests the function pointer to decide if it is or is not null.

Code: [Select]

if (fn && fn (parameterList))
does not call the function iff "fn" is null.
It doesn't check to see if "fn" is a valid pointer, only that it is or is not null.

I think we are talking past each other. This is exactly what I was saying about my original code example, and exactly how I want it to work and how it does work in every one of the dozens of cases where I have this kind of code, except this one. Which is why I am pulling my hair out.

I do realize that the "if (fn)" condition does not validate the memory address if one is assigned, but only checks for null vs. non-null. However, adding braces around the conditional block should have zero impact on whether the function pointer being tested contains null or not, yet this what seems to be occurring (although I know it cannot be, which is why I wish I had hardware-level debug access).

[Jeff Rowberg]

Also, to test, I have just tried adding explicit zero initializations:

Code: [Select]


uint8_t (*func_pointer)() = 0;


...and explicit non-zero conditional checks:

Code: [Select]


if (func_pointer != 0) ...


...and explicit braces around the single-line conditional blocks:

Code: [Select]


if (func_pointer != 0) { ... }


...and the Arduino compiler reports the exact same flash and RAM usage before and after this modification. I will leave the code in for slightly better (?) explicit readability and try testing with it as well, but it seems that the compiler is optimizing these explicit bits out, or at least not optimizing any differently than before they were added.

[AWOL]

Post code - this is all just hand-waving.
(You can't put code formatting inside code tags)