Add workaround for the const-or-not user_context issue (#635) #7291
Conversation
This is subtle: for historical reasons, Halide-generated AOT code defines user_context as `void const*`, but expects all define_extern code with user_context usage to use `void *`. This usually isn't an issue, but turns into one if both the caller and callee of the define_extern() use the C++ backend, *and* a user_context is passed, *and* c_plus_plus_name_mangling is enabled, in which case we get link errors because of this dichotomy. Fixing this "correctly" (ie so that everything always uses identical types for user_context in all cases) will require a *lot* of downstream churn, so this is an ugly workaround: If this is an external function, and it's a C++ implementation, and it uses user_context, then add a wrapper with `void*` ucon -> `void const*` ucon. In most cases this will be ignored (and probably dead-stripped), but in these cases it's critical. (Note that we don't check to see if c_plus_plus_name_mangling is enabled, since that would have to be done on the caller side, and this is purely a callee-side fix.)
|
Looks like this is finally green (and also looks good inside Google).... PTAL |
|
I'm ok with the code changes but I'd prefer if Zalman approved it because I'm not following the exact scenario that's giving you trouble. Is it calling a halide stage as an extern stage in another halide pipeline, but with the c backend and c++ mangling on? |
Yes, except it affects the LLVM backend too. (I am gobsmacked that we apparently never tested that scenario anyway -- even inside Google -- but that is indeed apparently the case.) |
src/CodeGen_C.cpp
Outdated
| @@ -1487,7 +1487,7 @@ class ExternCallPrototypes : public IRGraphVisitor { | |||
| } | |||
|
|
|||
| bool has_c_declarations() const { | |||
| return !c_externs.empty(); | |||
| return !c_externs.empty() || !destructors.empty(); | |||
There was a problem hiding this comment.
This looks like a general fix outside of what this PR is doing. If so, worth a separate PR?
There was a problem hiding this comment.
Well, maybe -- it's a drive-by that was needed only because the changes to the nested_externs test caused a call to halide_device_free_as_destructor to be injected when building under host-cuda, but it failed to compile because the call in question had no prototype. I can try to make minor changes to pull this out separately.
steven-johnson
changed the title
…alide#7291) Add a workaround for the const-or-not user_context issue (halide#635)
For historical reasons, Halide-generated AOT code defines user_context as
void const*, but expects all define_extern code with user_context usage to usevoid *. This usually isn't an issue, but turns into one ifboth the caller and callee of the define_extern() use the C++ backend, anda user_context is passed, and c_plus_plus_name_mangling is enabled, in which case we get link errors because of this dichotomy. Fixing this "correctly" (ie so that everything always uses identical types for user_context in all cases) will require a lot of downstream churn, so this is an ugly workaround:If this is an external function,
and it's a C++ implementation,and it uses user_context, then add a wrapper withvoid*ucon ->void const*ucon. In most cases this will be ignored (and probably dead-stripped), but in these cases it's critical.(Note that we don't check to see if c_plus_plus_name_mangling is enabled, since that would have to be done on the caller side, and this is purely a callee-side fix.)
EDIT: amazingly, this is broken in the LLVM backend as well -- I find it extremely hard to believe that no one has ever attempted to wire an AOT-Halide-filter-with-ucon directly to
define_extern()with c_plus_plus_name_mangling enabled before, but apparently that must be the case. Updating this PR to include a similar shim in the LLVM backend as well.