AviSynth / AviSynthPlus

AviSynth with improvements
http://avs-plus.net
971 stars 75 forks source link

AVS_linkage macros in avisynth.h break One Definition Rule and cause infinite recursion on linux #130

Open wangqr opened 4 years ago

wangqr commented 4 years ago

I'm trying to compile Aegisub on Archlinux with Avs+ 2fd4156. The compilation finished without warning, but loading a simple avs file will cause SIGSEGV. This issue does not happen on Windows. I'm not exactly sure if this is an Aegisub issue, or linker issue, or AviSynth issue. So I'm posting here to see if you have any suggestions on how to debug.

Here are some tracebacks:

In Aegisub, it basically calls

IScriptEnvironment *env = CreateScriptEnvironment(AVISYNTH_INTERFACE_VERSION);
env->Invoke("Import", filename);

But I cannot reproduce this issue with a single file example with the above commands.

Here is the linking command used by Aegisub (CMakeFiles/Aegisub.dir/src/avisynth_wrap.cpp.o appears before /usr/lib/libavisynth.so):

/usr/bin/c++   -pthread -g  -L/home/wangqr/lib CMakeFiles/Aegisub.dir/src/command/app.cpp.o CMakeFiles/Aegisub.dir/src/command/audio.cpp.o CMakeFiles/Aegisub.dir/src/command/automation.cpp.o CMakeFiles/Aegisub.dir/src/command/command.cpp.o CMakeFiles/Aegisub.dir/src/command/edit.cpp.o CMakeFiles/Aegisub.dir/src/command/grid.cpp.o CMakeFiles/Aegisub.dir/src/command/help.cpp.o CMakeFiles/Aegisub.dir/src/command/keyframe.cpp.o CMakeFiles/Aegisub.dir/src/command/recent.cpp.o CMakeFiles/Aegisub.dir/src/command/subtitle.cpp.o CMakeFiles/Aegisub.dir/src/command/time.cpp.o CMakeFiles/Aegisub.dir/src/command/timecode.cpp.o CMakeFiles/Aegisub.dir/src/command/tool.cpp.o CMakeFiles/Aegisub.dir/src/command/video.cpp.o CMakeFiles/Aegisub.dir/src/command/vis_tool.cpp.o CMakeFiles/Aegisub.dir/src/dialog_about.cpp.o CMakeFiles/Aegisub.dir/src/dialog_align.cpp.o CMakeFiles/Aegisub.dir/src/dialog_attachments.cpp.o CMakeFiles/Aegisub.dir/src/dialog_automation.cpp.o CMakeFiles/Aegisub.dir/src/dialog_autosave.cpp.o CMakeFiles/Aegisub.dir/src/dialog_colorpicker.cpp.o CMakeFiles/Aegisub.dir/src/dialog_detached_video.cpp.o CMakeFiles/Aegisub.dir/src/dialog_dummy_video.cpp.o CMakeFiles/Aegisub.dir/src/dialog_export.cpp.o CMakeFiles/Aegisub.dir/src/dialog_export_ebu3264.cpp.o CMakeFiles/Aegisub.dir/src/dialog_fonts_collector.cpp.o CMakeFiles/Aegisub.dir/src/dialog_jumpto.cpp.o CMakeFiles/Aegisub.dir/src/dialog_kara_timing_copy.cpp.o CMakeFiles/Aegisub.dir/src/dialog_log.cpp.o CMakeFiles/Aegisub.dir/src/dialog_paste_over.cpp.o CMakeFiles/Aegisub.dir/src/dialog_progress.cpp.o CMakeFiles/Aegisub.dir/src/dialog_properties.cpp.o CMakeFiles/Aegisub.dir/src/dialog_resample.cpp.o CMakeFiles/Aegisub.dir/src/dialog_search_replace.cpp.o CMakeFiles/Aegisub.dir/src/dialog_selected_choices.cpp.o CMakeFiles/Aegisub.dir/src/dialog_selection.cpp.o CMakeFiles/Aegisub.dir/src/dialog_shift_times.cpp.o CMakeFiles/Aegisub.dir/src/dialog_spellchecker.cpp.o CMakeFiles/Aegisub.dir/src/dialog_style_editor.cpp.o CMakeFiles/Aegisub.dir/src/dialog_style_manager.cpp.o CMakeFiles/Aegisub.dir/src/dialog_styling_assistant.cpp.o CMakeFiles/Aegisub.dir/src/dialog_text_import.cpp.o CMakeFiles/Aegisub.dir/src/dialog_timing_processor.cpp.o CMakeFiles/Aegisub.dir/src/dialog_translation.cpp.o CMakeFiles/Aegisub.dir/src/dialog_video_details.cpp.o CMakeFiles/Aegisub.dir/src/dialog_video_properties.cpp.o CMakeFiles/Aegisub.dir/src/subtitle_format.cpp.o CMakeFiles/Aegisub.dir/src/subtitle_format_ass.cpp.o CMakeFiles/Aegisub.dir/src/subtitle_format_ebu3264.cpp.o CMakeFiles/Aegisub.dir/src/subtitle_format_encore.cpp.o CMakeFiles/Aegisub.dir/src/subtitle_format_microdvd.cpp.o CMakeFiles/Aegisub.dir/src/subtitle_format_mkv.cpp.o CMakeFiles/Aegisub.dir/src/subtitle_format_srt.cpp.o CMakeFiles/Aegisub.dir/src/subtitle_format_ssa.cpp.o CMakeFiles/Aegisub.dir/src/subtitle_format_transtation.cpp.o CMakeFiles/Aegisub.dir/src/subtitle_format_ttxt.cpp.o CMakeFiles/Aegisub.dir/src/subtitle_format_txt.cpp.o CMakeFiles/Aegisub.dir/src/visual_tool.cpp.o CMakeFiles/Aegisub.dir/src/visual_tool_clip.cpp.o CMakeFiles/Aegisub.dir/src/visual_tool_cross.cpp.o CMakeFiles/Aegisub.dir/src/visual_tool_drag.cpp.o CMakeFiles/Aegisub.dir/src/visual_tool_rotatexy.cpp.o CMakeFiles/Aegisub.dir/src/visual_tool_rotatez.cpp.o CMakeFiles/Aegisub.dir/src/visual_tool_scale.cpp.o CMakeFiles/Aegisub.dir/src/visual_tool_vector_clip.cpp.o CMakeFiles/Aegisub.dir/src/MatroskaParser.c.o CMakeFiles/Aegisub.dir/src/aegisublocale.cpp.o CMakeFiles/Aegisub.dir/src/ass_attachment.cpp.o CMakeFiles/Aegisub.dir/src/ass_dialogue.cpp.o CMakeFiles/Aegisub.dir/src/ass_entry.cpp.o CMakeFiles/Aegisub.dir/src/ass_export_filter.cpp.o CMakeFiles/Aegisub.dir/src/ass_exporter.cpp.o CMakeFiles/Aegisub.dir/src/ass_file.cpp.o CMakeFiles/Aegisub.dir/src/ass_karaoke.cpp.o CMakeFiles/Aegisub.dir/src/ass_override.cpp.o CMakeFiles/Aegisub.dir/src/ass_parser.cpp.o CMakeFiles/Aegisub.dir/src/ass_style.cpp.o CMakeFiles/Aegisub.dir/src/ass_style_storage.cpp.o CMakeFiles/Aegisub.dir/src/async_video_provider.cpp.o CMakeFiles/Aegisub.dir/src/audio_box.cpp.o CMakeFiles/Aegisub.dir/src/audio_colorscheme.cpp.o CMakeFiles/Aegisub.dir/src/audio_controller.cpp.o CMakeFiles/Aegisub.dir/src/audio_display.cpp.o CMakeFiles/Aegisub.dir/src/audio_karaoke.cpp.o CMakeFiles/Aegisub.dir/src/audio_marker.cpp.o CMakeFiles/Aegisub.dir/src/audio_player.cpp.o CMakeFiles/Aegisub.dir/src/audio_provider_factory.cpp.o CMakeFiles/Aegisub.dir/src/audio_renderer.cpp.o CMakeFiles/Aegisub.dir/src/audio_renderer_spectrum.cpp.o CMakeFiles/Aegisub.dir/src/audio_renderer_waveform.cpp.o CMakeFiles/Aegisub.dir/src/audio_timing_dialogue.cpp.o CMakeFiles/Aegisub.dir/src/audio_timing_karaoke.cpp.o CMakeFiles/Aegisub.dir/src/auto4_base.cpp.o CMakeFiles/Aegisub.dir/src/auto4_lua.cpp.o CMakeFiles/Aegisub.dir/src/auto4_lua_assfile.cpp.o CMakeFiles/Aegisub.dir/src/auto4_lua_dialog.cpp.o CMakeFiles/Aegisub.dir/src/auto4_lua_progresssink.cpp.o CMakeFiles/Aegisub.dir/src/base_grid.cpp.o CMakeFiles/Aegisub.dir/src/charset_detect.cpp.o CMakeFiles/Aegisub.dir/src/colorspace.cpp.o CMakeFiles/Aegisub.dir/src/colour_button.cpp.o CMakeFiles/Aegisub.dir/src/compat.cpp.o CMakeFiles/Aegisub.dir/src/context.cpp.o CMakeFiles/Aegisub.dir/src/export_fixstyle.cpp.o CMakeFiles/Aegisub.dir/src/export_framerate.cpp.o CMakeFiles/Aegisub.dir/src/fft.cpp.o CMakeFiles/Aegisub.dir/src/font_file_lister.cpp.o CMakeFiles/Aegisub.dir/src/frame_main.cpp.o CMakeFiles/Aegisub.dir/src/gl_text.cpp.o CMakeFiles/Aegisub.dir/src/gl_wrap.cpp.o CMakeFiles/Aegisub.dir/src/grid_column.cpp.o CMakeFiles/Aegisub.dir/src/help_button.cpp.o CMakeFiles/Aegisub.dir/src/hotkey.cpp.o CMakeFiles/Aegisub.dir/src/hotkey_data_view_model.cpp.o CMakeFiles/Aegisub.dir/src/image_position_picker.cpp.o CMakeFiles/Aegisub.dir/src/initial_line_state.cpp.o CMakeFiles/Aegisub.dir/src/main.cpp.o CMakeFiles/Aegisub.dir/src/menu.cpp.o CMakeFiles/Aegisub.dir/src/mkv_wrap.cpp.o CMakeFiles/Aegisub.dir/src/pen.cpp.o CMakeFiles/Aegisub.dir/src/persist_location.cpp.o CMakeFiles/Aegisub.dir/src/preferences.cpp.o CMakeFiles/Aegisub.dir/src/preferences_base.cpp.o CMakeFiles/Aegisub.dir/src/project.cpp.o CMakeFiles/Aegisub.dir/src/resolution_resampler.cpp.o CMakeFiles/Aegisub.dir/src/search_replace_engine.cpp.o CMakeFiles/Aegisub.dir/src/selection_controller.cpp.o CMakeFiles/Aegisub.dir/src/spellchecker.cpp.o CMakeFiles/Aegisub.dir/src/spline.cpp.o CMakeFiles/Aegisub.dir/src/spline_curve.cpp.o CMakeFiles/Aegisub.dir/src/string_codec.cpp.o CMakeFiles/Aegisub.dir/src/subs_controller.cpp.o CMakeFiles/Aegisub.dir/src/subs_edit_box.cpp.o CMakeFiles/Aegisub.dir/src/subs_edit_ctrl.cpp.o CMakeFiles/Aegisub.dir/src/subs_preview.cpp.o CMakeFiles/Aegisub.dir/src/subtitles_provider.cpp.o CMakeFiles/Aegisub.dir/src/subtitles_provider_libass.cpp.o CMakeFiles/Aegisub.dir/src/text_file_reader.cpp.o CMakeFiles/Aegisub.dir/src/text_file_writer.cpp.o CMakeFiles/Aegisub.dir/src/text_selection_controller.cpp.o CMakeFiles/Aegisub.dir/src/thesaurus.cpp.o CMakeFiles/Aegisub.dir/src/timeedit_ctrl.cpp.o CMakeFiles/Aegisub.dir/src/toggle_bitmap.cpp.o CMakeFiles/Aegisub.dir/src/toolbar.cpp.o CMakeFiles/Aegisub.dir/src/tooltip_manager.cpp.o CMakeFiles/Aegisub.dir/src/utils.cpp.o CMakeFiles/Aegisub.dir/src/validators.cpp.o CMakeFiles/Aegisub.dir/src/vector2d.cpp.o CMakeFiles/Aegisub.dir/src/version.cpp.o CMakeFiles/Aegisub.dir/src/video_box.cpp.o CMakeFiles/Aegisub.dir/src/video_controller.cpp.o CMakeFiles/Aegisub.dir/src/video_display.cpp.o CMakeFiles/Aegisub.dir/src/video_frame.cpp.o CMakeFiles/Aegisub.dir/src/video_out_gl.cpp.o CMakeFiles/Aegisub.dir/src/video_provider_cache.cpp.o CMakeFiles/Aegisub.dir/src/video_provider_dummy.cpp.o CMakeFiles/Aegisub.dir/src/video_provider_manager.cpp.o CMakeFiles/Aegisub.dir/src/video_provider_yuv4mpeg.cpp.o CMakeFiles/Aegisub.dir/src/video_slider.cpp.o CMakeFiles/Aegisub.dir/src/visual_feature.cpp.o CMakeFiles/Aegisub.dir/src/crash_writer.cpp.o CMakeFiles/Aegisub.dir/src/font_file_lister_fontconfig.cpp.o CMakeFiles/Aegisub.dir/src/audio_player_alsa.cpp.o CMakeFiles/Aegisub.dir/src/audio_provider_avs.cpp.o CMakeFiles/Aegisub.dir/src/avisynth_wrap.cpp.o CMakeFiles/Aegisub.dir/src/video_provider_avs.cpp.o CMakeFiles/Aegisub.dir/src/subtitles_provider_csri.cpp.o CMakeFiles/Aegisub.dir/src/audio_provider_ffmpegsource.cpp.o CMakeFiles/Aegisub.dir/src/ffmpegsource_common.cpp.o CMakeFiles/Aegisub.dir/src/video_provider_ffmpegsource.cpp.o CMakeFiles/Aegisub.dir/src/spellchecker_hunspell.cpp.o CMakeFiles/Aegisub.dir/src/audio_player_pulse.cpp.o CMakeFiles/Aegisub.dir/src/audio_player_openal.cpp.o  -o aegisub  -ldl libaegisub.a libluabins.a libluajit.a libresrc.a libcsri.a /usr/lib/libfontconfig.so /usr/lib/libass.so /usr/lib64/libboost_chrono.so.1.72.0 /usr/lib64/libboost_filesystem.so.1.72.0 /usr/lib64/libboost_locale.so.1.72.0 /usr/lib64/libboost_regex.so.1.72.0 /usr/lib64/libboost_system.so.1.72.0 /usr/lib64/libboost_thread.so.1.72.0 /usr/lib/libOpenGL.so /usr/lib/libGLX.so /usr/lib/libGLU.so /usr/lib/libicuuc.so /usr/lib/libicudata.so /usr/lib/libicui18n.so -pthread -lwx_gtk3u_adv-3.0 -lwx_baseu-3.0 -lwx_gtk3u_core-3.0 -lwx_gtk3u_gl-3.0 -lwx_gtk3u_stc-3.0 -lwx_baseu_xml-3.0 /usr/lib/libz.so /usr/lib/libasound.so /usr/lib/libavisynth.so /usr/lib/libffms2.so /usr/lib/libfftw3.so /usr/lib/libfftw3f.so /usr/lib/libfftw3l.so /usr/lib/libhunspell-1.7.so /usr/lib/libpulse.so /usr/lib/libopenal.so /usr/lib/libuchardet.so -lc -ldl /usr/lib64/libboost_chrono.so.1.72.0 -lpthread
wangqr commented 4 years ago

Here is the Minimum Reproducible Example:

#include <stdio.h>
#include <avisynth.h>

bool foo(VideoInfo* vi) {
  vi->IsRGB();
  vi->HasAudio();
  return false;
}

int main() {
  puts("Started");
  IScriptEnvironment *env = CreateScriptEnvironment(AVISYNTH_INTERFACE_VERSION);
  env->Invoke("Import", "test.avs");
  puts("After env->Invoke");
  foo(NULL);
  puts("Ready to exit");
  return 0;
}
~/avstest$ g++ -I/usr/include/avisynth test.cpp -lavisynth
~/avstest$ cat test.avs
BlankClip()
Info()
~/avstest$ ./a.out
Started
[1]    45622 segmentation fault (core dumped)  ./a.out
~/avstest$ g++ -I/usr/include/avisynth test.cpp -c -o test.o
~/avstest$ nm test.o
                 U AVS_linkage
                 U CreateScriptEnvironment
0000000000000000 V DW.ref.__gxx_personality_v0
                 U _GLOBAL_OFFSET_TABLE_
                 U __gxx_personality_v0
000000000000002b T main
                 U puts
                 U __stack_chk_fail
                 U _Unwind_Resume
0000000000000000 T _Z3fooP9VideoInfo
0000000000000000 W _ZN8AVSValueC1EPKc
0000000000000000 W _ZN8AVSValueC2EPKc
0000000000000000 n _ZN8AVSValueC5EPKc
0000000000000000 W _ZN8AVSValueD1Ev
0000000000000000 W _ZN8AVSValueD2Ev
0000000000000000 n _ZN8AVSValueD5Ev
0000000000000000 W _ZNK9VideoInfo5IsRGBEv
0000000000000000 W _ZNK9VideoInfo8HasAudioEv
wangqr commented 4 years ago

In Aegisub's source code, VideoInfo::IsRGB and VideoInfo::HasAudio somehow failed to inline and exports a symbol in the corresponding object file. These object files appears earlier than /usr/lib/libavisynth.so when linking and overrides the actual implementation. This kind of usage of inline member function actually violates C++ ODR, and caused this issue.

It worked when AVS was a Windows only thing, because dll does not export every symbol by default, and internally resolved those symbols. But this is not the case on *nix systems. I understand that I can "workaround" this issue by manually inline those function calls, and hope gcc's optimization will remove these unused problematic symbols. But I don't think this should considered the correct solution.

I found this which explained this AVS_linkage change. I see that you're trying to be both forward and backward compatible at ABI level. But in my opnion the current solution is just reinventing the dynamic linker / Procedure Linkage Table in a non-standard-complaint way.

If the interface still needs to be kept this way, I sugget change internal function to a different name (e.g. prefix an underscore, or use another namespace), and all function calls within AviSynth itself use these internal names. Then the interface functions should only be used outside AVS implementation.

pinterf commented 4 years ago

You probably have to use the C interface with avisynth_c.h That allows you to use AVS_VideoInfo instead of VideoInfo, etc. For a working example see: https://github.com/qyot27/FFmpeg/blob/avsplus_linux/libavformat/avisynth.c https://github.com/qyot27/FFmpeg/blob/avsplus_linux/libavformat/avisynth.c#L565

qyot27 commented 4 years ago

This is unfortunately something that got inherited from classic AviSynth. I don't recall anything specifically Plus-related that ever messed around with AVS_Linkage.

I'm just wondering if this is needed at all on non-Windows and we could get away with putting it behind an AVS_WINDOWS ifdef. There is no straightforward equivalent of the 2.5 interface on *nix (AvxSynth excluded, unless Aegisub can actually work with it), so the compatibility tricks using AVS_Linkage would seem to probably be pretty Windows-specific in the first place.

Not least of all because very very very few plugins were ever ported to AvxSynth (offhand I think it was like, five), so any that would otherwise work perfectly with AviSynth+ will almost assuredly get properly fixed up for the modern version of the code anyway (and would need to in order to take advantage of the new features Plus brings in). The AVS_Linkage stuff, as described in the FilterSDK, was at least partially intended to compensate for the breakage and relieve the need to recompile a plugin with the fixes. But that seems like a very Windows-centric concern; Linux development, or nix more generally, usually kind of pivots around the idea of recompiling things if they've been fixed properly (because it usually is a lot easier to just recompile something there than it is on Windows). If it's possible to keep that kind of compatibility on Windows while just dropping it entirely on nix, how far would that go toward resolving this?

While I certainly would like to see more use of the C interface to talk to the library, I know that's not always possible, and the C++ interface shouldn't end up causing problems like this for the sake of just one platform.

pinterf commented 4 years ago

Finally. My first linux proggy, I managed to use the CPP interface

What it demonstrates:

Build commands (dunno if /c++11 is really needed - threads)

g++ -std=c++11 -Wall -I. -g test.cpp -c
g++ -std=c++11 -Wall test.o -o test -ldl -pthread
chmod a+x test 

Important! Specify -pthread thread library for linking or else std::thread create will segfault within libavisynth

I just copied the avisynth.h and the avs/ folder right next to the source (-I. option)

test.cpp:

#include <avisynth.h>
#include <stdio.h>
#include <dlfcn.h>

const AVS_Linkage *AVS_linkage = nullptr;

int main() {
  fprintf(stdout,"Program started\r\n");

  void *handle = dlopen("libavisynth.so", RTLD_LAZY);
  fprintf(stdout,"after dlopen\r\n");
  if (handle == NULL) {
    fprintf(stdout, "Cannot load libavisynth.so\r\n");
    return 1;
  }

  fprintf(stdout, "Getting 'CreateScriptEnvironment' entry point\r\n");
  void *mkr = dlsym(handle, "CreateScriptEnvironment");
  if(mkr == NULL) {
    fprintf(stdout, "Cannot find CreateScriptEnvironment\r\n");
    return 1;
  }

  typedef IScriptEnvironment * (* func_t)(int);
  func_t CreateScriptEnvironment = (func_t)mkr;
  fprintf(stdout,"Calling CreateScriptEnvironment\r\n");
  IScriptEnvironment *env = CreateScriptEnvironment(AVISYNTH_INTERFACE_VERSION);
  fprintf(stdout,"After CreateScriptEnvironment\r\n");

  fprintf(stdout,"Filling up AVS_Linkage\r\n");
  AVS_linkage = env->GetAVSLinkage(); // e.g. for VideoInfo.BitsPerComponent, etc..

  fprintf(stdout,"Invoke test: calling env->Invoke with test.avs\r\n");
  try {
    // replace test.avs with an unknown filename to get and catch avisynth error
    env->Invoke("Import", "test.avs");
    fprintf(stdout,"env->Invoke finished successfully\r\n");
  }
  catch (const AvisynthError &err) {
    fprintf(stdout,"Avisynth exception during invoke: %s\r\n", err.msg);
  }

  VideoInfo vi;
  vi.pixel_type = VideoInfo::CS_YUV444P10;
  int bitdepth = vi.BitsPerComponent();
  fprintf(stdout,"VideoInfo bitdepth comes through AVS_linkage. YUV444P10 is %d bits\r\n", bitdepth);

  // exception raised and caught
  try {
    env->ThrowError("Hello %s", "Leo");
  }
  catch (const AvisynthError &err) {
    fprintf(stdout,"Avisynth exception caught: %s\r\n", err.msg);
  }

  env->DeleteScriptEnvironment();
  fprintf(stdout,"Avisynth Script Environment deleted\r\n");

  fprintf(stdout,"Ready to exit\r\n");
  dlclose(handle);
  return 0;
} 

We'll have a heap of warnings during the build, nevertheless, output of test:

Program started
after dlopen
Getting 'CreateScriptEnvironment' entry point
Calling CreateScriptEnvironment
After CreateScriptEnvironment
Filling up AVS_Linkage
Invoke test: calling env->Invoke with test.avs
env->Invoke finished successfully
VideoInfo bitdepth comes through AVS_linkage. YUV444P10 is 10 bits
Avisynth exception caught: Hello Leo
Avisynth Script Environment deleted
Ready to exit

All this on Ubuntu 19.10, in Windows 10 WSL

wangqr commented 4 years ago

I see the key point is that you are using dynamic load to workaround the conflicting definition issue. It does work, and I additionally added RTLD_LOCAL | RTLD_DEEPBIND to dlopen parameter to ensure the loader resolves the symbol using implementation in shared object.

We still need to decide whether AVS should support dynamic / static linking, or only usable via dynamic loading.