bugGNU Octave - Bugs: bug #61472, AddressSanitizer crash in MEX tests

 
 

bug #61472: AddressSanitizer crash in MEX tests

Submitter:  Arun Giridhar <arungiridhar>
Submitted:  Sat 13 Nov 2021 11:21:14 AM UTC
   
 
Category:  Configuration and Build System Severity:  3 - Normal
Priority:  7 - High Item Group:  Build Failure
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 7.0.90 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 03 Jan 2022 04:09:59 PM UTC, comment #50: 

I'm going to close this as fixed for now. If we'd like to discuss whether we want to raise the minimum requirement to C++17, we could discuss that e.g. on discourse.

Markus Mützel <mmuetzel>
Group administrator
Thu 23 Dec 2021 04:17:12 PM UTC, comment #49: 

Sorry, I meant last question in comment #36.

Markus Mützel <mmuetzel>
Group administrator
Thu 23 Dec 2021 03:01:04 PM UTC, comment #48: 


> But is that necessary for our Windows builds since we are building with GCC 11 now?  Can't we just build everything with the (now default) C++17 mode and use the std::pmr::... allocator code?


I was thinking about this, too. See the last question in comment #38.
But in general we don't know whether it is ok to ignore/override if a package is setting a specific standard version (e.g. `-std=c++11`) in their build rules. That could be because they'd like to use features that weren't available in prior versions. But it could also be because their code wouldn't compile in C++ versions newer than the set one...
So, if we'd want to compile everything with C++17, we'd need to raise the minimum C++ version for packages to C++17. (Or I didn't understand your proposition.)
If we'd like to do that, we should probably give package developers a prior note and the chance to adapt their package to the new requirements.

Markus Mützel <mmuetzel>
Group administrator
Thu 23 Dec 2021 02:14:21 PM UTC, comment #47: 

tldr: It's probably best to defer the full fix for this issue until after version 7, so for now let's just disable the std::pmr:... allocator by default to avoid further delay for the release.

@Markus: I think the instantiation_guard was added long ago as a way to prevent implicit instantiations, probably to speed up building Octave by not having the compiler repeatedly instantiate the same templates many times.  But I'm not sure whether that really matters?  If the full template definition is not available to the compiler, instantiation is skipped anyway.

Today we should probably do this job with "extern template ..." declarations for each type we want to explicitly instantiate.  Maybe that still makes sense for an object like Array<T> that is so widely used.

What I would like to do (but is clearly too much for version 7) is

  1. refactor the Array class so that the full definition is always available to the compiler so that implicit instantiation can happen
  2. maybe use extern template declarations and explicit instantiation for some commonly used types (float, double, complex<float>, complex<double>, intN, ...)?
  3. see whether we can allow Octave to be built with a C++17 compiler for std::pmr::... and still build .oct files with C++11?


For the release, I suppose it reasonable to disable the std::pmr::... bits by default.  But is that necessary for our Windows builds since we are building with GCC 11 now?  Can't we just build everything with the (now default) C++17 mode and use the std::pmr::... allocator code?

BTW, I did try step 1 above but ran into some trouble with the instantiation of the sorting functions.  Which then made me question whether we still need to be using our own custom sort code (we stole it from Python quite some time ago) or whether it would be OK now to use sorting functions from the standard library.  I'm also thinking that we should follow what is done in the standard library and eliminate the Array<T>::sort method in favor of an independent sort function.  Standard library compatible iterators for Array<T> would also make sense.

John W. Eaton <jwe>
Group administrator
Wed 22 Dec 2021 04:05:51 PM UTC, comment #46: 

We currently have this in Array.cc to prevent accidental implicit instantiations of the Array<T> class template (IIUC):

template <typename T, typename Alloc>
OCTARRAY_API
void Array<T, Alloc>::instantiation_guard ()
{
  // This guards against accidental implicit instantiations.
  // Array<T, Alloc> instances should always be explicit and use INSTANTIATE_ARRAY.
  T::__xXxXx__ ();
}


What was the motivation for that? Is that no longer an issue? Or can we make changes so that Array<T> can be implicitly instantiated?

Markus Mützel <mmuetzel>
Group administrator
Wed 22 Dec 2021 03:55:28 PM UTC, comment #45: 

The files like liboctave/array/Array-f.cc were originally intended only to perform explicit instantiation of Array<T> objects for a specific type.  Maybe now they also include some specializations for a few functions (I see Array<float>::issorted in Array-f.cc, for example).  If these specializations are still needed for speed, then I think we could declare them as extern in Array.h and still provide them in liboctave.  But otherwise, provide a full definition of the Array<T> template when Array.h is included and allow the compiler to instantiate the template as needed.  Isn't that how <vector> and other standard library templates work?

John W. Eaton <jwe>
Group administrator
Wed 22 Dec 2021 03:47:27 PM UTC, comment #44: 

Maybe I'm misunderstanding. But aren't the Array-*.cc files in liboctave/array mainly for specializations of the Array<T, ..> template for different types? Wouldn't we need to convert all of these to headers if we'd like to instantiate these types "on-the-fly"?

Markus Mützel <mmuetzel>
Group administrator
Wed 22 Dec 2021 03:43:39 PM UTC, comment #43: 

Which specializations?  I don't see any in Array.h or Array.cc.  Are there specializations defined somewhere else?

John W. Eaton <jwe>
Group administrator
Wed 22 Dec 2021 03:22:37 PM UTC, comment #42: 

What about specializations?

Markus Mützel <mmuetzel>
Group administrator
Wed 22 Dec 2021 03:19:42 PM UTC, comment #41: 

If explicit instantiation is an issue, then maybe we should try to fix the Array class so that it may be instantiated as needed.  Then, even if Octave uses Array<T,std::pmr::polymporphic_allocator<T>> internally, Array<T,std::allocator<T>> should be instantiated if it is used.

John W. Eaton <jwe>
Group administrator
Wed 22 Dec 2021 03:10:06 PM UTC, comment #40: 

IIUC, the issue that prevented packages to compile with C++11 if Octave was compiled with the new C++17 feature is the following:

If Octave was compiled with the new feature, OCTAVE_DISABLE_STD_PMR_POLYMORPHIC_ALLOCATOR is defined to 1. In that case, the headers use the new C++17 feature.
If a package sets compiler flags to use (only) C++11 features and includes those headers, the compiler complains (rightfully so).

I'm not sure if your proposed solution will work.
If Octave was compiled with the new feature, it will export functions that expect Array<T, std::pmr::polymorphic_allocator<T>> types as arguments.
If a packages sets e.g. `-std=c++11`, the headers will define functions that expect Array<T, std::allocator<T>> types as arguments.
Will that work without issues? It might be worth a try...

Markus Mützel <mmuetzel>
Group administrator
Wed 22 Dec 2021 01:43:27 PM UTC, comment #39: 

My intent was never to force a switch to require a C++17 compiler even if Octave is built with one.

My original change used std::pmr::memory_resource conditional on


#if __cplusplus >= 201703L


which I understood to be sufficient to detect this feature.  Using this conditional, I think Octave could be compiled with a C++17 compiler that supports std::pmr::polymorphic_allocator and then also allow .oct files to be built with -std=c++11.

But then there were reports that some compilers lie about their support for C++17 features and fail to compile code using std::pmr::memory_resource even though the __cplusplus >= 201703L test succeeds.

So how about doing the following instead?  In Octave, we can use


#if __cplusplus >= 201703L && ! defined (OCTAVE_DISABLE_STD_PMR_POLYMORPHIC_ALLOCATOR)


We can check whether the compiler used to build Octave is lying about supporting C++17 and disable this feature automatically when building  Octave but DO NOT define OCTAVE_DISABLE_STD_PMR_POLYMORPHIC_ALLOCATOR in octave-config.h.  Then if Octave is compiled with a proper C++17 compiler, we use this feature internally.  But if a .oct file is compiled with -std=c++11, we fall back on using std::allocator.  This choice should not affect pure MEX files because the Array interface is not exposed in the MEX interface.

If your compiler lies to you and defines __cplusplus to be 201703L or later but doesn't support std::pmr::polymorphic_allocator, you can define OCTAVE_DISABLE_STD_PMR_POLYMORPHIC_ALLOCATOR yourself (command line or in your code prior to including Octave header files).

Would this solution work?

If I understand correctly, a conforming compiler should also define __cpp_lib_memory_resource if it has the <memory_resource> header, so maybe we should also use that macro in some way?  Or should it be sufficient to use the value of __cplusplus alone?

John W. Eaton <jwe>
Group administrator
Sun 19 Dec 2021 06:29:57 PM UTC, comment #38: 

I pushed a change that makes using std::pmr::polymorphic_allocator optional:
https://hg.savannah.gnu.org/hgweb/octave/rev/9b41aba64c12

IIUC, that means that Octave will be using the less efficient mex interface by default. At the same time, an Octave with default configuration should be able to compile most .oct file code out there again.

Maybe, we should rename the corresponding preprocessor variable from OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR to OCTAVE_ENABLE_STD_PMR_POLYMORPHIC_ALLOCATOR now that its usage can be turned on or off with a configure switch...

Should we also add a note to the NEWS file to announce that Octave will require C++17 in a future version so that package developers are aware of this and can check if their code compiles in that C++ version?

Markus Mützel <mmuetzel>
Group administrator
Fri 17 Dec 2021 11:35:54 AM UTC, comment #37: 

We are already quite late in the Octave 7 release cycle. Combined with the fact that compiling with this feature will raise the minimum compiler version significantly (clang's libc++ doesn't support it presently), should be maybe make that requirement optional with a configure option? To keep it save, should we keep that feature off by default?
A user or packager could than opt-in to that feature if they understand the implications. Those include that all .oct files that are linked to that version of Octave must be compatible with C++17 and must be compiled with a compiler that supports `std::pmr::polymorphic_allocator`.

What do you think?

Markus Mützel <mmuetzel>
Group administrator
Fri 17 Dec 2021 10:35:59 AM UTC, comment #36: 

It looks like that flag is set in the build rules of the package:
https://sourceforge.net/p/octave/ltfat/ci/master/tree/oct/Makefile_unix

export CXXFLAGS := $(shell $(MKOCTFILE) -p CXXFLAGS) -std=gnu++11 -Wall -DLTFAT_LARGEARRAYS $(OPTCXXFLAGS)


Does that mean the package will be incompatible with Octave 7 if Octave was build with newer features? Do we require that the package is updated to stay compatible? Or should we somehow parse the flags and strip off the ones we don't like in mkoctfile?

Markus Mützel <mmuetzel>
Group administrator
Fri 17 Dec 2021 10:13:48 AM UTC, comment #35: 

Compilation went on a little further. But it failed for the ltfat package:

make[3]: Entering directory '/home/osboxes/Documents/Repositories/Octave/mxe-octave/tmp-of-ltfat/tmpmycjcpn3-pkg/ltfat/oct'
mkoctfile: warning: LFLAGS is deprecated and will be removed in a future version of Octave, use LDFLAGS instead
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/bin/x86_64-w64-mingw32-mkoctfile --verbose -strip -I../src/modules/libltfat/include -DNDEBUG -L../lib -lltfat comp_atrousfilterbank_td.cc
mkoctfile: warning: LFLAGS is deprecated and will be removed in a future version of Octave, use LDFLAGS instead
x86_64-w64-mingw32-g++ -c -I/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include  -I/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/.. -I/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave -I/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include  -pthread -fopenmp -g -O2 -std=gnu++11 -Wall -DLTFAT_LARGEARRAYS -DLTFAT_BUILD_STATIC -DMINGW=1   -I../src/modules/libltfat/include  -DNDEBUG comp_atrousfilterbank_td.cc -o /tmp/oct-zGPSFX.o
In file included from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/Array.h:38,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/Array-util.h:31,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/MSparse.h:31,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/MatrixType.h:31,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/mx-base.h:33,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/Matrix.h:34,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/oct.h:33,
                 from ltfat_oct_template_helper.h:5,
                 from comp_atrousfilterbank_td.cc:8:
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/Array-fwd.h:34:45: error: 'pmr' in namespace 'std' does not name a type
   34 | template <typename T, typename Alloc = std::pmr::polymorphic_allocator<T>>
      |                                             ^~~
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/Array-fwd.h:34:40: note: 'std::pmr' is only available from C++17 onwards
   34 | template <typename T, typename Alloc = std::pmr::polymorphic_allocator<T>>
      |                                        ^~~
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/Array-fwd.h:34:48: error: expected '>' before '::' token
   34 | template <typename T, typename Alloc = std::pmr::polymorphic_allocator<T>>
      |                                                ^~
In file included from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/Array.h:39,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/Array-util.h:31,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/MSparse.h:31,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/MatrixType.h:31,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/mx-base.h:33,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/Matrix.h:34,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/oct.h:33,
                 from ltfat_oct_template_helper.h:5,
                 from comp_atrousfilterbank_td.cc:8:
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/dim-vector.h:516:35: error: template argument 2 is invalid
  516 |   OCTAVE_API Array<octave_idx_type> as_array (void) const;
      |                                   ^
In file included from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/Array.h:40,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/Array-util.h:31,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/MSparse.h:31,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/MatrixType.h:31,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/mx-base.h:33,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/Matrix.h:34,
                 from /home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/oct.h:33,
                 from ltfat_oct_template_helper.h:5,
                 from comp_atrousfilterbank_td.cc:8:
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/idx-vector.h:109:61: error: template argument 2 is invalid
  109 |       virtual idx_base_rep * sort_idx (Array<octave_idx_type>&) = 0;
      |                                                             ^
/home/osboxes/Documents/Repositories/Octave/mxe-octave/usr/x86_64-w64-mingw32/include/octave-8.0.0/octave/../octave/idx-vector.h:120:36: error: template argument 2 is invalid
  120 |       virtual Array<octave_idx_type> as_array (void);
      |                                    ^


And continuing on with similar error messages.

The compiler command includes `-std=gnu++11`. Maybe that deactivates support for `std::pmr::polymorphic_allocator`?
Is that something that `mkoctfile` adds on its own? Or is it something that the package sets?
How should this be handled correctly?

Markus Mützel <mmuetzel>
Group administrator
Thu 16 Dec 2021 10:07:30 PM UTC, comment #34: 

The new pre-processor variable was called OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR in some places and HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR in others. Since this is facing to the public, I figured it might be best calling it OCTAVE_HAVE_STD_PMR_POLYMORPHIC_ALLOCATOR everywhere:
https://hg.savannah.gnu.org/hgweb/octave/rev/79a7f3e3cf54

Markus Mützel <mmuetzel>
Group administrator
Thu 16 Dec 2021 08:02:42 AM UTC, comment #33: 

For future reference: bug #59820 comments #56 to #62 are probably related to the change for this report.

Markus Mützel <mmuetzel>
Group administrator
Thu 16 Dec 2021 07:55:31 AM UTC, comment #32: 

Compiling Octave packages seem to fail with linking errors on the buildbots (which might be related to the changes for this report?):
http://buildbot.octave.org:8010/#/builders/14/builds/1305/steps/9/logs/stdio

/scratch/buildbot/workers/jwe-debian-x86_64-5/w32-on-debian/src/usr/bin/i686-w64-mingw32-ld: /tmp/oct-UX2ScX.o:mark_for_deletion.cc:(.rdata$_ZTV5ArrayINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESaIS5_EE[__ZTV5ArrayINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESaIS5_EE]+0x10): undefined reference to `Array<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::resize_fill_value() const'


That might be caused by those symbols either not being dllexported or they might not be marked as dllimport when they are used in the package.

Markus Mützel <mmuetzel>
Group administrator
Thu 16 Dec 2021 01:38:54 AM UTC, comment #31: 

I checked in a series of changesets so that we will now match malloc/std::free calls when passing float and double values from MEX functions back to the interpreter if Octave is built with a C++ compiler that supports the C++17 std::pmr:polymorphic_allocator concept.  If that feature is not available, then we still transfer real-valued float and double arrays from MEX functions back to the interpreter without a copy, but we rely on the ability to use delete[] to release the storage allocated with malloc.  As far as I know, that should work on current systems for plain old data types like float and double, but will not work for class types like std::complex<float> or intNDArray objects.  Also, more changes are needed to make copy-free transfer work for intNDArray objects.  I'll try to make that happen before the release, but I'm out of time for now.

If there are problems with mixing malloc and delete[] on some real systems (apart from those that show up with the address sanitizer) then we can go back to always performing a copy unless the c++17 feature is available.  It works for me with GCC 11 without needing to specify -std=c++17 because that is now the default.

John W. Eaton <jwe>
Group administrator
Mon 06 Dec 2021 07:34:17 PM UTC, comment #30: 

Markus: Yes, the changes you made did help. thanks.  I think it was fixing the declarations for octave_int<T>.  I have some more pending changes for range<T>, MArray<T>, and intNDArray<T>.  There are probably a few others that also need to be fixed but none of these other changes appear to be necessary to fix the visibility problems.

I'll look at enabling the polymorphic allocator only if we are using a compiler that claims to be C++17 actually supports the feature and otherwise falling back on copying complex objects otherwise.  I can also make copying an option for non-class objects, though I think it should be OK to mix malloc with delete for those.  If it causes trouble on actual systems, users can disable it.

John W. Eaton <jwe>
Group administrator
Mon 06 Dec 2021 04:58:39 PM UTC, comment #29: 

Oh, I think I did attach them but then I previewed my reply and then submitted it.  Does that lose the attachments?  Hmm.  Anyway, I'm attaching the two files here.



(file #52441, file #52442)

John W. Eaton <jwe>
Group administrator
Mon 06 Dec 2021 04:10:08 PM UTC, comment #28: 

@jwe: Did you intend to attach some files to your last comment?

Wrt the visibility issue: Does that still happen with a current tip? Some of the changes for bug #59820 might have improved the situation...

Markus Mützel <mmuetzel>
Group administrator
Mon 06 Dec 2021 03:32:30 PM UTC, comment #27: 

After looking at this a little more, I'm not sure what the best solution is.

We only mix malloc with delete in the case of returning array data from MEX functions.  Those arrays are either double/float or pairs of double/float used to represent complex numbers (and then, we only mix malloc/delete when using the new interleaved complex storage layout that is enabled when using the -R2018a option for compiling MEX files).

With double data, there are no destructors to call.  With complex, the destructors don't actually do anything.  I know it works for double arrays because I've tested that.  But it fails for complex.  Did I not test that?!?  I guess not.  I'm attaching a simple mex file to demonstrate.  Using stable Octave without my proposed changes, try


octave> mkoctfile -mex -R2018a freetest.c
octave> x = freetest ();
octave> clear x  ## EXPECT error and crash


Note that it should work if you don't use interleaved complex data (omit the -R2018a option or use -R2017b instead).

I'm also attaching a simpler stand-alone program that demonstrates the problem.  When I run it, I see:


$ g++ -O2 vec-dtor.cc
$ ./a.out
new + delete[] for double: ok
malloc + delete[] for double: ok
new + delete[] for complex: ok
malloc + delete for complex: ok
malloc + delete[] for complex: free(): invalid pointer
Aborted


If we could find a simple workaround for the visibility problem, I would have no objection to using the polymorphic allocator when using a compiler that supports it.

Otherwise, I think the best solution for version 7 is to only allow malloc and delete to be mixed when returning real-valued arrays from MEX functions.  For complex data, we can just go back to copying data even when using interleaved complex storage.

John W. Eaton <jwe>
Group administrator
Mon 06 Dec 2021 11:10:14 AM UTC, comment #26: 

By the looks of it, a compiler (or its preprocessor) can claim to be C++17 complete even if the C++ standard library it will be using doesn't implement all features.

Anyhow, I'm with jwe on this. It looks like a proper fix will be quite intrusive. We'd need to raise the minimum requirements quite significantly (libstdc++ version 9 or newer, no support for libc++ currently).
Alternatively, we'd need to introduce a new dependency on the boost libraries. (It needs confirmation if this is an actual option.)
In any case, it looks like the changes will be too significant to squeeze into Octave 7 without delaying its release (possibly substantially).

For Octave 7, should we just leave it as it is and be aware that the mex interface might work unexpectedly with certain combinations of compiler versions/C++ library versions/platforms/... at certain operations? As far as we currently can tell, a normal user might not notice anything about this on most (if not all?) platforms because the C++ standard libraries probably implement `delete` on top of `free`...

If we are using `delete[]` though, anything might happen ranging from memory leaks to freeing a huge amount of memory (resulting in loads of dangling references or crashing immediately). This is because `delete[]` seems to read the memory in front of where the pointer points to find how much memory to actually free. In general, that could be anything for malloc-ed data.
See e.g. this for how it is implemented in MS compilers (but I'm assuming other compilers don't do it much differently):
https://devblogs.microsoft.com/oldnewthing/20040203-00/?p=40763

IIUC, the oct interface isn't affected by any of this.

Markus Mützel <mmuetzel>
Group administrator
Mon 06 Dec 2021 02:02:52 AM UTC, comment #25: 

If currently C++17 cannot be adopted there are multiple polymorphic allocator implementations on web compatible with C++11 including:

1. https://github.com/phalpern/CppCon2017Code

It's Author proposed polymorphic allocators to standard.

2. http://man.hubwiz.com/docset/Boost.docset/Contents/Resources/Documents/boost/doc/html/container/cpp_conformance.html

Boost implementation.

Anonymous
Sun 05 Dec 2021 11:19:31 PM UTC, comment #24: 

@Dmitri: Markus had a technique for running only the MEX test here: https://octave.discourse.group/t/running-and-disabling-bists/1903/2 which is reproduced here:


make test/mex/bug_51725.mex
./run-octave --eval "addpath('../test/mex', 'test/mex'); test ../test/mex/bug-51725.tst"


ASAN definitely reports memory leaks but it's not clear if they're connected to MEX (probably not, since even when ASAN crashed at the Jupyter tests just before the MEX tests it still reported memory leaks. The Jupyter leaks are listed as being a possible false positive by ASAN itself, based on Octave's use of custom stack unwinding.)

Arun Giridhar <arungiridhar>
Group Member
Sun 05 Dec 2021 07:00:43 PM UTC, comment #23: 

The problem with a "normal" build might be a memory leak.
I am not sure how to run this test standalone
"test bug-54096.tst" does not work for me.
The idea is to do it many times in a loop and see if memory usage increases.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sun 05 Dec 2021 06:47:09 PM UTC, comment #22: 

@jwe in comment #19: Yes, changing the typedef to add the word "template", and changing INSTANTIATE_ARRAY_WITH_ALLOCATOR to INSTANTIATE_ARRAY both made Clang compile it fully without errors. I also did not need to add --disable-lib-visibility-flags  to configure. But it did give the malloc-vs-free warnings during compilation and the MEX test crashed ASAN. It might be because the #if check for C++17 works only for GCC and not Clang.

Re whether this should be fixed for 7.1 or not, it causes a problem only in ASAN checks and not in regular use (comment #2).

Arun Giridhar <arungiridhar>
Group Member
Sun 05 Dec 2021 04:03:47 PM UTC, comment #21: 

Mixing `malloc` and `delete` causes undefined behavior. So, no guarantees what could or could not happen for any platform. But a quick Google search didn't show any definite examples for platforms where that actually causes trouble. (I might not have used the correct search terms though.)

Markus Mützel <mmuetzel>
Group administrator
Sun 05 Dec 2021 03:43:39 PM UTC, comment #20: 

Perhaps it is best to defer fixing this problem until after version 7 is released and to make any changes related to it on the default branch?

Does mixing malloc and delete cause trouble for is in real life, at least for the systems we currently care about?  If so, then we could also change the MEX functions to fall back to copying data on those systems where it matters, at least for now.

John W. Eaton <jwe>
Group administrator
Sun 05 Dec 2021 03:16:36 PM UTC, comment #19: 

Arun, doeas it help if you change the typedef to


typedef typename Alloc_traits::template rebind_traits<T> T_Alloc_traits;


or does clang really not support this feature even though it apparently is claiming to be a c++17 compiler (based on the definition of __cplusplus that enables this code)?

Also, there was a typo in the cset-1-array-alloc.txt file that I posted.  Instead of INSTANTIATE_ARRAY_WITH_ALLOCATOR, it should have just been INSTANTIATE_ARRAY.

John W. Eaton <jwe>
Group administrator
Sun 05 Dec 2021 02:36:15 PM UTC, comment #18: 

Like Anonymous wrote in comment #14, libc++ (used by default when compiling with clang) doesn't implement support for polymorphic memory resources. So, that is expected to fail.
You might get more lucky with clang if you try and link to libstdc++.
See also:
https://en.cppreference.com/w/cpp/compiler_support/17

Markus Mützel <mmuetzel>
Group administrator
Sun 05 Dec 2021 02:30:12 PM UTC, comment #17: 

Yes, I was compiling with GCC 11.2. With GCC, adding --disable-lib-visibility-flags makes it build properly. Make check passes for the MEX tests with that flag.

With Clang 13 errors like these show whether or not --disable-lib-visibility-flags is used. The configure flags, CFLAGS, CXXFLAGS etc are all the same as with the GCC build:

In file included from ../libinterp/dldfcn/__delaunayn__.cc:53:
../liboctave/array/Array.h:139:36: error: use 'template' keyword to treat 'rebind_traits' as a dependent template name
    typedef typename Alloc_traits::rebind_traits<T> T_Alloc_traits;
                                   ^
                                   template


Arun Giridhar <arungiridhar>
Group Member
Sun 05 Dec 2021 09:39:54 AM UTC, comment #16: 

Your change e38202d3628d seems to have greatly reduced the number of warnings on the macOS runners about "direct access to global weak symbols" (see bug #59820). The remaining ones are mostly about instantiations involving `class Sparse<T>` and `class octave_int<T>` templates afaict.
Maybe if we take the same approach (dedicated headers to provide guarded forward declarations for those template classes), we could reduce the impact of that bug. Maybe that could also resolve the issue you are seeing when using the patches you provided here with enabled visibility flags.

Markus Mützel <mmuetzel>
Group administrator
Sat 04 Dec 2021 11:11:10 PM UTC, comment #15: 

RE: comment #13.  I'm using g++ 11 which has -std=c++17 as the default.  I see now that it was only working for me because I happened to have configured with --disable-lib-visibility-flags.  If I remove that configure option I see the same errors as you.  I'm looking at it, but I'm not sure what the correct fix is for that.  For me, it appears to be only the Arrays of octave_{u,}int{16,32,64}.  There aren't complaints about Arrays of bool, char, float, or double values, or for octave_{u,}int8.  Whatever the difference is for those types, I'm not seeing it yet.

John W. Eaton <jwe>
Group administrator
Sat 04 Dec 2021 08:03:35 PM UTC, comment #14: 

Polymorphic allocator. Good news! It is a big step forward. I guess it can be used to cheaply send nested structures and cells from mex to Octave also it can bring the opportunity to make Arrays much dynamic like std::vector i.e. supporting reserve, push_back and emplace_back having amortized constant complexity. Thanks!

I'm not sure but I think support of polymorphic allocator in clang libc++ is currently incomplete.

Anonymous
Sat 04 Dec 2021 07:25:30 PM UTC, comment #13: 

With the two patches, I am getting build errors like the following:


  CXXLD    src/octave-cli
  CXXLD    src/octave-gui
/usr/bin/ld: libinterp/.libs/liboctinterp.so: undefined reference to `Array<octave_int<unsigned short>, std::pmr::polymorphic_allocator<octave_int<unsigned short> > >::column(long) const'
/usr/bin/ld: libinterp/.libs/liboctinterp.so: undefined reference to `Array<octave_int<long>, std::pmr::polymorphic_allocator<octave_int<long> > >::checkelem(Array<long, std::pmr::polymorphic_allocator<long> > const&) const'
/usr/bin/ld: libinterp/.libs/liboctinterp.so: undefined reference to `Array<octave_int<unsigned int>, std::pmr::polymorphic_allocator<octave_int<unsigned int> > >::nnz() const'


I think it's trying to compile with C++11 instead of C++17. Does it pick up automatically or does it need to be forced via CXXFLAGS?

Arun Giridhar <arungiridhar>
Group Member
Sat 04 Dec 2021 04:05:18 PM UTC, comment #12: 

I would prefer to avoid adding function pointers to the Array classes.  Instead, I think we should extend them with template parameters that allow for custom allocators in the same way that the standard library containers do.

I'm attaching two changesets for discussion.

The first adds a template parameter to the Array class to allow allow custom allocators and uses the allocator class to manage storage allocation for the ArrayRep object.  In this change, the default allocator is std::allocator<T> so it will work with C++11.  But if you attempt to define a custom allocator for the MEX objects using this interface, you'll run into trouble because Array<double,std::allocator<double>> is a different type from Array<double,my_special_allocator> so storing the Array with the custom allocator in an octave_value object requires modifying the octave_value class to accept a new type.  We definitely don't want that.

The second change changes the default allocator for the Array class to be std::pmr::polymorphic_allocator<T> and modifies the Array<T,Alloc>::Array(T *data, size_t n) constructor to accept a allocator object.  With this interface, we don't end up with different types when using custom allocators so no changes are required for the octave_value container.  However, the change requires C++17 so I made the change conditional.  If you don't have C++17 features, you'll continue to mix malloc/delete.

Note that these changes don't add data to the Array classes.  The allocator objects don't have data, they just provide functions.

Things that remain to be done are

  • Add allocators template parameters to other containers in Octave like Sparse<T>
  • Add allocator parameters to other constructors
  • Move the definition of the custom MEX allocator to a separate file.


To try the two attached changesets, you'll need to be at revision http://hg.savannah.gnu.org/hgweb/octave/rev/e38202d3628d or newer

(file #52426, file #52427)

John W. Eaton <jwe>
Group administrator
Fri 03 Dec 2021 02:14:34 PM UTC, comment #11: 

Thank you, Anon. Here are Anon's changes rolled up into one hg diff (attached).

Make check does not crash for the MEX tests with these edits but ASAN indicates much heavier memory leakage than before, about 1.7 MB instead of 100 KB. I have not isolated the source yet.

(file #52421)

Arun Giridhar <arungiridhar>
Group Member
Fri 03 Dec 2021 01:32:25 AM UTC, comment #10: 

Attachments are limited to 4 files. Here is the 5th. Sorry for spam:


(file #52417)

Anonymous
Fri 03 Dec 2021 01:27:38 AM UTC, comment #9: 

Attachments here missed from the last post:


(file #52413, file #52414, file #52415, file #52416)

Anonymous
Fri 03 Dec 2021 01:23:31 AM UTC, comment #8: 

Regarding comment #6:

I fixed 4 files :

liboctave/array/Array.h 
liboctave/array/Sparse.h
liboctave/array/Sparse.cc
libinterp/corefcn/mex.cc

and prepared a git diff because I'm not sure how to prepare a proper hg patch. Please feel free to use the diff or use 4 fixed files to create a patch.

Anonymous
Thu 02 Dec 2021 04:54:32 PM UTC, comment #7: 

I will work on this problem.  I think the best solution is to add an allocator class as a new template argument for the Array<T> class, similar to the way that allocators are used in the C++ standard library.

John W. Eaton <jwe>
Group administrator
Thu 02 Dec 2021 03:23:26 PM UTC, comment #6: 

Some days ago I had tried incorporating the code from Anonymous in comment #4 but it was not clear. The variable T *m_data is part of the ArrayRep class but comment #4 adds it directly to the Array class. It was not clear which classes to modify with the custom destructor (Array or ArrayRep or something else).

@Anonymous from comment #4: if you are reading this, please consider adding a Mercurial patch.

Arun Giridhar <arungiridhar>
Group Member
Thu 02 Dec 2021 02:02:59 PM UTC, comment #5: 

While certain combinations of compilers and standard library implementations might work without issues when using `delete []` on `malloc`-ed memory, that might not be the case with different combinations on other platforms. IIUC, this is undefined behavior.

We should probably better fix this before the release of Octave 7.

Markus Mützel <mmuetzel>
Group administrator
Tue 23 Nov 2021 09:42:35 AM UTC, comment #4: 

Ah! Sorry, Something in the last post is wrong(using delete in lambda). Here is the corrected one:


using deleter_t = void (*)(void *);

template<typename T>
class Array
{
public:
  Array (const dim_vector& dv)
  : m_data (new T[dv.safe_numel ()])
  , m_dv (dv)
  , m_deleter ([](void *p){delete[] static_cast<T *>(p);})
  {}

  Array (T *buffer, const dim_vector& dv, deleter_t del)
  : m_data (buffer)
  , m_dv (dv)
  , m_deleter (del)
  {}

  ~Array()
  {
    m_deleter (m_data);
  }

private:
  T *m_data;
  dim_vector m_dv;
  deleter_t m_deleter;
};

// usage
double * buf = (double *) std::malloc (5 * sizeof (double));

Array<doudblr> arr (buf, dim_vector (1, 5), &std::free);


Anonymous
Tue 23 Nov 2021 06:42:44 AM UTC, comment #3: 

Hello there!
Regarding comment #1:
A custom deleter can be provided to the constructor of `Array`. The Array also needs a member of type deleter:


using deleter_t = void (*)(void *);

template<typename T>
class Array
{
public:
  Array (const dim_vector& dv)
  : m_data (new T[dv.safe_numel ()])
  , m_dv (dv)
  , m_deleter ([](void *p){::operatr delete[] (p);})
  {}

  Array (T *buffer, const dim_vector& dv, deleter_t del)
  : m_data (buffer)
  , m_dv (dv)
  , m_deleter (del)
  {}

  ~Array()
  {
    deleter (m_data);
  }

private:
  T *m_data;
  dim_vector m_dv;
  deleter_t m_deleter;
};


It can be used like this:


double * buf = (double *) std::malloc (5 * sizeof (double));

Array<doudblr> arr (buf, dim_vector (1, 5), &std::free);


Anonymous
Tue 23 Nov 2021 12:51:45 AM UTC, comment #2: 

This behavior only shows up in ASAN builds and not in regular use. Your comment indicates it does not break things elsewhere in Octave. Unless it is an indicator of future trouble (e.g. a future C++ standard makes it illegal to mix them), I feel it is probably not worth your time implementing these changes simply to placate ASAN. Thoughts? Do you think it's safe to ask ASAN to ignore the mismatch?

Arun Giridhar <arungiridhar>
Group Member
Mon 22 Nov 2021 08:31:28 PM UTC, comment #1: 

When I made this change http://hg.savannah.gnu.org/hgweb/octave/rev/b00ff462e0f2 I forgot that the mxMalloc, mxCalloc, mxRealloc, and mxFree actually use the corresponding C-library functions instead of C++ new and delete operators.  Hmm.  So I guess we need to either keep track of that in the Array classes or implement these mxArray memory management functions using the C++ new and delete operators.  Or is there a better solution?

John W. Eaton <jwe>
Group administrator
Sat 13 Nov 2021 11:21:14 AM UTC, original submission:  

There is a repeatable crash during the execution of mex/bug-51725.tst in a debug build, with AddressSanitizer complaining about "alloc-dealloc-mismatch (malloc vs operator delete [])". Does anyone else see this? Is it safe to ignore it by setting ASAN_OPTIONS="alloc_dealloc_mismatch=0"?

This is for Octave 7, hg id 6c7567bfac91.

Output attached.

Arun Giridhar <arungiridhar>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #52441:  freetest.c added by jwe (612B - text/x-csrc)
file #52442:  vec-dtor.cc added by jwe (2KiB - text/x-c++src)
file #52426:  cset-1-array-alloc.txt added by jwe (58KiB - text/plain)
file #52427:  cset-2-custom-mex-alloc.txt added by jwe (5KiB - text/plain)
file #52421:  anon_diff.patch added by arungiridhar (10KiB - text/x-patch)
file #52417:  bug61472.diff added by None (10KiB - application/octet-stream - pacht)
file #52413:  Array.h added by None (28KiB - text/plain - 4 modified files)
file #52414:  Sparse.h added by None (22KiB - text/plain - 4 modified files)
file #52415:  Sparse.cc added by None (89KiB - text/plain - 4 modified files)
file #52416:  mex.cc added by None (122KiB - text/plain - 4 modified files)
file #52267:  mex_crash added by arungiridhar (31KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dasergatskov (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by arungiridhar (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 17 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-01-03 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2021-12-16 mmuetzel StatusNone Ready For Test
    2021-12-06 jwe Attached File- Added freetest.c, #52441
        Attached File- Added vec-dtor.cc, #52442
    2021-12-04 jwe Attached File- Added cset-1-array-alloc.txt, #52426
        Attached File- Added cset-2-custom-mex-alloc.txt, #52427
    2021-12-03 arungiridhar Attached File- Added anon_diff.patch, #52421
    2021-12-03 None Attached File- Added bug61472.diff, #52417
    2021-12-03 None Attached File- Added Array.h, #52413
        Attached File- Added Sparse.h, #52414
        Attached File- Added Sparse.cc, #52415
        Attached File- Added mex.cc, #52416
    2021-12-02 mmuetzel Priority5 - Normal 7 - High
        Releasedev 7.0.90
        Operating SystemGNU/Linux Any
    2021-11-13 arungiridhar Attached File- Added mex_crash, #52267

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code