bugGNU Octave - Bugs: bug #50463, gsvd implementation uses functions...

 
 

bug #50463: gsvd implementation uses functions removed from lapack >= 3.6.0

Submitter:  Carlo de Falco <cdf>
Submitted:  Sun 05 Mar 2017 06:50:25 PM UTC
   
 
Category:  Configuration and Build System Severity:  5 - Blocker
Priority:  5 - Normal Item Group:  Build Failure
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 09 Feb 2018 11:22:19 PM UTC, comment #75: 

It works for me now with Windows XP.

I pushed the change to configure.ac to allow building for win32 here (http://hg.savannah.gnu.org/hgweb/octave/rev/9af783c01c7f).

I pushed the most recent change to winqueryreg here (http://hg.savannah.gnu.org/hgweb/octave/rev/48f22c5b22aa).

Marking as fixed and closing report.

I will bring up John D.'s question about how an empty value should be treated (error or just "") on bug #52935.

Rik <rik5>
Group administrator
Fri 09 Feb 2018 08:34:08 PM UTC, comment #74: 

@Markus: I downloaded your latest patch.  Give me an hour or two to re-compile.

Rik <rik5>
Group administrator
Fri 09 Feb 2018 08:30:36 PM UTC, comment #73: 

Thanks jwe for the fix and sorry for the frequent changes.
I set up a virtual box with Windows XP and can confirm the first error. It looks like the maximum buffer size is one shorter than stated on the MSDN page (or I mis-interpreted).
@Rik: Before pushing again, could I ask you to please test with the attached patch? Hopefully for the last time.

The error to the second command is actually expected. There is no default value for that key. The error message with the patch is a bit more specific.

(file #43221)

Markus Mützel <mmuetzel>
Group administrator
Fri 09 Feb 2018 08:25:34 PM UTC, comment #72: 

Using win32 version:


>> VALUENAMES = winqueryreg ("name", "HKEY_CURRENT_USER",  "Environment")

VALUENAMES =
{
  [1,1] = TEMP
  [1,2] = TMP
  [1,3] = NO_XILINX_DATA_LICENSE
  [1,4] = Path
  [1,5] = CARBON_MEM_DISABLE
  [1,6] = OneDrive
}
>> winqueryreg ("HKEY_CURRENT_USER", "Environment", "temp")
ans = %USERPROFILE%\AppData\Local\Temp
>> winqueryreg ("HKEY_CURRENT_USER", "Environment")

error: winqueryreg: error 2 reading the specified key
>>



For the default value query, the default key is empty, so should probally return "" ? Not sure what matlab does in that case.

If I change the default value to something, it does return the value ok.

John Donoghue <lostbard>
Group Member
Fri 09 Feb 2018 01:15:06 AM UTC, comment #71: 

jwe's patch fixes the segfault so I think that is probably the way to go.  On the other hand, winqueryreg still doesn't do anything for me.  I used examples from the DOCSTRING.


>>  VALUENAMES = winqueryreg ("name", "HKEY_CURRENT_USER", ...
                                    "Environment");
error: winqueryreg: error 234 reading names from registry
>> winqueryreg ("HKEY_CURRENT_USER", "Environment")
error: winqueryreg: error 2 reading the specified key


If Windows XP is just too old to be supported then someone else will need to do the testing on Windows.

Rik <rik5>
Group administrator
Thu 08 Feb 2018 09:52:46 PM UTC, comment #70: 

For what it's worth, I was able to compile MXE Octave and then run it on Windows XP inside a virtual machine.  However, the function winqueryreg doesn't appear to work for me.  And I was able to get Octave to segfault when using the function.

I'll try jwe's patch and see if that helps.

Rik <rik5>
Group administrator
Thu 08 Feb 2018 08:25:18 PM UTC, comment #69: 

I'd prefer to avoid the reinterpret_cast on the function pointer.  I'd use something like the attached patch instead.



(file #43207)

John W. Eaton <jwe>
Group administrator
Thu 08 Feb 2018 07:07:49 PM UTC, comment #68: 

@Markus: I'm compiling MXE now with the patch contained in file #43160.  I'll let you know in a few hours whether it all works.

Rik <rik5>
Group administrator
Thu 08 Feb 2018 07:01:26 PM UTC, comment #67: 

Thanks, John for noticing and Rik for your tests. I hope this changeset finally fixes the issues with winqueryreg:
http://hg.savannah.gnu.org/hgweb/octave/rev/6a6108db3a6b

Should I push file #43160 as well?

Markus Mützel <mmuetzel>
Group administrator
Wed 07 Feb 2018 09:34:41 PM UTC, comment #66: 

I will refrain from testing the patch until the RegCloseKey issue is resolved.

Rik <rik5>
Group administrator
Wed 07 Feb 2018 08:51:54 PM UTC, comment #65: 

You are missing a RegCloseKey call in those last changes.

John Donoghue <lostbard>
Group Member
Wed 07 Feb 2018 07:18:55 PM UTC, comment #64: 

I pushed a changeset here: http://hg.savannah.gnu.org/hgweb/octave/rev/15daa0b34826

Those functions should be available in the WinAPI in Windows XP. But I only have a Windows 10 to test with.

Markus Mützel <mmuetzel>
Group administrator
Tue 06 Feb 2018 08:32:00 AM UTC, comment #63: 

Hmm. Seems like this function isn't available on Windows XP (apart from the very rare XP 64bit installations).
"RegQueryValueEx" is available in Windows 2000 Pro and newer. If I remember correctly, John D uses this function in his implementation of winqueryreg in the Windows package. I'll "borrow" from him again, I guess. :-)

Markus Mützel <mmuetzel>
Group administrator
Mon 05 Feb 2018 09:26:10 PM UTC, comment #62: 

@Marks: I tried your patch.  It did allow the MXE compile to complete.  But the resulting binary doesn't run on the Windows XP virtual machine I have.  When I try, I get an Error message box with


The procedre entry point RegGetValueA could not be located in the dynamic link library ADVAPI32.dll



Rik <rik5>
Group administrator
Sat 03 Feb 2018 04:01:01 PM UTC, comment #61: 

Rik, I think you are right: It might be best to just check for the presence of the header and, if it found, assume that the library exists as well. A quick google search revealed that other projects do the same.
I am attaching a patch that lets me successfully cross-build for the win32 target with MXE Octave again. But maybe someone you knows the build system better than me should check that it is correct.

(file #43160)

Markus Mützel <mmuetzel>
Group administrator
Sat 03 Feb 2018 11:04:53 AM UTC, comment #60: 

I'll let someone with more knowledge of the MXE cross-building environment speak up.  But, it should be possible to run a configure test which just checks that the include file psapi.h exists, but doesn't actually link against the library.  If psapi.h exists you could then add '-lpsapi' to LDFLAGS manually. 

However, this still might fail at the end of the build process when the octave executable is linked.

Rik <rik5>
Group administrator
Sat 03 Feb 2018 10:01:25 AM UTC, comment #59: 

The problem seems to be that win32 uses __stdcall calling convention. Win64 switched to a different calling convention that apparently works nicely with our configure test.
The number behind the @ is the number of bytes in the argument list. Here is where the config test is failing. Even if we manually inserted a "__stdcall" to the declaration, this number would probably be off.
The declaration of EnumProcessModules in psapi.h is:

  WINBOOL WINAPI EnumProcessModules(HANDLE hProcess,HMODULE *lphModule,DWORD cb,LPDWORD lpcbNeeded);


I don't know the build system very well. Maybe we could write a dedicated test that uses the necessary "#include"s. (I don't know how complicated that would be.)
Could we instead add -lpsapi to the link switches when compiling for Windows without any dedicated config test?

Markus Mützel <mmuetzel>
Group administrator
Sat 03 Feb 2018 09:15:05 AM UTC, comment #58: 

I started from scratch and configured MXE-Octave with:

./configure --enable-devel-tools --disable-windows-64 --enable-octave=default --enable-binary-packages --with-pkg-dir=../mxe-octave-pkg --with-ccache --disable-system-opengl --enable-qt5


Now, I am seeing the exact same error. The strange thing is, I do have "libpsapi.a" in "~/Documents/Repositories/Octave/mxe-octave-7/usr/i686-w64-mingw32/lib".
The relevant parts of nm -R of the 32bit and 64bit libraries are:
32bit:

dqcucs00004.o:
00000000 b .bss
00000000 d .data
00000000 T EnumProcessModules@16
         U _head_lib32_libpsapi_a
00000000 i .idata$4
00000000 i .idata$5
00000000 i .idata$6
00000000 i .idata$7
00000000 I _imp__EnumProcessModules@16
00000000 t .text


64bit:

dswsds00004.o:
0000000000000000 b .bss
0000000000000000 d .data
0000000000000000 T EnumProcessModules
                 U _head_lib64_libpsapi_a
0000000000000000 i .idata$4
0000000000000000 i .idata$5
0000000000000000 i .idata$6
0000000000000000 i .idata$7
0000000000000000 I __imp_EnumProcessModules
0000000000000000 t .text


I am not sure why the configure test is succeeding with the 64bit version but fails with the 32bit version. Might it be because of the decoration with an "@16" at the end of the exported name? If I understand correctly that doesn't mix well with the extern "C" in the config test:

[snipping off the pragmas]
| /* Override any GCC internal prototype to avoid an error.
|    Use char because int might match the return type of a GCC
|    builtin and then its argument prototype would still apply.  */
| #ifdef __cplusplus
| extern "C"
| #endif
| char EnumProcessModules ();
| #ifdef F77_DUMMY_MAIN
|
| #  ifdef __cplusplus
|      extern "C"
| #  endif
|    int F77_DUMMY_MAIN() { return 1; }
|
| #endif
| int
| main ()
| {
| return EnumProcessModules ();
|   ;
|   return 0;
| }


Markus Mützel <mmuetzel>
Group administrator
Sat 03 Feb 2018 12:15:01 AM UTC, comment #57: 

I'm sure the presence of libpsapi.a is why your build succeeds.  I guess I could go re-configure MXE for a 64 bit build and see if that generates the library.  John D. is on the CC list for this bug so maybe he will know why MXE is having trouble here.

Rik <rik5>
Group administrator
Fri 02 Feb 2018 11:58:08 PM UTC, comment #56: 

The buildbots seem to have the same problem.
On my local mxe-octave, however, the config test succeeds:

checking for EnumProcessModules in -lpsapi... yes


I also have "libpsapi.a" in "/home/markus/Documents/Repositories/Octave/mxe-octave-6/usr/x86_64-w64-mingw32/lib".

My config switches are:

./configure --enable-devel-tools --enable-windows-64 --enable-octave=default --enable-binary-packages --with-pkg-dir=../mxe-octave-pkg --with-ccache --disable-system-opengl --enable-qt5


The only difference to the buildbot I can see is that I have "--enable-windows64" instead of "--disable-windows64". Maybe that makes the difference?

Markus Mützel <mmuetzel>
Group administrator
Fri 02 Feb 2018 10:28:07 PM UTC, comment #55: 

I pushed this change (http://hg.savannah.gnu.org/hgweb/octave/rev/aa9c29f48718) which now checks for psapi only when compiling for a Windows host.

Rik <rik5>
Group administrator
Fri 02 Feb 2018 10:24:06 PM UTC, comment #54: 

Good news is that your changeset did fix the warning after 'make check' on Linux.

The FIXME is correct, we don't really want that to be run when we're not compiling for Windows.  But, it does no harm in the sense that the library isn't found and so the flag '-lpsapi' is never set.

The config.log shows


configure:68220: checking for EnumProcessModules in -lpsapi
configure:68253: i686-w64-mingw32-gcc -o conftest.exe -g -O2 -pthread -fopenmp -I/home/rik/wip/Projects_Mine/mxe-octave/usr/i686-w64-mingw32/include -Wl,-rpath-link,/home/rik/wip/Projects_Mine/mxe-octave/usr/i686-w64-mingw32/lib -L/home/rik/wip/Projects_Mine/mxe-octave/usr/i686-w64-mingw32/lib -L/home/rik/wip/Projects_Mine/mxe-octave/usr/i686-w64-mingw32/qt5/lib conftest.c -lpsapi  -lgdi32 -lm   >&5
/tmp/ccMLVLDh.o: In function `main':
/home/rik/wip/Projects_Mine/mxe-octave/tmp-default-octave/octave-4.3.0+/.build/conftest.c:564: undefined reference to `EnumProcessModules'


I do see a file /home/rik/wip/Projects_Mine/mxe-octave/usr/i686-w64-mingw32/include/psapi.h, but there is no equivalent psapi.lib which this Microsoft documentation refers to: https://msdn.microsoft.com/en-us/library/windows/desktop/ms682631(v=vs.85).aspx.



Rik <rik5>
Group administrator
Fri 02 Feb 2018 09:54:23 PM UTC, comment #53: 

I didn't read your last message in time.
From what I can tell from the MSDN that function should be available since Windows XP and should be available on both 32bit and 64bit systems [1].

It has to be linked in with "-lpsapi". I thought the following change to configure.ac would add that linker switch automatically:

@@ -1978,6 +1978,9 @@
   AC_SUBST(X11_LIBS)
 fi

+## FIXME: This check should probably be for Windows only.
+AC_CHECK_LIB([psapi], [EnumProcessModules])
+
 ### Check for the Carbon framework on macOS systems

 OCTAVE_HAVE_FRAMEWORK([Carbon],


Maybe I added those lines in the wrong place. But it worked for me.

It might also depend on the version of MinGW. I am using version 5.0.2.

[1] https://msdn.microsoft.com/de-de/library/windows/desktop/ms682631(v=vs.85).aspx

Markus Mützel <mmuetzel>
Group administrator
Fri 02 Feb 2018 09:39:45 PM UTC, comment #52: 

I pushed another change here:
http://hg.savannah.gnu.org/hgweb/octave/rev/69b89e81d3d3

Hopefully this fixes the issue.

Part of the problem was that I used a static dynamic_library object in gsvd.cc. That prevented deleting it when the reference count reached 0 (in particular the nil_rep). Not understanding the issue, I made the wrong "fix" to the destructors.

Markus Mützel <mmuetzel>
Group administrator
Fri 02 Feb 2018 09:38:51 PM UTC, comment #51: 

Re-opening this report.  Besides the issue in comment #50, I can no longer compile MXE version of Octave.  I am getting this result


  CXXLD    src/octave.exe
liboctave/util/.libs/libutil.a(liboctave_util_libutil_la-oct-shlib.o): In function `ZN6octave16octave_w32_shlib13global_searchERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE':
/home/rik/wip/Projects_Mine/mxe-octave/tmp-default-octave/octave-4.3.0+/liboctave/util/oct-shlib.cc:449: undefined reference to `EnumProcessModules@16'
/home/rik/wip/Projects_Mine/mxe-octave/tmp-default-octave/octave-4.3.0+/liboctave/util/oct-shlib.cc:456: undefined reference to `EnumProcessModules@16'
collect2: error: ld returned 1 exit status
Makefile:13253: recipe for target 'liboctave/liboctave.la' failed
make[4]: *** [liboctave/liboctave.la] Error 1
make[4]: *** Waiting for unfinished jobs....
libinterp/build-env-features.cc: In function 'octave_scalar_map octave::build_env::features()':
libinterp/build-env-features.cc:15:5: note: variable tracking size limit exceeded with -fvar-tracking-assignments, retrying without
     features (void)
     ^~~~~~~~


I am compiling for a 32-bit Windows XP target which may have something to do with it.

Rik <rik5>
Group administrator
Fri 02 Feb 2018 06:56:00 PM UTC, comment #50: 

@Markus: I'm seeing this warning after running 'make check'


Inconsistency detected by ld.so: dl-close.c: 811: _dl_close: Assertion `map->l_init_called' failed!


It started being printed with this changeset:


parent: 24663:ca416267c65f
 oct-shlib: move common property to base class.
branch: default


Could you investigate?

Rik <rik5>
Group administrator
Wed 31 Jan 2018 10:41:55 PM UTC, comment #49: 

Work fine on Windows too.
Thanks for fixing.

Closing report

Philip Nienhuis <philipnienhuis>
Group Member
Wed 31 Jan 2018 09:16:01 PM UTC, comment #48: 

Works fine for me as well now, that is make check passes on Linux with 1 known FAIL (reported bug).
I hope I can test already later tonight on Windows with run_test_suite_

Philip Nienhuis <philipnienhuis>
Group Member
Wed 31 Jan 2018 07:28:44 PM UTC, comment #47: 

Works for me now with the deprecated functions missing, tested both a 32-bit integer and 64-bit integer build of Octave.

Mike Miller <mtmiller>
Group Member
Wed 31 Jan 2018 01:17:33 AM UTC, comment #46: 
Markus Mützel <mmuetzel>
Group administrator
Wed 31 Jan 2018 01:01:30 AM UTC, comment #45: 

The development tree is where things occasionally break, and that is okay.  But we do try and keep the tree clean so it compiles at all times.  I would suggest you do patch the development branch immediately if it will stop segfaults that other developers are experiencing.  Long-term code re-organization can happen later.

Rik <rik5>
Group administrator
Wed 31 Jan 2018 12:49:33 AM UTC, comment #44: 

@Mike: Thank you very much for your help debugging. I didn't take the operator bool overload into account. Your approach fixes that.
Do you think we can apply the attached patch to fix the immediate issues and move search_all_loaded into the common rep base class later?

(file #43127)

Markus Mützel <mmuetzel>
Group administrator
Tue 30 Jan 2018 11:51:54 PM UTC, comment #43: 

Sure. With the default BLAS on Debian (with deprecated symbols included):


have_DGGSVD3: 1
have_DGGSVD: 1
have_DGGSVD123: 0


With a BLAS library built without deprecated symbols:


have_DGGSVD3: 1
have_DGGSVD: 0
have_DGGSVD123: 0


And with this change I get no segmentation fault, because it is no longer checking "if (! dyn_libs)".

I also made a local change to test, without this patch, with the following change instead, the correct function was found and used:


--- a/liboctave/util/oct-shlib.cc
+++ b/liboctave/util/oct-shlib.cc
@@ -182,7 +182,7 @@ namespace octave
     // normally throw an exception, avoiding the construction of an invalid
     // library.  Leave it here for possible future use.

-    bool is_open (void) const { return (library != nullptr); }
+    bool is_open (void) const { return (search_all_loaded || library != nullptr); }

   private:


There is no need to make the file property public, there is already a file_name accessor method in both the facade class and the rep base class.

Mike Miller <mtmiller>
Group Member
Tue 30 Jan 2018 11:31:38 PM UTC, comment #42: 

Philip or Mike: Could you please try with the attached patch and report what output you see before the segfault (if it still happens)?

(file #43124)

Markus Mützel <mmuetzel>
Group administrator
Tue 30 Jan 2018 10:51:46 PM UTC, comment #41: 

Sorry, I was looking at the gdb output wrong. The call to octave::dynamic_library ("") returns an octave::dynamic_library instance which has an octave_dlopen_shlib rep with count=1, file="", fcn_names set to an empty map, library=nullptr, and search_all_loaded=true.

But the next line "if (! dyn_libs)" returns true, so have_DGGSVD3 is set to false.

Maybe the various is_open methods should return true if search_all_loaded is set to true?

Should we move search_all_loaded into the common rep base class?

Mike Miller <mtmiller>
Group Member
Tue 30 Jan 2018 10:27:11 PM UTC, comment #40: 

I also get a segmentation fault on Debian using a BLAS built without deprecated symbols.

The initialize_gsvd function appears to not work on my system. The call to octave::dynamic_library ("") returns the nil_rep instance, have_DGGSVD3 is always false, and then attempting to call the old DGGSVD function results in a segmentation fault.

Mike Miller <mtmiller>
Group Member
Tue 30 Jan 2018 08:51:05 PM UTC, comment #39: 

After Markus' cset, I get a segfault during "make check"

:
  libinterp/corefcn/gsvd.cc-tst ...............................fatal: caught signal Segmentation fault -- stopping myself...
octave exited with signal 11


Mageia-6 64bit; gcc 5.4.0; lib64lapack 3.6.1

Until yesterday I had Carlo de Falco's original patch (comment #10) applied, that worked fine; tonight I started with a new clone.

How can I help further with debugging?

Philip Nienhuis <philipnienhuis>
Group Member
Tue 30 Jan 2018 07:43:51 PM UTC, comment #38: 

I made that change to avoid a double free on exit when a library wasn't actually loaded. But I can see why this was needed in the normal use case now.
I think I can provide a follow up patch shortly.

Markus Mützel <mmuetzel>
Group administrator
Tue 30 Jan 2018 06:44:26 PM UTC, comment #37: 

Your patch includes the following lines:


diff --git a/liboctave/util/oct-shlib.h b/liboctave/util/oct-shlib.h
--- a/liboctave/util/oct-shlib.h
+++ b/liboctave/util/oct-shlib.h
@@ -123,8 +123,7 @@ namespace octave

     ~dynamic_library (void)
     {
-      if (--rep->count == 0)
-        delete rep;
+      rep->count--;
     }

     dynamic_library (const dynamic_library& sl)


Doesn't that cause a memory leak when (for example) loading and unloading .oct files?  Why was this change made?

John W. Eaton <jwe>
Group administrator
Tue 30 Jan 2018 06:11:00 PM UTC, comment #36: 

Thanks for testing and reviewing. I pushed the patch here: http://hg.savannah.gnu.org/hgweb/octave/rev/f0de21a6a426

Markus Mützel <mmuetzel>
Group administrator
Mon 29 Jan 2018 07:58:36 PM UTC, comment #35: 

Markus, you have Mercurial privileges, do you want to check this in?

Rik <rik5>
Group administrator
Mon 29 Jan 2018 02:38:48 PM UTC, comment #34: 

Hi,

All test failures seem to be occurring in ode15{i,s} and are
due to SIP being enabled by default on macos High Sierra, so
it seems all gsvd tests are passing.

I also tried the test proposed in comment #33 directly and it
produces no errors.

Thanks!


Carlo de Falco <cdf>
Group Member
Tue 23 Jan 2018 08:57:07 PM UTC, comment #33: 

Carlo, thank you for testing the patch on macOS. Could you run this test (one of the BISTs) and see whether it gives any errors?

 A0 = randn (5, 3);
 B0 = diag ([1 2 4]);
 A = A0;
 B = B0;

## A (5x3) and B (3x3) are full rank
%!test <48807>
 [U, V, X, C, S, R] = gsvd (A, B);
 D1 = zeros (5, 3);  D1(1:3, 1:3) = C;
 D2 = S;
 assert (norm (diag (C).^2 + diag (S).^2 - ones (3, 1)) <= 1e-6);
 assert (norm ((U'*A*X) - D1*R) <= 1e-6);
 assert (norm ((V'*B*X) - D2*R) <= 1e-6);


Markus Mützel <mmuetzel>
Group administrator
Mon 22 Jan 2018 10:08:22 PM UTC, comment #32: 

I just tested the patch in file #42867 on
macOS High Sierra with clang++

With the patch applied on hg-id ab2321d4ba03
compilation works but make check produces 61 failures.

I'll investigate further to see if any failures are related to this bug.

Carlo de Falco <cdf>
Group Member
Sun 07 Jan 2018 09:35:37 PM UTC, comment #31: 

The last patch no longer compiles after the changes from: http://hg.savannah.gnu.org/hgweb/octave/rev/c5c11b07598a
I followed that changeset and also removed the interrupt handling.

I would appreciate if anyone affected by this bug could try the attached updated patch and report if it worked for them.

(file #42867)

Markus Mützel <mmuetzel>
Group administrator
Wed 03 Jan 2018 08:02:58 PM UTC, comment #30: 

The attached patch changes the dynamic_library class to search all loaded functions if the library name is empty.
I refrained from deleting all other interfaces apart from dlopen and LoadLibrary just yet. Maybe someone else can do that in the future...

I don't know whether the patch breaks gsvd on Mac and I can't test.
It should be working on Linux and Windows.

As jwe said on IRC, the check in configure.ac should probably be made conditional for Windows only. Any help appreciated.

(file #42812)

Markus Mützel <mmuetzel>
Group administrator
Wed 03 Jan 2018 06:00:49 PM UTC, comment #29: 

@Carlo: I might have misread your question: A test on build-time would probably be not too difficult. I thought the agreed goal was to have a check on run-time (or load-time).

Markus Mützel <mmuetzel>
Group administrator
Wed 03 Jan 2018 04:25:52 PM UTC, comment #28: 

@Markus, no problem at all! I was just thinking about it in theory, I would never have had time to actually do anything in practice so you are VERY welcome to work on this.

Anyway, if the approach I had in mind requires GCC it is also
not practical on macos as the default system compiler is now clang++.

thanks for answering my curiosity!
c.

Carlo de Falco <cdf>
Group Member
Wed 03 Jan 2018 04:14:21 PM UTC, comment #27: 

@Carlo: Sorry. I didn't realize you were also working on a solution. I should have asked earlier. Sorry for the double work.

From what I've read during the last few days, your approach could work using weak links to these functions (a gcc addition to the standard). Unfortunately, I haven't found an equivalent to weak links on Windows.

Markus Mützel <mmuetzel>
Group administrator
Wed 03 Jan 2018 03:45:36 PM UTC, comment #26: 

Hi,

It seems Markus has an almost finished solution so this
comment should be now out of date, but I had been thinking
of an alternative solution and, even though it will not be used,
 I still wonder whether that would have been feasible.

I was thinking we could ship with Octave sources a shared lib
containing xGGSVD3 from the reference LAPACK implementation and
tweak the build system so that it is overridden by the implementation in the linked lapack lib if there is one provided at build time or via LD_PRELOAD.

Would that be possible somehow?

c.

Carlo de Falco <cdf>
Group Member
Wed 03 Jan 2018 03:28:11 PM UTC, comment #25: 

A quick check seems to indicate that this actually might work. After compiling the attached file with "mkoctfile -lpsapi check_lib_fcn3.cc", I see the following with the liblapack.dll from Octave 4.2.1:

>> check_lib_fcn3 ('dggsvd_')
ans = 1
>> check_lib_fcn3 ('dggsvd3_')
ans = 0


Most of the build system is still a black box to me: How would psapi be added to the linked libraries? Or is it already?

(file #42808)

Markus Mützel <mmuetzel>
Group administrator
Wed 03 Jan 2018 03:24:13 PM UTC, comment #24: 

It looks like that could work for Windows.  Some care will apparently be needed to ensure that the output array is sized correctly, but I guess it is possible.  Weird interface with the length being specified in bytes..

This page https://developer.apple.com/library/content/qa/qa1180/_index.html says that dlopen has been part of Mac OS since 10.3 (released in 2003) and based on dyld calls since 10.4 (released in 2005) so I think it's OK to remove direct support for the dyld interface from Octave now.

A quick search shows that HP/UX did eventually get a dlopen interface so it might be OK to remove support for shl_load even if we still cared about support HP/UX.  But I don't recall anyone reporting a problem with HP/UX in many years, so I suspect no one has built current versions of Octave on HP systems in a decade or more.  Indeed, this page https://gcc.gnu.org/install/specific.html#hppa-hp-hpux says that although GCC does still support HP/UX, it requires explicit template instantiation, so it would be virtually impossible at this point to build Octave.  So I'd say we just declare HP/UX dead to Octave and sing songs of great joy on this glorious day.

 and there are only a few places in Octave that

John W. Eaton <jwe>
Group administrator
Wed 03 Jan 2018 02:53:11 PM UTC, comment #23: 

jwe: That is what I thought as well.
To change the dlsym-part of Octave's dynamic_library class shouldn't be too hard.
Unfortunately, "GetModuleHandle (0)" only works for the "main" process. Subsequent calls to "GetProcAddress" don't find functions linked in from loaded libraries. I was unsuccessful finding an equivalent to RTLD_GLOBAL for LoadLibrary (or GetModuleHandle).
Searching the loaded function list seems to be possible on HP/UX (but I have no way of testing this). Removing support for it would also simplify the code.
I haven't researched the DYLD_API (again, I have no way to test any changes). If this could be removed as well even better (from a developer's point of view).

I haven't tried. But for the Windows platform, it might be possible to query a list of all loaded libraries (which are called "modules" in Microsoft's speech from what I gather) with "EnumProcessModules" [1] on the current process and then check each module iteratively with "GetProcAdress" for an entry point to the queried function.

Should I give that a try?

[1] https://msdn.microsoft.com/de-de/library/windows/desktop/ms682631(v=vs.85).aspx

Markus Mützel <mmuetzel>
Group administrator
Wed 03 Jan 2018 01:42:08 PM UTC, comment #22: 

It appears that


GetModuleHandle (0);


will return a handle to the current process that may then be passed to GetProcAddress to search for a function.  Does that also work to search any DLLs that are already linked or loaded as dlopen (0) and dlsym does?  If so, then we could easily fix Octave's dynamic_library class to search for functions that are available in the current process.

I found this info here:

https://stackoverflow.com/questions/11741580/dlopen-loadlibrary-on-same-application

John W. Eaton <jwe>
Group administrator
Wed 03 Jan 2018 01:16:14 PM UTC, comment #21: 

Markus: about comment #19, I think we should extend the dynamic_library class to do what is needed instead of including dlfcn.h and calling dlsym directly.

Is there a way for the LoadLibrary API to check for functions that are already linked in the way that dlsym does?

Does HP/UX still exist anywhere in the wild?  If we no longer care about that system, then the SHL_LOAD_API bits could be removed.

What about the DYLD_API part?  How long has Mac OS had the dlopen API?  Maybe we can also remove that now?

John W. Eaton <jwe>
Group administrator
Tue 02 Jan 2018 11:14:22 PM UTC, comment #20: 

Thank you, Carlo, for providing a patch that implements how the new *ggsvd3 functions of LAPACK are used.
The attached patch checks on run-time whether these functions are available in the used LAPACK library and falls back to the *ggsvd functions if they are not.
It works for me on Linux and Windows where I tried with a recent liblapack.dll from MXE-Octave (that includes the *ggsvd3 functions) and one from 4.2.1 (that doesn't provide them).
Can someone please check that it also works as expected with a LAPACK library without the *ggsvd functions?

(file #42792)

Markus Mützel <mmuetzel>
Group administrator
Sat 23 Dec 2017 02:12:38 PM UTC, comment #19: 

With a cross build of the development branch (I don't remember which hg id of mxe-octave that is from, but it was built on 16 Dec 2017), I see:

>> check_lib_fcn ("liblapack", "dggsvd_")
ans = 1
>> check_lib_fcn ("liblapack", "dggsvd3_")
ans = 1


On another note: the "dynamic_library" class (in oct-shlib.cc) differentiates between four different mechanism to load shared libraries:

  dynamic_library::dynlib_rep *
  dynamic_library::dynlib_rep::new_instance (const std::string& f)
  {
#if defined (HAVE_DLOPEN_API)
    return new octave_dlopen_shlib (f);
#elif defined (HAVE_SHL_LOAD_API)
    return new octave_shl_load_shlib (f);
#elif defined (HAVE_LOADLIBRARY_API)
    return new octave_w32_shlib (f);
#elif defined (HAVE_DYLD_API)
    return new octave_dyld_shlib (f);
#else
    (*current_liboctave_error_handler)
      ("support for dynamically loaded libraries was unavailable or disabled when liboctave was built");
#endif


Is it save to unconditionally use the dlsym magic on Linux and Mac, and the "dynamic_library" class on Windows only?

OT: Does MXE Octave insert its hg id somewhere into the build? A text file somewhere in the packed folders would probably be enough to keep track of it.

Markus Mützel <mmuetzel>
Group administrator
Sat 23 Dec 2017 12:33:17 PM UTC, comment #18: 

Windows is using lapack 3.7.1 (at least in the repo), released June, 2017.

It has depreciated functions turned on that was allowing it to work ok with octave.

John Donoghue <lostbard>
Group Member
Fri 22 Dec 2017 08:55:24 PM UTC, comment #17: 

Yes, that would do it.  Is the answer correct on Windows using a development version?  I'm surprised we don't ship an lapack that is modern.

Rik <rik5>
Group administrator
Fri 22 Dec 2017 08:19:51 PM UTC, comment #16: 

Compiling the attached file with "mkoctfile check_lib_fcn.cc", I see the following on Octave 4.2.1 on Windows 10:

>> check_lib_fcn ("liblapack", "dggsvd_")
ans = 1
>> check_lib_fcn ("liblapack", "dggsvd3_")
ans = 0


And this on Ubuntu 17.10:

>> check_lib_fcn ("liblapack.so", "dggsvd_")
ans = 1
>> check_lib_fcn ("liblapack.so", "dggsvd3_")
ans = 1


@Rik: Is that the kind of portable check you were referring to in comment #13?
At least on Windows we should know the name of the library that we are packing. Not so sure on Linux or Mac.


(file #42706)

Markus Mützel <mmuetzel>
Group administrator
Sat 25 Nov 2017 09:42:12 AM UTC, comment #15: 

The attached patch applies smoothly to the current development branch.

(file #42478)

Carlo de Falco <cdf>
Group Member
Fri 24 Nov 2017 09:25:35 AM UTC, comment #14: 

Hi, I can help with updating the patch and even  provide runtime check for unix but if something windows specific needs to bee done I'm not really sure I can help.



Carlo de Falco <cdf>
Group Member
Thu 23 Nov 2017 11:57:08 PM UTC, comment #13: 

It appears that a runtime check can be implemented, but there will need to be separate code for Windows and UNIX-like operating systems.


#if defined (OCTAVE_USE_WINDOWS_API)
  // Windows stuff
#else
  // UNIX stuff
#endif


For the UNIX side, the code below should work to create a persistent variable that is evaluated only once and can then be used as a conditional around blocks of code.


static bool have_DGGSVD3 = (dlsym (RTLD_DEFAULT, STRINGIZE (F77_FUNC (dggsvd3, DGGSVD3))) == nullptr);


For Windows, it appears that a combination of LoadLibrary/GetProcAddress is necessary.  See StackOverflow article: https://stackoverflow.com/questions/42703922/windows-api-call-to-check-if-a-dll-contains-a-particular-procedure#42704262.

Finally, the patch from comment #10 would need to be updated.

Probably the prototypes for both DGGSVD and DGGSVD3 needs to be at the start of the file just so it will actually compile.  It also probably means compiling and linking the version of Octave that is exported, say through MXE, with lapack >= 3.6.0.


Rik <rik5>
Group administrator
Mon 05 Jun 2017 02:28:26 PM UTC, comment #12: 

Ping!

I hit this also when building in Mageia-6. The patch in comment #10 helps.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 03 May 2017 05:09:01 PM UTC, comment #11: 

Great.  I'll review whenever it is available.

Rik <rik5>
Group administrator
Wed 03 May 2017 01:33:10 PM UTC, comment #10: 

Here's the updated patch.

It was so simple to update it that I wonder why mercurial
could not work it out automatically ...

This still replaces dggsvd by dggsvd3 unconditionally,
I'll now try the approach suggested by Rik for checking
whether dggsvd3 is actually available before invoking it.

c.

(file #40572)

Carlo de Falco <cdf>
Group Member
Fri 28 Apr 2017 11:25:47 PM UTC, comment #9: 

What was the resolution on whether this should be a runtime or compile time check for DGGSVD3?

I do see Mike's point about people switching BLAS libraries in and out.  The MXE installer for Windows, for example, gives the user the option to use either OpenBLAS or refBLAS.

If it is a runtime test, then maybe promote the test to a static variable that is initialized just once at startup?

For example,


static bool have_DGGSVD3 = (dlsym (RTLD_DEFAULT, STRINGIZE (F77_FUNC (dggsvd3, DGGSVD3))) == nullptr);


Also, this looks good, but is there a way to make this portable to other OS like Windows which might not have dlsym?  Could you do a try/catch block?

Rik <rik5>
Group administrator
Fri 28 Apr 2017 03:59:40 PM UTC, comment #8: 

as of hg id 855122b993da the patch below does not apply anymore.

I'll be looking at how to adapt the patch, unless anyone else knows straight away how to fix it ...

Carlo de Falco <cdf>
Group Member
Sun 26 Mar 2017 02:16:13 PM UTC, comment #7: 

Mike,

I do see the point in your proposed approach.

Please notice, though, that, as noted in comment #1 , the change
is indeed in netlib lapack API and not only in OpenBlas:

http://www.netlib.org/lapack/lapack-3.6.0.html#_3_lapack_3_6_0_what_8217_s_new

It also seems the problem had already been noticed by Nir and Carnë who were supervising a related GSOC project :

https://bitbucket.org/BarbaraLocsi/octave
http://octave.1599824.n4.nabble.com/Deprecated-LAPACK-routines-GSVD-tp4679040.html

Also it appears that scipy have been discussing the same issue:

https://github.com/scipy/scipy/issues/5266

So assuming the BLAS/LAPACK API to be stable is not really such a safe choice anyway ...

c.



Carlo de Falco <cdf>
Group Member
Sat 25 Mar 2017 09:27:34 PM UTC, comment #6: 

I don't have a patch, but a safer alternative might be to detect whether the LAPACK library that Octave has already loaded has the newer function or not.

Something like


void *fcn = dlsym (RTLD_DEFAULT, STRINGIZE (F77_FUNC (dggsvd3, DGGSVD3)));
if (fcn)
  {
    // code to use DGGSVD3
  }
else
  {
    // code to use DGGSVD
  }


and of course need to adapt this to other operating systems.

Mike Miller <mtmiller>
Group Member
Sat 25 Mar 2017 09:20:41 PM UTC, comment #5: 

Not saying this is a bad change, but this has a possibility of unintended consequences. Until now, Octave was built against the BLAS / LAPACK stable API that hasn't changed in decades. One could build Octave against standard BLAS and LAPACK, and then switch to either ATLAS or OpenBLAS at run time.

If this change is applied, it's possible to build Octave against an old BLAS / LAPACK, and then try to use OpenBLAS at run time, which may exhibit the same run time error. Or it may be possible to build Octave against a LAPACK that does have the xGGSVD3 functions, and switch to a different one at run time that doesn't, resulting in a similar run time error.

On my system, NETLIB, ATLAS, and OpenBLAS all have the xGGSVD3 functions, and I don't know how likely it is to find a system that has a mix of libraries with and without this function, but it's possible.

Mike Miller <mtmiller>
Group Member
Sun 19 Mar 2017 01:37:23 PM UTC, comment #4: 

Thanks Rik,
I'll combine the two patches tomorrow on the train to Geneve,
hope we can push it during the code sprints.
c.

Carlo de Falco <cdf>
Group Member
Sun 19 Mar 2017 05:47:54 AM UTC, comment #3: 

@Carlo: Attached is a small piece of detection code for configure.ac which will set the #define HAVE_DDGSVD3 if it is available in the LAPACK library on your system.

This should be combined with your existing patch.  Everywhere in the code where there is a difference between DDGSVD3 and DDGSVD use this construct:


#ifdef HAVE_DDGSVD3
  // Code for LAPACK >= 3.6.0
#else
  // Existing code for LAPACK < 3.6.0
#endif




(file #40023)

Rik <rik5>
Group administrator
Tue 07 Mar 2017 10:29:55 PM UTC, comment #2: 

Actually the reason why the previous patch did not
work was just a stupid mistake from my side.

The new patch build and passes all tests on my system.


Still needs the autoconf part to be complete,
I could really use some help for that.




(file #39931)

Carlo de Falco <cdf>
Group Member
Tue 07 Mar 2017 10:38:16 AM UTC, comment #1: 

I changed the title to include the minimum version of lapack needed to show the issue, which does not occur with the openblas implementation only.

I am still investigating why my patch does not work, even
if it did work it would still need to include autoconf tests
to determine the available version of ggsvd functions in
order to preserve backwards compatibility.

Carlo de Falco <cdf>
Group Member
Sun 05 Mar 2017 06:50:25 PM UTC, original submission:  

Hi,

I'm building the default branch on macOS 10.12.3
using maports for dependencies and openblas for
blas/lapack.

The build fails at the linking stage with the error


Undefined symbols for architecture x86_64:   "_cggsvd_"


Looking at openblas sources, it appears that cggsvd is
deprecated and replaced by cggsvd3.

The attached patch solves the build problem for me but fails
at runtime with error:


** On entry to SGGSVD31PrecisionSafe Minimum parameter number 24 had an illegal value



Carlo de Falco <cdf>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #43221:  bug50463_winqueryreg_winxp.patch added by mmuetzel (2KiB - application/octet-stream)
file #43207:  diffs.txt added by jwe (755B - text/plain)
file #43160:  bug50463_ac_check_psapi.patch added by mmuetzel (978B - application/octet-stream)
file #43127:  bug50463_lapack_ggsvd3_v3_additum_v2.patch added by mmuetzel (3KiB - application/octet-stream)
file #43124:  bug50463_lapack_ggsvd3_v3_additum.patch added by mmuetzel (2KiB - application/octet-stream)
file #42867:  bug50463_lapack_ggsvd3_v3.patch added by mmuetzel (32KiB - application/octet-stream)
file #42812:  bug50463_lapack_ggsvd3_v2.patch added by mmuetzel (34KiB - application/octet-stream)
file #42808:  check_lib_fcn3.cc added by mmuetzel (2KiB - application/octet-stream)
file #42792:  bug50463_lapack_ggsvd3.patch added by mmuetzel (29KiB - application/octet-stream)
file #42706:  check_lib_fcn.cc added by mmuetzel (1KiB - application/octet-stream)
file #42478:  open_OazyWCsr.txt added by cdf (13KiB - text/plain)
file #40572:  open_sJ86Ow6X.txt added by cdf (13KiB - text/plain)
file #40023:  config.ac.diff added by rik5 (948B - application/x-download)
file #39931:  open_1VYMi0ZP.txt added by cdf (13KiB - text/plain)
file #39896:  open_F5jqLbmL.txt added by cdf (13KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by lostbard (Posted a comment)
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by cdf (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 25 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-02-09 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2018-02-09 mmuetzel Attached File- Added bug50463_winqueryreg_winxp.patch, #43221
        StatusIn Progress Patch Submitted
    2018-02-08 jwe Attached File- Added diffs.txt, #43207
    2018-02-08 mmuetzel StatusReady For Test In Progress
    2018-02-07 mmuetzel StatusIn Progress Ready For Test
    2018-02-03 mmuetzel Attached File- Added bug50463_ac_check_psapi.patch, #43160
    2018-02-02 rik5 StatusFixed In Progress
        Open/ClosedClosed Open
    2018-02-02 mmuetzel StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2018-02-02 rik5 StatusFixed In Progress
        Open/ClosedClosed Open
    2018-01-31 philipnienhuis StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-01-31 mmuetzel Attached File- Added bug50463_lapack_ggsvd3_v3_additum_v2.patch, #43127
    2018-01-30 mmuetzel Attached File- Added bug50463_lapack_ggsvd3_v3_additum.patch, #43124
    2018-01-30 mmuetzel StatusPatch Reviewed Ready For Test
    2018-01-29 rik5 StatusPatch Submitted Patch Reviewed
    2018-01-07 mmuetzel Attached File- Added bug50463_lapack_ggsvd3_v3.patch, #42867
    2018-01-03 mmuetzel Attached File- Added bug50463_lapack_ggsvd3_v2.patch, #42812
    2018-01-03 mmuetzel Attached File- Added check_lib_fcn3.cc, #42808
    2018-01-02 mmuetzel Attached File- Added bug50463_lapack_ggsvd3.patch, #42792
        StatusIn Progress Patch Submitted

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code