Bugs blocking the 6.1 release

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Bugs blocking the 6.1 release

John W. Eaton
Administrator
We have three bugs tagged as blockers for the 6.1 release:

   * GUI hangs on use of uiputfile
     https://savannah.gnu.org/bugs/index.php?52840
     (Apparently MacOS only?

   * Segmentation faults with clang when running the test suite
     https://savannah.gnu.org/bugs/index.php?57591
     (Intermittent failure that happens only with Clang?)

   * handles to private functions may fail after "clear functions"
     https://savannah.gnu.org/bugs/index.php?57439
     (Also revealed many function handle issues.)

I'm not sure what to do about the first two but I've been working on
function handle issues related to the third one.  I have a series of
pending changes that you can see here:

   https://hg.octave.org/octave-jwe

Here's a summary with additional commentary not included in the commit
messages.  By far, the largest change is the most recent one:

   * 64b7dedbc220
     wip: refactor octave_fcn_handle class

     Split the octave_fcn_handle class internally into several
sub-classes for the following types of function handles:

       - simple
       - scopedfunction
       - nested
       - classsimple
       - anonymous

     These changes and others leading up to this point were initially
motivated by bug #57439 (handles to private functions may fail after
"clear functions").  Investigating that bug revealed many more issues
with function handles.  A rewrite seemed like a better long-term
solution than simply fixing symptoms.

     This change is still a work in progress.  The most important
remaining task is to make saving and loading handles work for all types
of handles and file types (text, binary, hdf5, and mat5) but this has a
lower priority for me than making handles themselves work properly.

Other related changes leading up to this one are, in reverse
chronological order:

   * 4c35fd270522
     try harder to find functions in some symbol_table find_* functions

     If a function names is not already in the cache of known functions,
add it to the list and search again.  Some search functions already
worked this way, so this change makes the behavior more consistent.

   * c2a7460d1900
     new functions for finding scoped functions and class methods

     Moving these searches into separate functions helps to make
changeset 64b7dedbc220 (wip: refactor octave_fcn_handle class) simpler.

   * 7d3280c8373b
     move make_fcn_handle to tree_evaluator class

     The make_fcn_handle function belongs in the evaluator.  This helps
to make changeset 64b7dedbc220  (wip: refactor octave_fcn_handle class)
simpler.

   * a40c4af1c73d
     refactor handling of parent functions and localfunctions

     A step toward proper scopedfunction function handles.

   * e8b8f7ea102c
     new cellstring constructor

     Trivial change that helps with implementing changeset a40c4af1c73d.

   * 4c3873bf7b9a
     store local init vars for anonymous functions in handle, not
function object

     As with nested functions, the local variables associated with
anonymous functions should be stored in the function handle instead of
the function referenced by the handle.

   * 61fbce0b302b
     refactor octave_function call method

     This change is a step toward storing stack frames associated with
handles to nested functions in the function handle instead of the
function referenced by the handle.

   * 45227dd1dbb3
     don't document the vectorize function.

     Use of this function is discouraged.  It has limited usefulness,
even for the obsolete @inline class.

   * a90e8d0da19d
     convert obsolete octave_fcn_inline object to @inline class

     Use of @inline objects is discouraged and should be replaced by
anonymous functions.  Moving @inline into an M-file legacy class instead
of deriving it from the internal function handle object makes it easier
to remove later, and simplifies refactoring the internal function handle
object.

   * 6900f86908af
     use shared_ptr for stack frames in call stack and for accesss and
static links

     This makes it easy for us to grab references to subsets of stack
frames that are used to access values of non-local symbols in nested
functions.  Having shared access to these stack frames is necessary to
properly implement handles to nested functions.

   * f959268bca07
     hide specific stack frame and stack frame walker classes

     The specific stack frame classes are not needed explicitly so it
seems better to only manipulate them through the generic interface to
the stack_frame object.  This is a fairly minor change.

Another change that will eventually be needed is to modify function
handles to hold a weak_ptr reference to the actual function object.
That way, handles that are used after functions are cleared can noticed
that the function was cleared an properly attempt to reload them.
Ultimately, this change is the one that will correctly fix the bug that
motivated all of these other changes(!).  But until that is done, at
least Octave won't fail and the performance issue that Philip noticed on
Windows systems will be minimized.

I've been working on stable with the idea that these changes could be
applied to stable and included in the release.  I know that this is a
big set of changes to push at the last minute, but it seems best to me
to make handles to nested functions actually work properly since that
was one of the big changes that was supposed to happen in version 6.

A big positive from these changes is that the function handle
implementation is much clearer and more reliable than it was before as
well as being more compatible with Matlab behavior.  The way function
calls are performed has also improved.

This is a lot of information to digest in one message, so we can break
the discussion up in pieces if needed.  Either way, I welcome any comments.

jwe

Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

Dmitri A. Sergatskov


On Wed, May 13, 2020 at 2:19 PM John W. Eaton <[hidden email]> wrote:
We have three bugs tagged as blockers for the 6.1 release:
 

   * Segmentation faults with clang when running the test suite
     https://savannah.gnu.org/bugs/index.php?57591
     (Intermittent failure that happens only with Clang?)


I thought it was established that it fails with gcc as well. Looking at buildbot's history fedora-gcc
fails more often than clang (for dev version). The bug description was not updated. At this moment it is more likely
fail than pass.

Dmitri.
--


 
Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

mmuetzel
In reply to this post by John W. Eaton
Am 13. Mai 2020 um 20:18 Uhr schrieb "John W. Eaton":

> We have three bugs tagged as blockers for the 6.1 release:
>
>    * GUI hangs on use of uiputfile
>      https://savannah.gnu.org/bugs/index.php?52840
>      (Apparently MacOS only?
>
>    * Segmentation faults with clang when running the test suite
>      https://savannah.gnu.org/bugs/index.php?57591
>      (Intermittent failure that happens only with Clang?)
>
>    * handles to private functions may fail after "clear functions"
>      https://savannah.gnu.org/bugs/index.php?57439
>      (Also revealed many function handle issues.)
>
> I'm not sure what to do about the first two but I've been working on
> function handle issues related to the third one.  I have a series of
> pending changes that you can see here:
>
>    https://hg.octave.org/octave-jwe

I built from your repository and see 4 failing tests. All seem to be related to loading binary files:

>>>>> processing /home/osboxes/Documents/Repositories/Octave/octave-jwe/scripts/plot/util/hgload.m
***** test
 unwind_protect
   h1 = figure ("visible", "off");
   col = get (h1, "color");
   ftmp = [tempname() ".ofig"];
   hgsave (h1, ftmp);
   close (h1);
   [h2, old] = hgload (ftmp);
   assert (old, {[]});
   [h3, old] = hgload (ftmp, struct ("color", [1 0 0]));
   assert (get (h3, "color"), [1 0 0]);
   assert (iscell (old) && numel (old) == 1);
   assert (isstruct (old{1}) && isfield (old{1}, "color"));
   assert (old{1}.color, col);
 unwind_protect_cleanup
   unlink (ftmp);
   try, close (h1); end_try_catch
   try, close (h2); end_try_catch
   try, close (h3); end_try_catch
 end_unwind_protect
!!!!! test failed
load: trouble reading binary file ''


>>>>> processing /home/osboxes/Documents/Repositories/Octave/octave-jwe/scripts/plot/util/openfig.m
***** test
 unwind_protect
   h1 = figure ("visible", "off");
   ftmp = [tempname() ".ofig"];
   hgsave (h1, ftmp);
   close (h1);
   h2 = openfig (ftmp, "new", "invisible");
   h3 = openfig (ftmp, "reuse");
   assert (h2 == h3);
   close (h2);
 unwind_protect_cleanup
   unlink (ftmp);
   try, close (h1); end_try_catch
   try, close (h2); end_try_catch
   try, close (h3); end_try_catch
 end_unwind_protect
!!!!! test failed
load: trouble reading binary file ''

>>>>> processing /home/osboxes/Documents/Repositories/Octave/octave-jwe/.build/libinterp/octave-value/ov-fcn-handle.cc-tst
***** test <*33857>
 a = 2;
 f = @(x) a + x;
 g = @(x) 2 * x;
 hm = @version;
 hdld = @svd;
 hbi = @log2;
 f2 = f;
 g2 = g;
 hm2 = hm;
 hdld2 = hdld;
 hbi2 = hbi;
 modes = {"-text", "-binary"};
 if (isfield (__octave_config_info__, "HAVE_HDF5")
     && __octave_config_info__ ("HAVE_HDF5"))
   modes(end+1) = "-hdf5";
 endif
 for i = 1:numel (modes)
   mode = modes{i};
   nm = tempname ();
   unwind_protect
     f2 (1);
     save (mode, nm, "f2", "g2", "hm2", "hdld2", "hbi2");
     clear f2 g2 hm2 hdld2 hbi2
     load (nm);
     assert (f (2), f2 (2));
     assert (g (2), g2 (2));
     assert (g (3), g2 (3));
     unlink (nm);
     save (mode, nm, "f2", "g2", "hm2", "hdld2", "hbi2");
   unwind_protect_cleanup
     unlink (nm);
   end_unwind_protect
 endfor
!!!!! regression: https://octave.org/testfailure/?33857
load: trouble reading binary file '/tmp/oct-mmPcCN'
***** test <*35876>
 a = 2;
 f = @(x) a + x;
 g = @(x) 2 * x;
 hm = @version;
 hdld = @svd;
 hbi = @log2;
 f2 = f;
 g2 = g;
 hm2 = hm;
 hdld2 = hdld;
 hbi2 = hbi;
 modes = {"-text", "-binary"};
 if (isfield (__octave_config_info__, "HAVE_HDF5")
     && __octave_config_info__ ("HAVE_HDF5"))
   modes(end+1) = "-hdf5";
 endif
 for i = 1:numel (modes)
   mode = modes{i};
   nm = tempname ();
   unwind_protect
     fcn_handle_save_recurse (2, mode, nm, f2, g2, hm2, hdld2, hbi2);
     clear f2 g2 hm2 hdld2 hbi2
     [f2, f2, hm2, hdld2, hbi2] = fcn_handle_load_recurse (2, nm);
     load (nm);
     assert (f (2), f2 (2));
     assert (g (2), g2 (2));
     assert (g (3), g2 (3));
     unlink (nm);
     fcn_handle_save_recurse (2, mode, nm, f2, g2, hm2, hdld2, hbi2);
   unwind_protect_cleanup
     unlink (nm);
   end_unwind_protect
 endfor
!!!!! regression: https://octave.org/testfailure/?35876
load: trouble reading binary file '/tmp/oct-2qXYGZ'


I'll try and cross-compile for Windows next. I'll report if I should run into any issues or better if it fixes the performance issue.

> Another change that will eventually be needed is to modify function
> handles to hold a weak_ptr reference to the actual function object.
> That way, handles that are used after functions are cleared can noticed
> that the function was cleared an properly attempt to reload them.
> Ultimately, this change is the one that will correctly fix the bug that
> motivated all of these other changes(!).  But until that is done, at
> least Octave won't fail and the performance issue that Philip noticed on
> Windows systems will be minimized.
>
> I've been working on stable with the idea that these changes could be
> applied to stable and included in the release.  I know that this is a
> big set of changes to push at the last minute, but it seems best to me
> to make handles to nested functions actually work properly since that
> was one of the big changes that was supposed to happen in version 6.

I get your point that this is quite a large set of changes shortly before a release. But the performance on Windows - especially if Octave is installed on a HDD - is appalling at the moment.
So if this is necessary to get the interpreter to a state in which it is possible to actually work with it, I wouldn't mind to delay the release another few weeks (or whatever is necessary).

Markus

Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

mmuetzel
Am 14. Mai 2020 um 21:24 Uhr schrieb "Markus Mützel":
> I'll try and cross-compile for Windows next. I'll report if I should run into any issues or better if it fixes the performance issue.

Well, I didn't reach very far. When I tried to build the .lz source ball for hg id 64b7dedbc220 with "make all dist-lzip", I got the following error:
make[1]: *** No rule to make target 'scripts/legacy/@inline/module.mk', needed by 'distdir-am'.  Stop.
make[1]: Leaving directory '/home/osboxes/Documents/Repositories/Octave/octave-jwe/.build'
make: *** [Makefile:27530: distdir] Error 2

Markus

Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

John W. Eaton
Administrator
On 5/14/20 3:30 PM, "Markus Mützel" wrote:
> Am 14. Mai 2020 um 21:24 Uhr schrieb "Markus Mützel":
>> I'll try and cross-compile for Windows next. I'll report if I should run into any issues or better if it fixes the performance issue.
>
> Well, I didn't reach very far. When I tried to build the .lz source ball for hg id 64b7dedbc220 with "make all dist-lzip", I got the following error:
> make[1]: *** No rule to make target 'scripts/legacy/@inline/module.mk', needed by 'distdir-am'.  Stop.
> make[1]: Leaving directory '/home/osboxes/Documents/Repositories/Octave/octave-jwe/.build'
> make: *** [Makefile:27530: distdir] Error 2

Oops, thanks.  I haven't tried make dist or building from a tarball with
these changes.  I'll fix and push a new set of changes to the .  Since
I'd like to fix this at the point when the module.mk file was added,
this will require editing history so you'll need to strip these pending
changes and pull again from my semi-private repo.

jwe



Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

John W. Eaton
Administrator
In reply to this post by mmuetzel
On 5/14/20 3:30 PM, "Markus Mützel" wrote:

> Am 14. Mai 2020 um 21:24 Uhr schrieb "Markus Mützel":
>> I'll try and cross-compile for Windows next. I'll report if I should run into any issues or better if it fixes the performance issue.
>
> Well, I didn't reach very far. When I tried to build the .lz source ball for hg id 64b7dedbc220 with "make all dist-lzip", I got the following error:
> make[1]: *** No rule to make target 'scripts/legacy/@inline/module.mk', needed by 'distdir-am'.  Stop.
> make[1]: Leaving directory '/home/osboxes/Documents/Repositories/Octave/octave-jwe/.build'
> make: *** [Makefile:27530: distdir] Error 2
>
> Markus
>

Can you try again?  Strip the following changeset from your repo, pull
again from  https://hg.octave.org/octave-jwe and you should be able to
execute make dist and build from the resulting tar file.

   summary: convert obsolete octave_fcn_inline object to @inline class

jwe



Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

Daniel Sebald
In reply to this post by John W. Eaton
On 5/13/20 2:18 PM, John W. Eaton wrote:
> We have three bugs tagged as blockers for the 6.1 release:
>
>    * GUI hangs on use of uiputfile
>      https://savannah.gnu.org/bugs/index.php?52840
>      (Apparently MacOS only?

I came across a bug today in the editor "Highlight all occurrences"
feature that may not be a showstopper, but whew it is annoying:

https://savannah.gnu.org/bugs/index.php?58372

As I wrote the report, it looked as though it may be a simple fix.  I
will look into this over the weekend.  Would be nice to have that fixed
before the next major release.

Dan

Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

Daniel Sebald
On 5/15/20 12:34 AM, Daniel J Sebald wrote:

> On 5/13/20 2:18 PM, John W. Eaton wrote:
>> We have three bugs tagged as blockers for the 6.1 release:
>>
>>    * GUI hangs on use of uiputfile
>>      https://savannah.gnu.org/bugs/index.php?52840
>>      (Apparently MacOS only?
>
> I came across a bug today in the editor "Highlight all occurrences"
> feature that may not be a showstopper, but whew it is annoying:
>
> https://savannah.gnu.org/bugs/index.php?58372
>
> As I wrote the report, it looked as though it may be a simple fix.  I
> will look into this over the weekend.  Would be nice to have that fixed
> before the next major release.

Torsten,

Please review

https://savannah.gnu.org/bugs/index.php?58372

and consider patching for 6.1 release under the wire.

Thanks,

Dan

Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

mmuetzel
In reply to this post by John W. Eaton
Am 14. Mai 2020 um 22:22 Uhr schrieb "John W. Eaton":

> On 5/14/20 3:30 PM, "Markus Mützel" wrote:
> > Am 14. Mai 2020 um 21:24 Uhr schrieb "Markus Mützel":
> >> I'll try and cross-compile for Windows next. I'll report if I should run into any issues or better if it fixes the performance issue.
> >
> > Well, I didn't reach very far. When I tried to build the .lz source ball for hg id 64b7dedbc220 with "make all dist-lzip", I got the following error:
> > make[1]: *** No rule to make target 'scripts/legacy/@inline/module.mk', needed by 'distdir-am'.  Stop.
> > make[1]: Leaving directory '/home/osboxes/Documents/Repositories/Octave/octave-jwe/.build'
> > make: *** [Makefile:27530: distdir] Error 2
> >
> > Markus
> >
>
> Can you try again?  Strip the following changeset from your repo, pull
> again from  https://hg.octave.org/octave-jwe and you should be able to
> execute make dist and build from the resulting tar file.
>
>    summary: convert obsolete octave_fcn_inline object to @inline class

Thanks for the quick fix. I could successfully create the tarball with that change (hg id 178d101fd37d).

Cross-building the stable-octave target with MXE Octave fails with the following error:

/home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc: In constructor 'octave::simple_fcn_handle::simple_fcn_handle(const octave_value&, const string&)':
/home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:181:28: warning: declaration of 'octave_function* fcn' shadows a parameter [-Wshadow]
  181 |           octave_function *fcn = m_fcn.function_value ();
      |                            ^~~
/home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:176:44: note: shadowed declaration is here
  176 |     simple_fcn_handle (const octave_value& fcn, const std::string& name)
      |                        ~~~~~~~~~~~~~~~~~~~~^~~
/home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc: In constructor 'octave::scoped_fcn_handle::scoped_fcn_handle(const octave_value&, const string&, const std::__cxx11::list<std::__cxx11::basic_string<char> >&)':
/home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:1126:26: warning: declaration of 'octave_function* fcn' shadows a parameter [-Wshadow]
 1126 |         octave_function *fcn = m_fcn.function_value ();
      |                          ^~~
/home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:1117:61: note: shadowed declaration is here
 1117 |   scoped_fcn_handle::scoped_fcn_handle (const octave_value& fcn,
      |                                         ~~~~~~~~~~~~~~~~~~~~^~~
/home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc: In member function 'bool octave::anonymous_fcn_handle::parse(const string&)':
/home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:2253:18: warning: declaration of 'anonymous_fcn_handle' shadows a member of 'octave::anonymous_fcn_handle' [-Wshadow]
 2253 |     octave_value anonymous_fcn_handle
      |                  ^~~~~~~~~~~~~~~~~~~~
/home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:471:3: note: shadowed declaration is here
  471 |   {
      |   ^
/home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc: In member function 'virtual bool octave_fcn_handle::load_hdf5(octave_hdf5_id, const char*)':
/home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:2640:27: error: cannot bind non-const lvalue reference of type 'octave_hdf5_id&' {aka 'long long int&'} to an rvalue of type 'octave_hdf5_id' {aka 'long long int'}
 2640 |       if (afh->load_hdf5 (group_hid, space_hid, type_hid))
      |                           ^~~~~~~~~
/home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:2022:57: note:   initializing argument 1 of 'bool octave::anonymous_fcn_handle::load_hdf5(octave_hdf5_id&, octave_hdf5_id&, octave_hdf5_id&)'
 2022 |   bool anonymous_fcn_handle::load_hdf5 (octave_hdf5_id& group_hid,
      |                                         ~~~~~~~~~~~~~~~~^~~~~~~~~
make[5]: *** [Makefile:20696: libinterp/octave-value/liboctave_value_la-ov-fcn-handle.lo] Error 1


Some of those are warnings that I also see for native builds on Ubuntu Linux. But the error about the non-const lvalue seems to be Windows specific. I'm not sure why gcc doesn't mind on Linux though (maybe some standard extension?).

Markus


Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

John W. Eaton
Administrator
On 5/16/20 4:58 AM, "Markus Mützel" wrote:

> Am 14. Mai 2020 um 22:22 Uhr schrieb "John W. Eaton":
>> On 5/14/20 3:30 PM, "Markus Mützel" wrote:
>>> Am 14. Mai 2020 um 21:24 Uhr schrieb "Markus Mützel":
>>>> I'll try and cross-compile for Windows next. I'll report if I should run into any issues or better if it fixes the performance issue.
>>>
>>> Well, I didn't reach very far. When I tried to build the .lz source ball for hg id 64b7dedbc220 with "make all dist-lzip", I got the following error:
>>> make[1]: *** No rule to make target 'scripts/legacy/@inline/module.mk', needed by 'distdir-am'.  Stop.
>>> make[1]: Leaving directory '/home/osboxes/Documents/Repositories/Octave/octave-jwe/.build'
>>> make: *** [Makefile:27530: distdir] Error 2
>>>
>>> Markus
>>>
>>
>> Can you try again?  Strip the following changeset from your repo, pull
>> again from  https://hg.octave.org/octave-jwe and you should be able to
>> execute make dist and build from the resulting tar file.
>>
>>     summary: convert obsolete octave_fcn_inline object to @inline class
>
> Thanks for the quick fix. I could successfully create the tarball with that change (hg id 178d101fd37d).
>
> Cross-building the stable-octave target with MXE Octave fails with the following error:
>
> /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc: In constructor 'octave::simple_fcn_handle::simple_fcn_handle(const octave_value&, const string&)':
> /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:181:28: warning: declaration of 'octave_function* fcn' shadows a parameter [-Wshadow]
>    181 |           octave_function *fcn = m_fcn.function_value ();
>        |                            ^~~

[...]

I'll check out the warnings.  What version of GCC are you using that
produces these warnings?  I don't recall seeing them with GCC 8.

> /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc: In member function 'virtual bool octave_fcn_handle::load_hdf5(octave_hdf5_id, const char*)':
> /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:2640:27: error: cannot bind non-const lvalue reference of type 'octave_hdf5_id&' {aka 'long long int&'} to an rvalue of type 'octave_hdf5_id' {aka 'long long int'}
>   2640 |       if (afh->load_hdf5 (group_hid, space_hid, type_hid))
>        |                           ^~~~~~~~~
> /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:2022:57: note:   initializing argument 1 of 'bool octave::anonymous_fcn_handle::load_hdf5(octave_hdf5_id&, octave_hdf5_id&, octave_hdf5_id&)'
>   2022 |   bool anonymous_fcn_handle::load_hdf5 (octave_hdf5_id& group_hid,
>        |                                         ~~~~~~~~~~~~~~~~^~~~~~~~~
> make[5]: *** [Makefile:20696: libinterp/octave-value/liboctave_value_la-ov-fcn-handle.lo] Error 1
>
>
> Some of those are warnings that I also see for native builds on Ubuntu Linux. But the error about the non-const lvalue seems to be Windows specific. I'm not sure why gcc doesn't mind on Linux though (maybe some standard extension?).

Yeah, strange.  I'll try a Windows build and see if I can understand why
that's failing.

jwe

Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

mmuetzel
Am 16. Mai 2020 um 16:38 Uhr schrieb "John W. Eaton":

> On 5/16/20 4:58 AM, "Markus Mützel" wrote:
> > Cross-building the stable-octave target with MXE Octave fails with the following error:
> >
> > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc: In constructor 'octave::simple_fcn_handle::simple_fcn_handle(const octave_value&, const string&)':
> > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:181:28: warning: declaration of 'octave_function* fcn' shadows a parameter [-Wshadow]
> >    181 |           octave_function *fcn = m_fcn.function_value ();
> >        |                            ^~~
>
> [...]
>
> I'll check out the warnings.  What version of GCC are you using that
> produces these warnings?  I don't recall seeing them with GCC 8.

$ gcc --version
gcc (Ubuntu 9.3.0-10ubuntu2) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

In MXE Octave, gcc is at version 9.3.0, too.


> > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc: In member function 'virtual bool octave_fcn_handle::load_hdf5(octave_hdf5_id, const char*)':
> > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:2640:27: error: cannot bind non-const lvalue reference of type 'octave_hdf5_id&' {aka 'long long int&'} to an rvalue of type 'octave_hdf5_id' {aka 'long long int'}
> >   2640 |       if (afh->load_hdf5 (group_hid, space_hid, type_hid))
> >        |                           ^~~~~~~~~
> > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:2022:57: note:   initializing argument 1 of 'bool octave::anonymous_fcn_handle::load_hdf5(octave_hdf5_id&, octave_hdf5_id&, octave_hdf5_id&)'
> >   2022 |   bool anonymous_fcn_handle::load_hdf5 (octave_hdf5_id& group_hid,
> >        |                                         ~~~~~~~~~~~~~~~~^~~~~~~~~
> > make[5]: *** [Makefile:20696: libinterp/octave-value/liboctave_value_la-ov-fcn-handle.lo] Error 1
> >
> >
> > Some of those are warnings that I also see for native builds on Ubuntu Linux. But the error about the non-const lvalue seems to be Windows specific. I'm not sure why gcc doesn't mind on Linux though (maybe some standard extension?).
>
> Yeah, strange.  I'll try a Windows build and see if I can understand why
> that's failing.

Not sure if this will fix it, but can the input arguments be const references?

Markus


Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

Andrew Janke-2
In reply to this post by John W. Eaton


On 5/13/20 2:18 PM, John W. Eaton wrote:
> We have three bugs tagged as blockers for the 6.1 release:
>
>   * GUI hangs on use of uiputfile
>     https://savannah.gnu.org/bugs/index.php?52840
>     (Apparently MacOS only?

I believe this is macOS only. It's a bummer, but it's not a regression
(been around since 4.4.1 at least, I think), so while it'd be nice to
have a fix, IMHO it shouldn't be a release blocker.

Cheers,
Andrew


Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

mmuetzel
In reply to this post by John W. Eaton
Am 16. Mai 2020 um 16:38 Uhr schrieb "John W. Eaton":

> On 5/16/20 4:58 AM, "Markus Mützel" wrote:
> > Thanks for the quick fix. I could successfully create the tarball with that change (hg id 178d101fd37d).
> >
> > Cross-building the stable-octave target with MXE Octave fails with the following error:
> >
> > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc: In constructor 'octave::simple_fcn_handle::simple_fcn_handle(const octave_value&, const string&)':
> > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:181:28: warning: declaration of 'octave_function* fcn' shadows a parameter [-Wshadow]
> >    181 |           octave_function *fcn = m_fcn.function_value ();
> >        |                            ^~~
>
> [...]
>
> I'll check out the warnings.  What version of GCC are you using that
> produces these warnings?  I don't recall seeing them with GCC 8.
>
> > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc: In member function 'virtual bool octave_fcn_handle::load_hdf5(octave_hdf5_id, const char*)':
> > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:2640:27: error: cannot bind non-const lvalue reference of type 'octave_hdf5_id&' {aka 'long long int&'} to an rvalue of type 'octave_hdf5_id' {aka 'long long int'}
> >   2640 |       if (afh->load_hdf5 (group_hid, space_hid, type_hid))
> >        |                           ^~~~~~~~~
> > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:2022:57: note:   initializing argument 1 of 'bool octave::anonymous_fcn_handle::load_hdf5(octave_hdf5_id&, octave_hdf5_id&, octave_hdf5_id&)'
> >   2022 |   bool anonymous_fcn_handle::load_hdf5 (octave_hdf5_id& group_hid,
> >        |                                         ~~~~~~~~~~~~~~~~^~~~~~~~~
> > make[5]: *** [Makefile:20696: libinterp/octave-value/liboctave_value_la-ov-fcn-handle.lo] Error 1
> >
> >
> > Some of those are warnings that I also see for native builds on Ubuntu Linux. But the error about the non-const lvalue seems to be Windows specific. I'm not sure why gcc doesn't mind on Linux though (maybe some standard extension?).
>
> Yeah, strange.  I'll try a Windows build and see if I can understand why
> that's failing.
I think I finally understand why this is failing:
For native compilations on my Linux box, hid_t is typedef'ed as int64_t. In the version of the HDF5 headers that is included in MXE Octave, hid_t is typedef'ed as int. Like shown in the error message, octave_hdf5_id is likely typedef'ed as int64_t.
Thus, the types match for native builds on Linux. For Windows MXE builds, the compiler tries to create a temporary rvalue of matching type to call load_hdf5 and fails on binding the non-const lvalue reference to the temporary rvalue.
To be honest, the error message would have been a lot more helpful if it mentioned the type of hid_t...

I used the attached changes to fix that error and to also avoid the shadowed variable warnings.

I'll report back when the cross-compilation has finished and I could test the performance on Windows.

Markus

ov-fcn-handle_cross_compile.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Bugs blocking the 6.1 release

mmuetzel
Am 22. Mai 2020 um 16:15 Uhr schrieb "Markus Mützel":

> Am 16. Mai 2020 um 16:38 Uhr schrieb "John W. Eaton":
> > On 5/16/20 4:58 AM, "Markus Mützel" wrote:
> > > Thanks for the quick fix. I could successfully create the tarball with that change (hg id 178d101fd37d).
> > >
> > > Cross-building the stable-octave target with MXE Octave fails with the following error:
> > >
> > > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc: In constructor 'octave::simple_fcn_handle::simple_fcn_handle(const octave_value&, const string&)':
> > > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:181:28: warning: declaration of 'octave_function* fcn' shadows a parameter [-Wshadow]
> > >    181 |           octave_function *fcn = m_fcn.function_value ();
> > >        |                            ^~~
> >
> > [...]
> >
> > I'll check out the warnings.  What version of GCC are you using that
> > produces these warnings?  I don't recall seeing them with GCC 8.
> >
> > > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc: In member function 'virtual bool octave_fcn_handle::load_hdf5(octave_hdf5_id, const char*)':
> > > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:2640:27: error: cannot bind non-const lvalue reference of type 'octave_hdf5_id&' {aka 'long long int&'} to an rvalue of type 'octave_hdf5_id' {aka 'long long int'}
> > >   2640 |       if (afh->load_hdf5 (group_hid, space_hid, type_hid))
> > >        |                           ^~~~~~~~~
> > > /home/osboxes/Documents/Repositories/Octave/mxe-octave-stable/tmp-stable-octave/octave-6.0.1/libinterp/octave-value/ov-fcn-handle.cc:2022:57: note:   initializing argument 1 of 'bool octave::anonymous_fcn_handle::load_hdf5(octave_hdf5_id&, octave_hdf5_id&, octave_hdf5_id&)'
> > >   2022 |   bool anonymous_fcn_handle::load_hdf5 (octave_hdf5_id& group_hid,
> > >        |                                         ~~~~~~~~~~~~~~~~^~~~~~~~~
> > > make[5]: *** [Makefile:20696: libinterp/octave-value/liboctave_value_la-ov-fcn-handle.lo] Error 1
> > >
> > >
> > > Some of those are warnings that I also see for native builds on Ubuntu Linux. But the error about the non-const lvalue seems to be Windows specific. I'm not sure why gcc doesn't mind on Linux though (maybe some standard extension?).
> >
> > Yeah, strange.  I'll try a Windows build and see if I can understand why
> > that's failing.
>
> I think I finally understand why this is failing:
> For native compilations on my Linux box, hid_t is typedef'ed as int64_t. In the version of the HDF5 headers that is included in MXE Octave, hid_t is typedef'ed as int. Like shown in the error message, octave_hdf5_id is likely typedef'ed as int64_t.
> Thus, the types match for native builds on Linux. For Windows MXE builds, the compiler tries to create a temporary rvalue of matching type to call load_hdf5 and fails on binding the non-const lvalue reference to the temporary rvalue.
> To be honest, the error message would have been a lot more helpful if it mentioned the type of hid_t...
>
> I used the attached changes to fix that error and to also avoid the shadowed variable warnings.
>
> I'll report back when the cross-compilation has finished and I could test the performance on Windows.
Compilation finished successfully. The test suite results in a bunch of failed tests and a pair of regressions (mostly indexing related where a function is mistaken for an array, see the attached log file).
On Linux, I only see 2 failing tests and 2 regressions.

However, the performance difference is more than noticeable.
I attached "Very Sleepy" for about 2 minutes to what I think was the interpreter thread. "file_stat" still accounts for approx. 10% of the execution time (on a HDD). But that is very low compared to the about 80% from before the changes.

HTH,
Markus

fntests-jwe.log (4M) Download Attachment