Visibility of symbols in Octave libraries

jwe · May 12, 2021, 4:09pm

I’m opening this topic as a result of the discussion in GNU Octave - Bugs: bug #60567, oct-file has an undefined symbol [Savannah]

Since applying the patch from GNU Octave - Patches: patch #8919, Start of patch to enable... [Savannah]
we’ve been adding API tags to export symbols from the liboctave and liboctinterp shared libraries. Many tags have been added to individual symbols in public header files. Though it works, it adds a lot of clutter and could cause some confusion for library users if a symbol name appears in a public header file but the function is not publicly available when linking or loading a .oct file.

I’d prefer that we tag namespaces instead of individual functions.

If necessary, we can tag classes and individual functions that are not currently inside the octave namespace. However, if there are individual functions that need tags, then we should also be working to move them inside the octave namespace instead, and the namespace declaration in the header file should be tagged.

If there are currently symbols declared in public header files that we don’t wish to export, then we probably need to start moving their declarations to private header files that are not installed.

Ultimately, I’d like to see (almost) all symbols that are declared in public header files also have public visibility.

There may have been some confusion because of (possibly unnecessary) friend declarations in some of the header files. Visibility attributes that are applied to class declarations don’t apply to friend declarations in the class scope. The friend declaration just allows the function access to private data of the enclosing class.

As a separate issue, it would probably be a good idea to replace as many of those as possible with simple extern declarations outside the class scope. It is likely that many of them aren’t needed for access to private data and only appear because 25+ years ago I thought declaring operators that way was a good thing to do.

I’m also attaching a simple example that allows easy experimentation with visibility attributes. It’s small and self-contained so you can modify the compiler and flags, definition of the attributes, and move the attribute location from the namespace declaration to the class declaration, or attach it to individual functions.

symbol-visibilty.tar.gz (820 Bytes)

mmuetzel · May 13, 2021, 8:44am

Fwiw, there seem to be still issues with how we mark symbols for export currently (at least on macOS):

bug #59820, build fails on macOS with visibility settings

The build no longer fails. But the resulting binaries still seem to lack (or use incorrect overloads of) some symbols.

jwe · May 13, 2021, 2:15pm

I don’t know whether it will fix the problems you are seeing, but I pushed the following changeset to add the OCTAVE_API tag to more template function declarations: octave: a616f687cf63

mmuetzel · May 13, 2021, 3:45pm

The rules for the macOS runners seem to have broken earlier today. They should be working now again.
The build logs should be available here once it has finished running.

mmuetzel · May 13, 2021, 4:59pm

Unfortunately, it doesn’t look like it fixed bug #59820.

mmuetzel · May 14, 2021, 7:28am

It looks like the Windows cross-builds are failing after that change. The error is that functions that are marked as dllimport cannot be defined in the same compilation unit.

The approach for “visibility” settings is slightly different on Windows and Linux. On Windows, functions are not only marked as “dllexport” in compilation units that define those functions. They are also marked as “dllimport” in compilation units where they are used (but not defined).
Definitions cannot be marked as “dllimport”. Only declarations can.
Template instantiations are declarations in that sense. Template specializations (that include a function body) are definitions.

That behavior might also help to get the flags right on Linux. IMHO, there is not much point exporting a symbol if it is defined in every compilation unit that is using it anyway.

Edit:
Untested as of now. But we could probably remove the API flags from the template definitions that are included in each compilation unit that instantiates them.
That might fix the compilation error for Windows targets. And it shouldn’t make a difference on other platforms IIUC.
MSparse-visibility.patch (3.8 KB)

Edit:
With those changes, cross-compiling for Windows still fails for me while linking liboctinterp with the following errors:

/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-ld: /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-ld: DWARF error: could not find variable specification at offset df154
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-ld: DWARF error: could not find variable specification at offset df162
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-ld: DWARF error: could not find variable specification at offset df172
libinterp/octave-value/.libs/liboctave-value.a(liboctave_value_la-ov-cx-sparse.o): in function `MSparse<bool>::~MSparse()':
/home/osboxes/Documents/Repositories/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/liboctave/array/MSparse.h:75: undefined reference to `__imp__ZTV7MSparseIbE'
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-ld: /home/osboxes/Documents/Repositories/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/liboctave/array/MSparse.h:75: undefined reference to `__imp__ZTV7MSparseIbE'
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-ld: libinterp/octave-value/.libs/liboctave-value.a(liboctave_value_la-ov-cx-sparse.o): in function `MSparse<bool>::~MSparse()':
/home/osboxes/Documents/Repositories/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/liboctave/array/MSparse.h:75: undefined reference to `__imp__ZTV7MSparseIbE'
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-ld: libinterp/octave-value/.libs/liboctave-value.a(liboctave_value_la-ov-cx-sparse.o): in function `MSparse<bool>::~MSparse()':
/home/osboxes/Documents/Repositories/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/liboctave/array/MSparse.h:75: undefined reference to `__imp__ZTV7MSparseIbE'
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-ld: libinterp/octave-value/.libs/liboctave-value.a(liboctave_value_la-ov-cx-sparse.o): in function `MSparse<bool>::MSparse(Sparse<bool> const&)':
/home/osboxes/Documents/Repositories/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/liboctave/array/MSparse.h:57: undefined reference to `__imp__ZTV7MSparseIbE'
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-ld: /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-ld: DWARF error: could not find variable specification at offset dc804
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-ld: DWARF error: could not find variable specification at offset dc812
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-ld: DWARF error: could not find variable specification at offset dc822
libinterp/octave-value/.libs/liboctave-value.a(liboctave_value_la-ov-re-sparse.o):/home/osboxes/Documents/Repositories/Octave/mxe-octave/tmp-default-octave/octave-7.0.0/liboctave/array/MSparse.h:75: more undefined references to `__imp__ZTV7MSparseIbE' follow
collect2: error: ld returned 1 exit status
make[5]: *** [Makefile:13186: libinterp/liboctinterp.la] Error 1

__imp__ZTV7MSparseIbE demangled is __imp_vtable for MSparse<bool>. The __imp_-prefix is for functions that have dllimport/dllexport attributes.

Where do we even use MSparse<bool>? I thought we only had MSparse<double> and MSparse<Complex>…

Edit:
I tried to explicitly instantiate MSparse<bool> and that fails like so:

n file included from ../liboctave/array/MSparse.h:129,
                 from ../liboctave/array/MSparse-b.cc:30:
../liboctave/array/MSparse.cc: In instantiation of ‘MSparse<T> quotient(const MSparse<T>&, const MSparse<T>&) [with T = bool]’:
../liboctave/array/MSparse-b.cc:34:1:   required from here
../liboctave/array/MSparse.cc:551:11: error: call of overloaded ‘MSparse(octave_idx_type&, octave_idx_type&, int)’ is ambiguous
  551 |       r = MSparse<T> (a_nr, a_nc, (Zero / Zero));
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../liboctave/array/MSparse-b.cc:30:
../liboctave/array/MSparse.h:75:3: note: candidate: ‘MSparse<T>::MSparse(octave_idx_type, octave_idx_type, octave_idx_type) [with T = bool; octave_idx_type = long int]’
   75 |   MSparse (octave_idx_type r, octave_idx_type c, octave_idx_type num_nz)
      |   ^~~~~~~
../liboctave/array/MSparse.h:70:12: note: candidate: ‘MSparse<T>::MSparse(octave_idx_type, octave_idx_type, T) [with T = bool; octave_idx_type = long int]’
   70 |   explicit MSparse (octave_idx_type r, octave_idx_type c, T val)
      |            ^~~~~~~
In file included from ../liboctave/array/MSparse.h:129,
                 from ../liboctave/array/MSparse-b.cc:30:
../liboctave/array/MSparse.cc: In instantiation of ‘MSparse<T> product(const MSparse<T>&, const MSparse<T>&) [with T = bool]’:
../liboctave/array/MSparse-b.cc:34:1:   required from here
../liboctave/array/MSparse.cc:405:39: warning: ‘*’ in boolean context, suggest ‘&&’ instead [-Wint-in-bool-context]
  405 |               r.data (i) = a.data (0) * r.data (i);
      |                            ~~~~~~~~~~~^~~~~~~~~~~~
../liboctave/array/MSparse.cc:422:39: warning: ‘*’ in boolean context, suggest ‘&&’ instead [-Wint-in-bool-context]
  422 |               r.data (i) = r.data (i) * b.data (0);
      |                            ~~~~~~~~~~~^~~~~~~~~~~~
../liboctave/array/MSparse.cc:461:49: warning: ‘*’ in boolean context, suggest ‘&&’ instead [-Wint-in-bool-context]
  461 |                       r.data (jx) = a.data (ja) * b.data (jb);
      |                                     ~~~~~~~~~~~~^~~~~~~~~~~~~
config.status: executing build-aux/subst-config-vals.sh commands
make[2]: *** [Makefile:22148: liboctave/array/libarray_la-MSparse-b.lo] Error 1

That makes the reason for the vtable error clearer.
The question still remains: Where do we implicitly instantiate MSparse<bool>?

Edit:
Possible candidate for an implicit instantiation could be this template in MatrixType.h:

  template <typename T>
  OCTAVE_API
  MatrixType (const MSparse<T> &a);

There is no specialization for SparseBoolMatrix afaics.
Does that mean that this template is used with T is bool for something like this command in Octave?

matrix_type(logical(speye(3)))

jwe · May 14, 2021, 3:01pm

I see that boolSparse.h includes MSparse.h in addition to Sparse.h but only uses Sparse<T> directly, not MSparse<T>.

Running nm on all the object files in my build tree and searching for MSparse<bool> in the output shows only the following:

./libinterp/octave-value/.libs/liboctave_value_la-ov-re-sparse.o
0000000000000000 W MSparse<bool>::~MSparse()
0000000000000000 W MSparse<bool>::~MSparse()
0000000000000000 W MSparse<bool>::~MSparse()
0000000000000000 n MSparse<bool>::~MSparse()
0000000000000000 W MSparse<bool> MSparse<double>::map<bool>(bool (&)(double)) const
0000000000000000 V typeinfo for MSparse<bool>
0000000000000000 V typeinfo name for MSparse<bool>
0000000000000000 V vtable for MSparse<bool>

./libinterp/octave-value/.libs/liboctave_value_la-ov-cx-sparse.o
0000000000000000 W MSparse<bool>::~MSparse()
0000000000000000 W MSparse<bool>::~MSparse()
0000000000000000 W MSparse<bool>::~MSparse()
0000000000000000 n MSparse<bool>::~MSparse()
0000000000000000 W MSparse<bool> MSparse<std::complex<double> >::map<bool>(bool (&)(std::complex<double> const&)) const
0000000000000000 V typeinfo for MSparse<bool>
0000000000000000 V typeinfo name for MSparse<bool>
0000000000000000 V vtable for MSparse<bool>

So it looks like it is used as the return type for the mapper functions that return a logical result for operations on a real or complex value.

I suppose these don’t need to be MSparse but could just be Sparse instead?

mmuetzel · May 14, 2021, 5:20pm

Thanks. That was very helpful!

With this patch I was able to cross-compile Octave for Windows.
MSparse-visibility-v2.patch (5.1 KB)

Building with gcc and clang still works and the test suite passes.

I’m waiting for the remaining packages to cross-compile. If it runs ok, I’ll probably push that patch later.

Edit:
Pushed here: octave: 1311b9a3590e (gnu.org)

mmuetzel · May 15, 2021, 1:00pm

Some symbols still seem to be missing (on Linux). While trying to install the Octave Forge package sparsersb, I see the following error:

octave:1> pkg install -forge sparsersb
mkoctfile: warning: LFLAGS is deprecated and will be removed in a future version of Octave, use LDFLAGS instead
error: /home/osboxes/.local/share/octave/7/packages/sparsersb-1.0.8/x86_64-pc-linux-gnu-api-v55+/sparsersb.oct: failed to load: /home/osboxes/.local/share/octave/7/packages/sparsersb-1.0.8/x86_64-pc-linux-gnu-api-v55+/sparsersb.oct: undefined symbol: _ZNK18octave_base_sparseI12SparseMatrixE6resizeERK10dim_vectorb
error: called from
    doc_cache_create>create_cache at line 116 column 20
    gen_doc_cache_in_dir>@<anonymous> at line 150 column 16
    doc_cache_create>gen_doc_cache_in_dir at line 151 column 9
    doc_cache_create at line 62 column 12
    install>generate_lookfor_cache at line 840 column 5
    install at line 241 column 7
    pkg at line 570 column 9

That symbol demangles to: octave_base_sparse<SparseMatrix>::resize(dim_vector const&, bool) const
I’m not sure why it wouldn’t be exported.

mmuetzel · May 16, 2021, 6:37pm

I used the following file to check the behavior of the API flags when compiling for Windows:
impl.cc:

#if defined (_WIN32)
#define API __declspec(dllexport)
#endif

template <typename T> class API api_class
{
  API api_class (void) { };
  api_class (float) { };
  
  virtual API ~api_class(void) = default;

  // virtual API void pure_virtual_API (void);
  // virtual void pure_virtual_nonAPI (void);
  
  virtual API void virtual_API (void) { };
  virtual void virtual_nonAPI (void) { };
  
  API void some_API (void) { };
  void some_nonAPI (void) { };
};

template class API api_class<double>;
template class api_class<float>;

template <typename T> class nonApi_class
{
  API nonApi_class (void) { };
  nonApi_class (float) { };
  
  virtual ~nonApi_class(void) = default;

  // virtual API void pure_virtual_API (void);
  // virtual void pure_virtual_nonAPI (void);
  
  virtual API void virtual_API (void) { };
  virtual void virtual_nonAPI (void) { };
  
  API void some_API (void) { };
  void some_nonAPI (void) { };
};

template class API nonApi_class<double>;
template class nonApi_class<float>;

I compiled it with these commands and checked which symbols where exported:

x86_64-w64-mingw32-g++ -c -oimpl.o impl.cc -O0
x86_64-w64-mingw32-g++ -shared impl.o -oimpl.dll -Wl,--out-implib,impl.dll.a
nm -C impl.dll.a

Results:

api_class<double>::api_class(): exported
api_class<double>::api_class(float): not exported
api_class<double>::~api_class(): exported
api_class<double>::virtual_API(): exported
api_class<double>::virtual_nonAPI(): not exported
api_class<double>::some_API(): exported
api_class<double>::some_nonAPI(): not exported
The same for api_class<float>.
nonApi_class<double>::nonApi_class(): exported
nonApi_class<double>::nonApi_class(float): not exported
nonApi_class<double>::~nonApi_class(): not exported
nonApi_class<double>::virtual_API(): exported
nonApi_class<double>::virtual_nonAPI(): not exported
nonApi_class<double>::some_API(): exported
nonApi_class<double>::some_nonAPI(): not exported
Same for nonApi_class<float>.

So it looks like flagging the template class itself is not enough to export all constituent member functions.
It looks like we’d need to flag each and every member function in a template class definition to have an instantiation be exported completely.

API flags at the template class definition “header” or at the instantiation don’t seem to matter. So in that respect, we’d still be flexible to satisfy gcc and clang for Linux/macOS.

This will only be important if we try to avoid instantiating the template class multiple times with the same type. IIUC, we are currently instantiating the template from the header wherever we need it.
Looking at the coding pattern with the instantiation files Array-*.cc, I think that the intent was to instantiate the templates only once. Afaict, this isn’t what is happening currently.
Should we try to change that?

Maybe that is also (part of) the reason for the issues we are seeing on macOS.

jwe · May 17, 2021, 1:44am

Yes, the intent was to just instantiate those templates once. But that code is quite old, from around the time templates were first added to C++. I also noticed that MSparse.h includes MSparse.cc unconditionally. In other template files that have separate .h and .cc parts, we only include the .cc file when we mean to instantiate the templates for specific types.

mmuetzel · December 17, 2021, 7:07am

I’m resurrecting this old thread because visibility flags are still not working correctly. I was hoping that we could straighten out the issues during the release cycle for Octave 7. But that is taking longer than what I had hoped for.

We still have issues when we use visibility flags on macOS (see bug #59820, build fails on macOS with visibility settings).
Yesterday, I noticed that I accidentally deactivated visibility settings for Windows entirely a few months back…
I was already wondering why recent changes to visibility handling seemed to work out without many issues. But re-activating visibility handling for Windows now breaks the Windows builds…
I’ll probably push a change that fixes the option to activate visibility flags on Windows anyway. Maybe, we can still fix the issues in time before the Octave 7 release.

In any case, I’m uncertain if it is a good idea to ship Octave 7 with visibility settings by default as of now. If there are obvious issues on macOS and Windows, it is quite likely that there are more hidden issues on Linux.
Maybe, we should deactivate those settings (at least by default or even remove the option) on all (or some) platforms for Octave 7. Instead, we could use the time to get this in a better shape for Octave 8.

We can still try to get this working maybe until early or mid January. But if it is not working until then, it might be better to set that feature aside for Octave 7.

What do you think?

mmuetzel · December 17, 2021, 8:35pm

I pushed a patch here that should restore the option to build with visibility flags on Windows:
octave: e0e22183ffca (gnu.org)

And another one that will hopefully help the build to succeed with visibility flags on Windows:
octave: f79deeded5d3 (gnu.org)

siko1056 · January 17, 2022, 12:26pm

Recently, bug #61821 was filed. It seems to be a package maintainer, who relied on implicitly visible Octave internals. Is this change sufficiently announced in the NEWS file?

mmuetzel · January 17, 2022, 12:30pm

Visibility flags are disabled by default for Octave 7. All symbols are exported indiscriminately. There is no change with respect to previous versions (unless a developer configures with visibility flags).

siko1056 · January 17, 2022, 1:57pm

Sorry, I misunderstood the state of the project And many thanks for the answer is the bug report.