turran wrote:
> > As I mentioned before, keep in mind that the GLib maintainers have a strict
> > stance on the potential changes to address this situation. Check
> > https://gitlab.gnome.org/GNOME/glib/-/blob/main/docs/toolchain-requirements.md?ref_type=heads#function-pointer-conversions.
> > They state that the toolchain must handle function pointer mismatch; any
> > other attempt will be discarded, as it already happens to us in a MR we
> > opened.
>
> There is an important difference between GLib changes which remove function
> pointer mismatches, and changes which would allow those mismatches to
> continue to be used but make them more explicit. The former would be a
> complete architectural change, whereas the latter would not, and the
> toolchain would still handle function pointer mismatches, albeit explicitly
> instead of silently. That doesn't necessarily mean that GLib would accept
> them of course (maybe they just really don't want to have to think about that
> particular form of UB even a little bit). But the distinction is IMO
> important, and it's still helpful for thinking about this problem.
>
> > > And it appears that this patch is not sufficient to make glib
> > > transparently work. Mostly because there are plenty of casts in glib that
> > > are actually runtime casts and not compile-time casts. For example in
> > > [gslist.c](https://github.com/dschuff/glib/commit/f9fd9acd4213e67eebd71dd74900cfb360ea2ddf#diff-48030e2aa9b604d2a85bf174ef9ea31ef32d5341128d6317544d88a2551f82bbL1032-R1039)
> > > the list iteration functions are calling callbacks that are either
> > > 2-argument `GCompareFunc` or 3-argument `GCompareDataFunc`, but it can't
> > > tell statically which one it is; the cast is a runtime cast. LIkewise
> > > [g_queue_foreach](https://github.com/dschuff/glib/commit/f9fd9acd4213e67eebd71dd74900cfb360ea2ddf#diff-3196da21408e1f1c02f7923adfdd6680077d395feeeceff925665e711f4c7cb9L84-L87)
> > > does runtime casts on callbacks of type `GFunc` (2 arguments), but can't
> > > tell statically when it gets a function that only takes one argument
> > > (like `free_func` in the example).
> > > It doesn't seem like a flag that has these drawbacks and only implements
> > > part of a solution is worth it.
> >
> >
> > Correct. The current approach only handles static, compile-time casts, not
> > runtime casts. The same as the former WebAssemblyFixFunctionBitcasts. Even
> > if only static handling is done, it fixes the main problem with GType
> > inheritance, enabling a fully functional GStreamer port to WebAssembly
> > (gst.wasm).
>
> I don't think getting just GStreamer and whatever other random subset of
> GLib-using dependents that just happen to only use compile-time casts is good
> enough. Even if you never care about getting any other program working in the
> future, GLib itself doesn't care about the distinction (there are many
> runtime casts in GLib already), so nothing stops them from doing some
> refactoring in the future which breaks you and puts you right back here
> again. And it doesn't really help anyone besides you. The whole reason we are
> here now is because the original FixFunctionBitcasts was a half-measure and
> not done in a way that was sustainable.
I agree. I was just pointing out that a project like GStreamer, due to its code
base and number of dependencies, is a good example of transparency as you
mention afterwards.
>
> > If this has a go, the runtime check implementation is also doable.
> > Do you want me to include that as part of this PR?
>
> Some goals I would have for an ideal solution to this problem:
>
> 1. Spec compliance: We follow the rule that function pointers casted to
> other types and back keep their same value. Ideally they'd keep the same
> value even when not casted back.
I think it is debatable, as we are dealing with an UB here the consequences of
the changes might lead to a non spec compliant situation, wouldn't be the
first time a flag does that (`-fwrapv`, `-ffast-math`, `-fno-strict-aliasing`)
as long as it is guarded, I don't think this is a hard requirement.
>
> 2. Behaves as developers expect: to me this means it should handle both
> compile-time and runtime casts. As mentioned above, it's hard to reason about
> generally if this doesn't hold, and GLib itself freely uses both. Maybe this
> is really just a special cast of "spec compliance" because casting and
> casting back could include runtime casts. But another way to say this is that
> it solves the whole problem, not just part of it.
Agree
>
> 3. Localized: Ideally we'd have something that doesn't require a global
> transformation; making something explicit (e.g. a compiler builtin rather
> than a flag) provides this best, but even if it's a flag, it should be
> possible to use the flag on just part of the program (e.g. just in GLib and
> not its dependents). This implies ABI compatibility.
This is somehow contrary to the point 4. If it is achieved through some
compiler builtins, it means that the code has to be modified in the specific
places it is required, which will make the developer's code webassembly
specific.
>
> 4. Transparent: Obviously it would be nice if users didn't have to change
> their code to use the feature. Otherwise, fewer changes is better.
Agree
>
> 5. Cheap: Lower cost is obviously better, but also the cost should
> ideally scale with the "real" usage of the feature, i.e. only pay for what
> you use. Making effects more localized usually makes them cheaper, and helps
> with pay-as-you-go.
>
Agree. That's one of the reasons why for me the
`EMULATE_FUNCTION_POINTER_CASTS` was no go.
>
> We may not be able to perfectly meet all of these goals; I don't yet know of
> a solution that does.
>
> What kind of runtime check implementation did you have in mind?
I already implement a first approach. The idea is fairly simple. Currently, the
code is just taking into account constant casts. For dynamic casts, once are
detected (`!isa<llvm::Constant>`) a general thread-local global variable will
be created in the module to store the runtime casted function to call along
with a trampoline function specific to the source and target signatures. In the
runtime cast, the global variable will be set and the general trampoline will
be called by using the global variable. The generated IR is something like this:
```
@__wasm_runtime_wrapper_vi_to_vii_fptr = internal thread_local global ptr null
; Function Attrs: noinline
define internal void @__wasm_runtime_wrapper_vi_to_vii(ptr %0, ptr %1) #1 {
entry:
%2 = load volatile ptr, ptr @__wasm_runtime_wrapper_vi_to_vii_fptr, align 4
call void %2(ptr %0)
ret void
}
```
This can be later merged by the linker to avoid multiple runtime thunk
trampolines I guess.
I'll cleanup a bit the code and send a new commit so you can review it
https://github.com/llvm/llvm-project/pull/153168
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
