x42 / avldrums.lv2

Dedicated AVLDrumkits LV2 Plugin
http://x42-plugins.com/x42/x42-avldrums
GNU General Public License v2.0
39 stars 11 forks source link

Please fix clang40 warnings #4

Closed yurivict closed 6 years ago

yurivict commented 6 years ago
c++: warning: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated [-Wdeprecated]
c++: warning: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated [-Wdeprecated]
sf2/Black_Pearl_4_LV2.sf2 -> build/Black_Pearl_4_LV2.sf2
sf2/Red_Zeppelin_4_LV2.sf2 -> build/Red_Zeppelin_4_LV2.sf2
fluidsynth/src/fluid_iir_filter.c:284:8: warning: using integer absolute value function 'abs' when argument is of floating point type [-Wabsolute-value]
  if ((abs (fres - iir_filter->last_fres) > 0.01))
       ^
fluidsynth/src/fluid_iir_filter.c:284:8: note: use function 'fabs' instead
  if ((abs (fres - iir_filter->last_fres) > 0.01))
       ^~~
       fabs
1 warning generated.
fluidsynth/src/fluid_defsfont.c:1044:18: warning: comparison of array 'sfpreset->name' not equal to a null pointer is always true [-Wtautological-pointer-compare]
  if ((sfpreset->name != NULL) && (FLUID_STRLEN(sfpreset->name) > 0)) {
       ~~~~~~~~~~^~~~    ~~~~
fluidsynth/src/fluid_defsfont.c:1445:16: warning: comparison of array 'sfinst->name' not equal to a null pointer is always true [-Wtautological-pointer-compare]
  if ((sfinst->name != NULL) && (FLUID_STRLEN(sfinst->name) > 0)) {
       ~~~~~~~~^~~~    ~~~~
2 warnings generated.
fluidsynth/src/fluid_voice.c:1544:12: warning: use of unary operator that may be intended as compound assignment (-=)
      peak =- peak_min;
           ^~
1 warning generated.
strip -s build/avldrumsUI_gl.so
fluidsynth/src/fluid_sys.c:891:12: warning: equality comparison with extraneous parentheses [-Wparentheses-equality]
    if ((c == '\n'))
         ~~^~~~~~~
fluidsynth/src/fluid_sys.c:891:12: note: remove extraneous parentheses around the comparison to silence this warning
    if ((c == '\n'))
        ~  ^      ~
fluidsynth/src/fluid_sys.c:891:12: note: use '=' to turn this equality comparison into an assignment
    if ((c == '\n'))
           ^~
           =
x42 commented 6 years ago

These are upstream fluidsynth warnings. ( https://github.com/FluidSynth/fluidsynth/ ). It's included to statically link the plugin and make it self-contained.

yurivict commented 6 years ago

Bundling is a bad idea. Syntax problem like these, and security problems get inherited.

Amazingly, when FLUID_SRC is removed avldrums still builds successfully.

x42 commented 6 years ago

You'll probably get undefined references when you try to instantiate the plugin.

x42 commented 6 years ago

Bundling (and statically linking with a private namespace and hidden symbols) is the only way to sure that the plugin works in a DAW that loads many plugins into the host's memory-space.

You may well have different versions of some lib installed on the same system, and individual plugins work, but as soon as you load two plugins that need different APIs into the same host, there are crashes. fftw and ffmpg are notorious cases, but also gui side there are various libs pango (shared font-cache) and various other libs are affected. some do have global locks for shared caches and locks are a no-go for realtime audio.

In this particular case libfluidsynth does have a sf2 cache with mutexes, so using a system-wide shared lib is not reasonable for plugins.

tl;dr for audio-plugins statically linking a dedicated version (hidden symbols) is the only way to provide reliability.

x42 commented 6 years ago

PS. security issues a non-issue. You already load plugins and ask them to perform realtime tasks (memlock, elevated scheduling priority). If they want to wreak havoc that's a lot easier... and you don't usually do audio-production on some public server or a multi-user system..

Also rt-performance is key. you don't want stack-protection or anything that can increase DSP load.

yurivict commented 6 years ago

I am creating a FreeBSD port for avldrums. On FreeBSD the reasons that you've mentioned aren't valid, because everything is built from the same ports tree and has same versions, and there are no such stack protections. These are probably mostly for Windows where there is no standard way to install packages, and different packages can be installed (copied in) with different versions.

If a security problem is discovered in some library, all instances of it are required to be patched, regardless of the details or assurances that this doesn't really matter in some instances. So bundling becomes a headache in such case.

yurivict commented 6 years ago

I'll unbundle the fluidsynth with port patches for now.

I suggest you create a make argument that allows to use system-wide FluidSynth on platforms where this isn't a problem. This way the port can be simplified in the future (unbundling patches can be removed).

x42 commented 6 years ago

It's really bad practice (from a pro-audio POV), so if you do that, I'd also very much prefer if you change the plugin-name and the URI.

yurivict commented 6 years ago

Now I am between a rock and a hard place. FreeBSD policy prohibits bundling when it is possible to unbundle. And here you are saying that you are against unbundling.

x42 commented 6 years ago

For audio-plugins there are good technical cases to statically link. deja-vu with debian policy

yurivict commented 6 years ago

While I disagree, I have no time to argue. I will keep it bundled.

Cheers! Yuri