views:

130

answers:

4

Today I stumbled over a piece of code that looked horrifying to me. The pieces was chattered in different files, I have tried write the gist of it in a simple test case below. The code base is routinely scanned with FlexeLint on a daily basis, but this construct has been laying in the code since 2004.

The thing is that a function implemented with a parameter passing using references is called as a function with a parameter passing using pointers...due to a function cast. The construct has worked since 2004 on Irix and now when porting it actually do work on Linux/gcc too.

My question now. Is this a construct one can trust? I can understand if compiler constructors implement the reference passing as it was a pointer, but is it reliable? Are there hidden risks?

Should I change the fref(..) to use pointers and risk braking anything in the process?

What do you think?

Edit

In the actual code both fptr(..) and fref(..) use the same struct - changed code below to reflect this better.

#include <iostream>
#include <string.h>

using namespace std;

// ----------------------------------------
// This will be passed as a reference in fref(..)

struct string_struct {
    char str[256];
};

// ----------------------------------------
// Using pointer here!

void fptr(string_struct *str) 
{
    cout << "fptr: " << str->str << endl;
}

// ----------------------------------------
// Using reference here!

void fref(string_struct &str) 
{
    cout << "fref: " << str.str << endl;
}

// ----------------------------------------
// Cast to f(const char*) and call with pointer

void ftest(void (*fin)()) 
{
    string_struct str;
    void (*fcall)(void*) = (void(*)(void*))fin;
    strcpy(str.str, "Hello!");
    fcall(&str);
}

// ----------------------------------------
// Let's go for a test

int main() {
    ftest((void (*)())fptr); // test with fptr that's using pointer 
    ftest((void (*)())fref); // test with fref that's using reference
    return 0;
}
+1  A: 

It works by pure chance.

fptr expects a const char * while fref expects a string_struct &.

The struct string_struct have the same memory layout as the const char * since it only contains a 256 bytes char array, and does not have any virtual members.

In c++, call by reference e.g. string_struct & is implemented by passing a hidden pointer to the reference so on the call stack it will be the same as if it was passed as a true pointer.

But if the structure string_struct changes, everything will break so the code is not considered safe at all. Also it is dependent on compiler implementation.

Ernelli
+1  A: 

Let's just agree that this is very ugly and you're going to change that code. With the cast you promise that you make sure the types match and they clearly don't. At least get rid of the C-style cast.

Eddy Pronk
+4  A: 

What to you think?

Clean it up. That's undefined behavior and thus a bomb which might blow up anytime. A new platform or compiler version (or moon phase, for that matter) could trip it.

Of course, I don't know what the real code looks like, but from your simplified version it seems that the easiest way would be to give string_struct an implicit constructor taking a const char*, templatize ftest() on the function pointer argument, and remove all the casts involved.

sbi
+1  A: 

It's obviously a horrible technique, and formally it's undefined behaviour and a serious error to call a function through an incompatible type, but it should "work" in practice on a normal system.

At the machine level, a reference and a pointer have exactly the same representation; they are both just the address of something. I would fully expect that fptr and fref compile to exactly the same thing, instruction for instruction, on any computer you could get your hands on. A reference in this context can simply be thought of as syntactic sugar; a pointer that is auto-dereferenced for you. At the machine level they are exactly the same. Obviously there might be some obscure and/or defunct platforms where that might not be the case, but generally speaking that's true 99% of the time.

Furthermore, on most common platforms, all object pointers have the same representation, as do all function pointers. What you've done really isn't all that different from calling a function expecting an int through a type taking a long, on a platform where those types have the same width. It's formally illegal, and all but guaranteed to work.

It can even be inferred from the definition of malloc that all object pointers have the same representation; I can malloc a huge chunk of memory, and stick any (C-style) object I like there. Since malloc only returned one value, but that memory can be reused for any object type I like, it's hard to see how different object pointers could reasonably use different representations, unless the compiler was maintaining an big set of value-representation mappings for every possible type.

void *p = malloc(100000);
foo  *f =  (foo*)p;  *f = some_foo;  
bar  *b =  (bar*)p;  *b = some_bar;
baz  *z =  (baz*)p;  *z = some_baz; 
quux *q =  (quux*)p; *q = some_quux;  

(The ugly casts are necessary in C++). The above is required to work. So while I don't think it is formally required that afterwards memcmp(f, b) == memcmp(z, q) == memcmp(f, q) == 0, but it's hard to imagine a sane implementation that could make those false.

That being said, don't do this!

janks
For some reason that got double-posted. Someone please help me delete one of them
janks
I am inclined to give you the "right answer" because I like your analogy with malloc and that on a usage level it is we the developers that decide what is considered "right". But, still, even though this works here and now and you might be right that it will work on "any computer I can get my hands on", I have a hard time believing a statement like that without evidence.I think Ernelli puts it more a little more believable `It works by pure chance`. It's not defined to be implemented as such but by chance it has been and most often are...
epatel