Skip to content
Snippets Groups Projects

access: rtp: have owner callbacks instead of symbols

Closed François Cartegnie requested to merge fcartegnie/vlc:mr300601 into master
2 unresolved threads

duplicate symbols on static libs

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • This makes no sense to me. Sounds like you're papering over some build system misconfiguration

    • Link-incomplete convenience library usually leads to this kind of issues when linking statically (see json for reference) and are much more confusing because different definition of a given symbol exist at multiple location with the same name.

    • 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.

    • 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?

    • Those optimisations would not work in real life with non-trivial code preventing the compiler from eliding the indirect branches.

    • 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 and rtp_add_type while they are currently respectively called by symbol from vlc_rtp_pt_createand vlc_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 and add_type? Wouldn't the problem still happen even if it wasn't a convenience lib but a dynamic one if the test were using directly access/rtp/session.c and access/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 and rtp_add_type while they are currently respectively called by symbol from vlc_rtp_pt_createand vlc_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 Charmet
    • 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.

      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.

    • P.S.: Besides, while I wrote the code, I never asked to make it into a convenience library. I'm totally fine if this is brought back into the RTP module.

    • 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
    • Please register or sign in to reply
  • Alexandre Janniaux approved this merge request

    approved this merge request

  • Steve Lhomme changed milestone to %4.0

    changed milestone to %4.0

    • Owner callbacks are for the RTP payload format modules to call into the RTP stack. They're not for the RTP stack to call into its own self. Even the architecture of this change is wrong.

    • Please register or sign in to reply
  • François Cartegnie mentioned in merge request !3840 (merged)

    mentioned in merge request !3840 (merged)

  • added MRStatus::Stale label and removed MRStatus::InReview label

  • Thomas Guillem mentioned in merge request !4606 (closed)

    mentioned in merge request !4606 (closed)

Please register or sign in to reply
Loading