access: rtp: have owner callbacks instead of symbols
duplicate symbols on static libs
Merge request reports
Activity
added Component::Input: network streams label
assigned to @fcartegnie
I specifically asked not to have undefined symbols in the previous MR so there's a contradiction with saying that it's link-incomplete now.
Not that it makes much difference. If symbols are missing, you will get an error if you botch the build rules. Otherwise, you might avoid the error, but not the underlying problems. It's wrong either way, and needs build system, not code, changes either way. That's exactly my point. And IIRC, I already pointed it out with JSON.
It's not just about keeping the code as is. This change interferes with LTO and CFI. We didn't care twenty years ago, but nowadays we should, and I personally do.
I specifically asked not to have undefined symbols in the previous MR so there's a contradiction with saying that it's link-incomplete now.
I don't get it. The fact that it's link-incomplete is a fact. There's no contradiction, this is an oversight that nobody got during last review and which is being fixed by this MR.
I also didn't find where you specifically asked that. I guess you reference your request for addition of
-no-undefined
?Not that it makes much difference. If symbols are missing, you will get an error if you botch the build rules. Otherwise, you might avoid the error, but not the underlying problems. It's wrong either way, and needs build system, not code, changes either way. That's exactly my point. And IIRC, I already pointed it out with JSON.
Symbols are not missing. Symbols are duplicated because they cannot be local and plugins are linked together.
It's not just about keeping the code as is. This change interferes with LTO and CFI. We didn't care twenty years ago, but nowadays we should, and I personally do.
Does it really impact LTO and CFI when everything is statically known and fixed at link time? If it doesn't impact C++ virtual table calls but it does impact C function tables, that would be weird, especially since it's already pretty optimized in our current C code (motivating the changes to ops and cbs tables) without even enabling options like
-flto
.It would rather be weird if the compiler was able to elide uses of ops/cbs structures. It doesn't even know that they can't change during the lifetime of an object, unlike
vptr
, and the representation of POD structures is much stricter than that of virtual classes, as well.I highlight the fact that I mentioned "link time". If you include "linker" inside "compiler", then that's pretty easy to check.
Simple test:
// common.h struct foo_ops { void (*hello)(void); }; void foo_hello(const struct foo_ops *ops); // foo.c #include "common.h" void foo_hello(const struct foo_ops *ops) { ops->hello(); } // main.c #include "common.h" #include <stdlib.h> #include <stdio.h> void hello(void) { printf("Hello, world!"); } int main(int argc, char **argv) { (void)argc; (void)argv; static const struct foo_ops ops = { .hello = hello, }; foo_hello(&ops); return 0; }
without
-flto
:$ otool -tvV main main: (__TEXT,__text) section _hello: 0000000100003f50 pushq %rbp 0000000100003f51 movq %rsp, %rbp 0000000100003f54 leaq 0x43(%rip), %rdi ## literal pool for: "Hello, world!" 0000000100003f5b xorl %eax, %eax 0000000100003f5d popq %rbp 0000000100003f5e jmp 0x100003f98 ## symbol stub for: _printf 0000000100003f63 nopw %cs:(%rax,%rax) _main: 0000000100003f70 pushq %rbp 0000000100003f71 movq %rsp, %rbp 0000000100003f74 leaq _main.ops(%rip), %rdi 0000000100003f7b callq _foo_hello 0000000100003f80 xorl %eax, %eax 0000000100003f82 popq %rbp 0000000100003f83 retq 0000000100003f84 nop 0000000100003f85 nop 0000000100003f86 nop 0000000100003f87 nop 0000000100003f88 nop 0000000100003f89 nop 0000000100003f8a nop 0000000100003f8b nop 0000000100003f8c nop 0000000100003f8d nop 0000000100003f8e nop 0000000100003f8f nop _foo_hello: 0000000100003f90 pushq %rbp 0000000100003f91 movq %rsp, %rbp 0000000100003f94 popq %rbp 0000000100003f95 jmpq *(%rdi)
With
-flto
:otool -tvV main 130 ↵ main: (__TEXT,__text) section _main: 0000000100003f80 pushq %rbp 0000000100003f81 movq %rsp, %rbp 0000000100003f84 leaq 0x11(%rip), %rdi ## literal pool for: "Hello, world!" 0000000100003f8b xorl %eax, %eax 0000000100003f8d callq 0x100003f96 ## symbol stub for: _printf 0000000100003f92 xorl %eax, %eax 0000000100003f94 popq %rbp 0000000100003f95 retq
Same result with dynamic libraries of course (with main renamed to foo and set to visibility default).
OBJECTS = main.o foo.o CFLAGS = -O3 -g -flto -fvisibility=hidden LDFLAGS = -flto main: $(OBJECTS) main.dylib: $(OBJECTS) $(LINK.o) $(OBJECTS) -shared -o $@ clean: rm -f $(OBJECTS) main main.dylib
otool -tvV main.dylib main.dylib: (__TEXT,__text) section _foo: 0000000000003f80 pushq %rbp 0000000000003f81 movq %rsp, %rbp 0000000000003f84 leaq 0x11(%rip), %rdi ## literal pool for: "Hello, world!" 0000000000003f8b xorl %eax, %eax 0000000000003f8d callq 0x3f96 ## symbol stub for: _printf 0000000000003f92 xorl %eax, %eax 0000000000003f94 popq %rbp 0000000000003f95 retq
What is the issue here then?
Isn't this thread diverging a bit from the MR's original goal? At this point I'm not sure I still understand this whole problem here.
What I gather from the commits of this MR (and the genetec ones) is that François would like to be able override
vlc_rtp_pt_instantiate
andrtp_add_type
while they are currently respectively called by symbol fromvlc_rtp_pt_create
andvlc_rtp_add_media_types
which are both called with the owner structure as a parameter.Passing function pointers seems like a simple enough solution to me and short of using
--allow-multiple-definition
I don't really see how it could be fixed by the build system. Am I missing something?You can close your eyes and plug your ears as much as you want. That won't make single step static linking any less fundamentally broken. This has already caused serious and hard to track down bugs in the past. We've been through this discussion several times.
In normal dynamic builds and in proper two-phased static builds this patch is not only unnecessary but actively harmful as noted.
I'm not trying to close my eyes or plug my ears, I sincerely just don't understand the link between static linking and the need to override some functions.
What do you think should be done to be able to customize
instanciate
andadd_type
? Wouldn't the problem still happen even if it wasn't a convenience lib but a dynamic one if the test were using directlyaccess/rtp/session.c
andaccess/rtp/rtpfmt.c
Isn't this thread diverging a bit from the MR's original goal? At this point I'm not sure I still understand this whole problem here.
The initial issue is duplicated symbols on Android.
What I gather from the commits of this MR (and the genetec ones) is that François would like to be able override
vlc_rtp_pt_instantiate
andrtp_add_type
while they are currently respectively called by symbol fromvlc_rtp_pt_create
andvlc_rtp_add_media_types
which are both called with the owner structure as a parameter.Those functions are currently not defined by the convenience library and are currently defined by the client, which works since the objects are linked as part of the final target (ie. they are objects of the final plugin).
Since they are not local (= not static) function, if they are defined in multiple plugins in a static build situation, the compiler will find multiple occurrence and complain.
But the point is not even only on linking: convenience libraries with incomplete symbols (ie. requiring the client to implement a symbol to link correctly) are hard to document, hard to link, confusing and they bring nothing as shown above. libvlc_json started to do that just because bison generates a symbol which is directly exposed to the convenience API and not wrapped behind a structure like every other convenience libraries.
You can close your eyes and plug your ears as much as you want. That won't make single step static linking any less fundamentally broken. This has already caused serious and hard to track down bugs in the past. We've been through this discussion several times.
Currently, it feels like using premature optimization to deprecate the way Android builds and iOS builds are done, without even discussing it first directly, which is pretty frustrating.
It would effectively remove RTP support for Android and iOS application, for an unknown unmeasured alleged gain.
I'll do the checks on the suggested approach here to measure the difference, and submit my partial linking branch to mitigate the harm static linking is supposed to create. It doesn't change my point here though.
Those functions are currently not defined by the convenience library and are currently defined by the client, which works since the objects are linked as part of the final target (ie. they are objects of the final plugin).
Well
rtp_add_type
at least is a part of the convenience library since it's defined in session.c so a build system change won't be enough anyway.Edited by Denis CharmetSince they are not local (= not static) function, if they are defined in multiple plugins in a static build situation, the compiler will find multiple occurrence and complain.
Of course not. Since switching to Libtool, VLC has never supported linking multiple modules in a single namespace. The only way this error occurs is if the build rules are dangerously broken. And by dangerously, I mean that they already caused horrible aliasing bugs in the past, and that they probably still do silently considering the huge potential for aliasing bugs between the 100+ static libraries in contribs.
But the point is not even only on linking: convenience libraries with incomplete symbols (ie. requiring the client to implement a symbol to link correctly) are hard to document,
This is very misleading. If you compare that with a "regular" library, maybe it's slightly harder to document. But they are private static library - just a glorified collection of object files to avoid recompiling the same source modules multiple times (as you said yourself up-thread). Thus the fair comparison is with just that, a bunch of interdependent source modules/objects files within a single plugin (indeed, how both this and JSON static libs originate). I don't see how that's any easier or harder to document one or the other.
hard to link,
They can be troublesome as dynamic library, not to say impossible on some platforms and forbidden on some distributions. And we have accordingly avoided them in that case.
But as static library, they are as easy as any other library; just add it to LIBADD or equivalent. And that's what we are talking about here.
confusing
That's very subjective.
But in any case, linking modules statically in a single namespace that are not meant to be linked that way is far worse if it comes to causing confusion (as the project's history has shown quite a few times).
and they bring nothing as shown above.
Not much point continuing this discussion since you clearly dismiss whatever inconvenient counter argument comes.
They bring simpler, fast and safer code as noted above already.
This MR is a harmful kludge to work-around a dangerous kludge. And sure, the incrementally additional harm is rather minimal, but the point is that this MR brings literally nothing else than this minimally harmful change to support a much more harmful/dangerous kludge.
And people can keep doing that dangerous kludge all they want. All I'm asking is not to interfere with other people's proper builds.
-1 / Nack.
Not much point continuing this discussion since you clearly dismiss whatever inconvenient counter argument comes.
I'll just quote back what I wrote since you clearly want to misinterpret how I feel about this.
I'll do the checks on the suggested approach here to measure the difference, and submit my partial linking branch to mitigate the harm static linking is supposed to create.
Please let's not escalate this further. I understand the frustration but I won't let this thread become a war.
Edited by Denis Charmet
added MRStatus::InReview label
changed milestone to %4.0
mentioned in merge request !3840 (merged)
added MRStatus::Stale label and removed MRStatus::InReview label
mentioned in merge request !4606 (closed)