Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UB due to incompatible function pointer types #2

Closed
Hirrolot opened this issue Jun 25, 2021 · 5 comments
Closed

UB due to incompatible function pointer types #2

Hirrolot opened this issue Jun 25, 2021 · 5 comments

Comments

@Hirrolot
Copy link

Hirrolot commented Jun 25, 2021

Interface function implementations accept T * as a first parameter, whereas a corresponding function in a virtual table accepts void *, thereby making these two function types incompatible (godbolt).

Moreover (6.2.7 Compatible type and composite type):

All declarations that refer to the same object or function shall have compatible type; otherwise, the behavior is undefined.

And I am curious about this code:

TypeclassName T_to_TypeclassName(T* x)
{
    static TypeclassName_vtable const tc = {.func_name = (ReturnType (*const)(void*, ...))(T_func_name) };
    return (TypeclassName){.tc = &tc, .self = x};
}

Here, .func_name and T_func_name are two declarations to the same function having incompatible types. Then we cast T_func_name to the type accepting void * as a first parameter; we cast incompatible types. Does it even follow the standard, or it is UB?

The C99 draft standard: http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf

@Hirrolot
Copy link
Author

Yes, it is undefined behaviour: https://stackoverflow.com/questions/559581/casting-a-function-pointer-to-another-type.

How can then we get around this except for accepting void *self always and casting it to T * in every interface function implementation?

@Hirrolot Hirrolot changed the title An issue with compatible function types UB due to incompatible function pointer types Jun 25, 2021
@TotallyNotChase
Copy link
Owner

Very valid point. Regarding the warning about incompatible types, that is indeed a warning I'd expect - to make sure the user is using the "correct" function type. However the types in the interface and the type asked from the user are certainly formally incompatible. Though I believe you were looking for this excerpt-

§ 6.7.6.3
For two function types to be compatible, both shall specify compatible return types. Moreover, the parameter type lists, if both are present, shall agree in the number of parameters and in use of the ellipsis terminator; corresponding parameters shall have compatible types.

I think 6.2.7 is referring to variable declarations (of the same name) having different types- https://stackoverflow.com/a/57070607/10305477

Regardless, it's correct that the function types are incompatible since void* isn't compatible with the concrete T * type. Though in practice, due to the special casing of explicit void* casting (which in turn implicitly asks an implementation to make void* the same size and alignment as other object pointers - but not necessarily function pointers, at least that's my intuition), it is likely that this may not invoke different behaviors across implementations.

§ 6.3.2.3
A pointer to void may be converted to or from a pointer to any object type. A pointer to any object type may be converted to a pointer to void and back again; the result shall compare equal to the original pointer.

In general, the casting itself is perfectly allowed, calling it is UB (which this design does), as you've already noted-

§ 6.3.2.3
A pointer to a function of one type may be converted to a pointer to a function of another type and back again; the result shall compare equal to the original pointer. If a converted pointer is used to call a function whose type is not compatible with the referenced type, the behavior is undefined.

This feels rather muddy, in the context of what the standard mandates and what may actually happen in most (if not all) implementations. On one hand, I think the design needs some way to have user facing type safety - on the other hand, I can't think of a way to perform the cast back due to the generic nature of the function call.

This needs to be noted in the design doc itself - I'll add that.

@TotallyNotChase TotallyNotChase pinned this issue Jun 25, 2021
@Hirrolot
Copy link
Author

Hirrolot commented Jun 25, 2021

Though in practice, due to the special casing of explicit void* casting (which in turn implicitly asks an implementation to make void* the same size and alignment as other object pointers - but not necessarily function pointers, at least that's my intuition), it is likely that this may not invoke different behaviors across implementations.

Sure, most implementations work in the same way here. However, in the above SO link, people suggested some platforms on which this trick doesn't work (OpenWatcom, Emscripten LLVM to Javascript, etc.), and moreover, it implies that even if it would be fixed, it can emerge on other platforms in future as well, and it would be cumbersome to figure out what's wrong with the code. Sometimes it is really unfortunate that the standard paper and the real world are two different things.

@TotallyNotChase
Copy link
Owner

I noticed there's a naïve solution to this. The user provided function could be wrapped in another function that accepts void* instead of T*. This function can then call the user provided function, doing the usual implicit void* conversion on self. This is more boiler plate on the typeclass definer side - but not the implementer side. Which may be viable given that type safety and standard compliance are simultaneously achieved.

Something like this is what the impl_show macro could look like-

#define impl_show(T, Name, show_f)                                                                                     \
    static inline char* CONCAT(show_f, __)(void* self)                                                                 \
    {                                                                                                                  \
        char* (*const show_)(T* self) = (show_f);                                                                      \
        (void)show_;                                                                                                   \
        return show_f(self);                                                                                           \
    }                                                                                                                  \
    Show Name(T* x)                                                                                                    \
    {                                                                                                                  \
        static ShowTC const tc = { .show = (CONCAT(show_f, __)) };                                                     \
        return (Show){ .tc = &tc, .self = x };                                                                         \
    }

One wrapper function would be needed for every typeclass function, the type checking is now moved inside the wrapper functions. A whole lot of boilerplate for the definer. But hopefully a good experience for the implementer.

I implemented these changes in the ub-fix branch. I'd appreciate it if you could check it out as I may have overlooked something.

Going back to the disadvantage - even more boilerplate for the definer. I think if information about the typeclass functions are persisted in an arg list-esque structure - meta macros could potentially be used to automate the writing of those wrapper functions. One concern that came across to me immediately, though, is that void returning functions will need special treatment, as the wrapper function for those cannot return an expression. I'm no macro wizard, but I'd like to try and see if automating the wrapper defining is doable. Probably won't be doing it in a generally and flexibly usable way though, just out of curiosity.

@Hirrolot
Copy link
Author

Hirrolot commented Jun 26, 2021

The approach you're suggesting works but, as you've mentioned, it has a drawback concerning boilerplate on the definer's side (it already has a lot of boilerplate). It could be eliminated with meta-macros but really shouldn't because an interface function definition would look like this (macros need parentheses and commas to distinguish syntactical elements):

iFn(void, set, (void *, self), (int,  x));

instead of this:

iFn(void, set, void *self, int x);

Moreover, the case with void as a return type should be handled and it cannot be determined solely by the preprocessor, so we need even more syntactic forms:

iFnVoid(set, (void *, self), (int, x));

All this machinery complicates everything, from maintaining to the learning curve, and furthermore, looks less natural to C. My work on Poica has shown that this is a stillborn approach.

Your branch ub-fix looks fine. I still don't lose hope that it can be somehow fixed via type system punning (in this case, I must go and read the standard a couple of days). If you want, you can experiment with Metalang99 to accomplish automatically the proposed solution with a wrapper function, but to be honest, I hardly believe that It would look natural to C. If I come up with something through type punning, I'll inform you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants