Opened 11 years ago

Closed 10 years ago

#1878 closed patch (fixed)

remove setjmp/longjmp in grass plugin&provider and use exceptions instead

Reported by: jef Owned by:
Priority: major: does not work as expected Milestone: Version 1.3.0
Component: GRASS Version: Trunk
Keywords: Cc: rblazek
Must Fix for Release: No Platform: All
Platform Version: Awaiting user input: no

Description


Attachments (2)

1878_grassexception.diff (21.7 KB) - added by jef 11 years ago.
except.tar (10.0 KB) - added by rblazek 11 years ago.
Exception from C catching test

Download all attachments as: .zip

Change History (27)

comment:1 Changed 11 years ago by jef

Component: Build/InstallGRASS
Milestone: Version 1.2.0Version 1.3.0

comment:2 Changed 11 years ago by jef

Priority: critical: causes crash or data corruptionminor: annoyance

comment:3 Changed 11 years ago by pcav

A side effect of this patch seems to be that GRASS vectors are not visible on the canvas, unless the editing is activated

comment:4 in reply to:  3 ; Changed 11 years ago by jef

Replying to pcav:

A side effect of this patch seems to be that GRASS vectors are not visible on the canvas, unless the editing is activated

The updated patch should fix this and #1900.

comment:5 Changed 11 years ago by lutra

Hi,

if I apply the patch I get the following error while compiling.

By the way I cannot compile under windows, so it would be hard in any case to say if the crash is gone or not.

In file included from /home/gio/Desktop/qgis_unstable/src/providers/grass/qgsgrassprovider.cpp:19:
/home/gio/Desktop/qgis_unstable/src/providers/grass/qgsgrass.h: In constructor ‘QgsGrass::Exception::Exception(const char*)’:
/home/gio/Desktop/qgis_unstable/src/providers/grass/qgsgrass.h:37: error: no matching function for call to ‘std::exception::exception(const char*&)’
/usr/include/c++/4.3/exception:59: note: candidates are: std::exception::exception()
/usr/include/c++/4.3/exception:57: note:                 std::exception::exception(const std::exception&)


make[2]: *** [src/providers/grass/CMakeFiles/qgisgrass.dir/qgsgrassprovider.o] Error 1
make[1]: *** [src/providers/grass/CMakeFiles/qgisgrass.dir/all] Error 2
make: *** [all] Error 2

Changed 11 years ago by jef

Attachment: 1878_grassexception.diff added

comment:6 in reply to:  4 ; Changed 11 years ago by lutra

Replying to jef:

The updated patch should fix this and #1900.

Hi Jurgen, is there problem applying this patch? I'm asking because I have a few colleagues using qgis-dev and they are hitting the #1900 problem. Thanks.

comment:7 in reply to:  6 ; Changed 11 years ago by jef

Replying to lutra:

is there problem applying this patch? I'm asking because I have a few colleagues using qgis-dev and they are hitting the #1900 problem. Thanks.

Probably not, but I don't know. I'm no GRASS user and therefore didn't do real testing. Did you test it?

comment:8 in reply to:  7 Changed 11 years ago by lutra

Replying to jef:

Replying to lutra:

is there problem applying this patch? I'm asking because I have a few colleagues using qgis-dev and they are hitting the #1900 problem. Thanks.

Probably not, but I don't know. I'm no GRASS user and therefore didn't do real testing. Did you test it?

I have applied the patch and I see no problem compiling and also using qgis/grass doing my normal tasks. #1900 is definitely gone (at least under linux).

comment:9 Changed 11 years ago by jef

Resolution: fixed
Status: newclosed

applied in r11560

comment:10 in reply to:  9 ; Changed 11 years ago by rblazek

Resolution: fixed
Status: closedreopened

Replying to jef:

applied in r11560

Are you sure it works after http://trac.osgeo.org/grass/changeset?old_path=grass%2Ftrunk%2Flib%2Fgis%2Ferror.c&old=23438&new_path=grass%2Ftrunk%2Flib%2Fgis%2Ferror.c&new=23439? For me exceptions cannot be caught after that change. The installed routine in QGIS is called, but then it terminates. It seems, that a function in a library in C cannot be interrupted by calling a function in C++ which throws an exception. Wasn't used setjmp/longjmp to solve it?

#5  0x0066f452 in std::terminate() () from /usr/lib/libstdc++.so.6
#6  0x0066f591 in __cxa_throw () from /usr/lib/libstdc++.so.6
#7  0x05d6eed7 in QgsGrass::openMapset(QString, QString, QString) ()
   from /home/radim/apps/lib/libqgisgrass.so.1.5.0
#8  0x07c842e4 in ?? () from /usr/lib/grass64/lib/libgrass_gis.so
#9  0x07c848f2 in G_fatal_error () from /usr/lib/grass64/lib/libgrass_gis.so
#10 0x01fea6d5 in ?? () from /usr/lib/grass64/lib/libgrass_vect.so
#11 0x01feb225 in Vect__open_old () from /usr/lib/grass64/lib/libgrass_vect.so
#12 0x01feb77c in Vect_open_old_head () from /usr/lib/grass64/lib/libgrass_vect.so
#13 0x037ea4a1 in QgsGrassSelect::vectorLayers(QString, QString, QString, QString) ()
   from /home/radim/apps/lib/qgis/libgrassplugin.so

I dont understand how it can come to QgsGrass::openMapset however.

comment:11 Changed 11 years ago by rblazek

Cc: rblazek added

comment:12 in reply to:  10 Changed 11 years ago by jef

Replying to rblazek:

Replying to jef:

applied in r11560

Are you sure it works after http://trac.osgeo.org/grass/changeset?old_path=grass%2Ftrunk%2Flib%2Fgis%2Ferror.c&old=23438&new_path=grass%2Ftrunk%2Flib%2Fgis%2Ferror.c&new=23439? For me exceptions cannot be caught after that change. The installed routine in QGIS is called, but then it terminates. It seems, that a function in a library in C cannot be interrupted by calling a function in C++ which throws an exception. Wasn't used setjmp/longjmp to solve it?

I just verified that it works as expected on Windows. I'll try on linux in the evening.

comment:13 Changed 11 years ago by rblazek

Previous backtrace is probably invalid, maybe old core. The following backtrace makes sense ,QgsGrass::error_routine is called, exception is thrown but it is not caught and program is terminated by std::terminate().

#5  0x09859452 in std::terminate() () from /usr/lib/libstdc++.so.6
#6  0x09859591 in __cxa_throw () from /usr/lib/libstdc++.so.6
#7  0x02dd2042 in QgsGrass::error_routine (
    msg=0xbfb46078 "Unable to open vector map <fiumi_buf@pok> on level 2. Try to rebuild vector topology by v.build.", fatal=1) at /home/radim/devel/qgis/src/providers/grass/qgsgrass.cpp:401
#8  0x07b412e4 in ?? () from /usr/lib/grass64/lib/libgrass_gis.so
#9  0x07b418f2 in G_fatal_error () from /usr/lib/grass64/lib/libgrass_gis.so
#10 0x058956d5 in ?? () from /usr/lib/grass64/lib/libgrass_vect.so
#11 0x05896225 in Vect__open_old () from /usr/lib/grass64/lib/libgrass_vect.so
#12 0x0589677c in Vect_open_old_head () from /usr/lib/grass64/lib/libgrass_vect.so
#13 0x044f3a43 in QgsGrassSelect::vectorLayers (gisdbase=..., location=..., mapset=..., mapName=...)
    at /home/radim/devel/qgis/src/plugins/grass/qgsgrassselect.cpp:416

comment:14 in reply to:  13 Changed 11 years ago by jef

Replying to rblazek:

Previous backtrace is probably invalid, maybe old core. The following backtrace makes sense ,QgsGrass::error_routine is called, exception is thrown but it is not caught and program is terminated by std::terminate().

> #5  0x09859452 in std::terminate() () from /usr/lib/libstdc++.so.6
> #6  0x09859591 in __cxa_throw () from /usr/lib/libstdc++.so.6
> #7  0x02dd2042 in QgsGrass::error_routine (
>     msg=0xbfb46078 "Unable to open vector map <fiumi_buf@pok> on level 2. Try to rebuild vector topology by v.build.", fatal=1) at /home/radim/devel/qgis/src/providers/grass/qgsgrass.cpp:401
> #8  0x07b412e4 in ?? () from /usr/lib/grass64/lib/libgrass_gis.so
> #9  0x07b418f2 in G_fatal_error () from /usr/lib/grass64/lib/libgrass_gis.so
> #10 0x058956d5 in ?? () from /usr/lib/grass64/lib/libgrass_vect.so
> #11 0x05896225 in Vect__open_old () from /usr/lib/grass64/lib/libgrass_vect.so
> #12 0x0589677c in Vect_open_old_head () from /usr/lib/grass64/lib/libgrass_vect.so
> #13 0x044f3a43 in QgsGrassSelect::vectorLayers (gisdbase=..., location=..., mapset=..., mapName=...)
>     at /home/radim/devel/qgis/src/plugins/grass/qgsgrassselect.cpp:416

I get:

Debug: /home/fischer/src/qgis/qgis_unstable/src/providers/grass/qgsgrass.cpp: 734: (vectors) mapsetPath = /home/fischer/test/grass/spearfish60/user1
Debug: /home/fischer/src/qgis/qgis_unstable/src/plugins/grass/qgsgrassselect.cpp: 340: (setLayers) setLayers()
Debug: /home/fischer/src/qgis/qgis_unstable/src/providers/grass/qgsgrass.cpp: 335: (setLocation) gisdbase = /home/fischer/test/grass location = spearfish60
Debug: /home/fischer/src/qgis/qgis_unstable/src/plugins/grass/qgsgrassselect.cpp: 432: (vectorLayers) GRASS vector successfully opened
Debug: /home/fischer/src/qgis/qgis_unstable/src/plugins/grass/qgsgrassselect.cpp: 340: (setLayers) setLayers()
Debug: /home/fischer/src/qgis/qgis_unstable/src/providers/grass/qgsgrass.cpp: 335: (setLocation) gisdbase = /home/fischer/test/grass location = spearfish60
Debug: /home/fischer/src/qgis/qgis_unstable/src/providers/grass/qgsgrass.cpp: 394: (error_routine) error_routine (fatal = 1): Unable to open vector map <fiumi_buf@user1> on level 2. Try to rebuild vector topology by v.build.
Debug: /home/fischer/src/qgis/qgis_unstable/src/plugins/grass/qgsgrassselect.cpp: 421: (vectorLayers) Cannot open GRASS vector: Unable to open vector map <fiumi_buf@user1> on level 2. Try to rebuild vector topology by v.build.

So it looks like the exception is thrown and catched. But this is with r12751 - although I don't think that deriving from QgsException instead of std::runtime_error made it work.

comment:15 Changed 11 years ago by rblazek

Priority: minor: annoyancemajor: does not work as expected

For me it is still crashing, the exception is not caught. I tried to call a GRASS function and catch exception in libqgisgrass and also both moved to plugin, but it does not work. I have also recompiled GRASS 6.4 from source but nothing helped.

It does not work even with reverted http://trac.osgeo.org/grass/changeset?old_path=grass%2Ftrunk%2Flib%2Fgis%2Ferror.c&old=23438&new_path=grass%2Ftrunk%2Flib%2Fgis%2Ferror.c&new=23439

The change in GRASS above probably is not significant, G_fatal_error does not continue in execution after the installed error_routine was called, but the exception cannot be caught.

Kbuntu 9.10, gcc (Ubuntu 4.4.1-4ubuntu8) 4.4.1, grass-6.4.0RC5, GDAL svn trunk, QGIS svn trunk, libstdc++6-4.4-dbg

comment:16 Changed 11 years ago by rblazek

jef, I have created very simple test, attached (except.tar). Could you please try to just run 'make' and './test'? For me it does not work:

./test
set_error_routine() start
set_error_routine() end
error_cpp called
caught int from error_cpp
error() start
error_routine called
terminate called after throwing an instance of 'int'
Aborted (core dumped)

Changed 11 years ago by rblazek

Attachment: except.tar added

Exception from C catching test

comment:17 in reply to:  16 Changed 11 years ago by jef

Replying to rblazek:

jef, I have created very simple test, attached (except.tar). Could you please try to just run 'make' and './test'? For me it does not work:

Odd. Using GCC 4.4.2 it works on 64bit, but fails on 32bit.

comment:18 Changed 11 years ago by rblazek

The problem is that C code must be compiled with -fexceptions, -fno-exceptions is probably default for gcc on some platforms.

comment:20 Changed 11 years ago by rblazek

It seems that we are back at the beginning of this ticket. GRASS developer seem to be reluctant to compile with -fexception and using setjmp/longjmp was suggested http://lists.osgeo.org/pipermail/grass-dev/2010-January/048086.html

For curiosity, were there particular problems with setjmp/longjmp?

In any case, I think that using exceptions is much better, I don't want to return to setjmp/longjmp.

comment:21 in reply to:  20 ; Changed 11 years ago by jef

Replying to rblazek:

It seems that we are back at the beginning of this ticket. GRASS developer seem to be reluctant to compile with -fexception and using setjmp/longjmp was suggested http://lists.osgeo.org/pipermail/grass-dev/2010-January/048086.html

I think we shouldn't try to intercept fatal GRASS errors in the GRASS library, we shouldn't be using GRASS libraries in the first place.

We should call GRASS modules (like g.gisenv) and parse their output. I think that's Glynn's point in making it harder to use GRASS libraries for purposes they are not meant for.

And I think that's also what the other GUI frontends do. But that probably is a major rewrite of the plugin.

For curiosity, were there particular problems with setjmp/longjmp?

In any case, I think that using exceptions is much better, I don't want to return to setjmp/longjmp.

None that I know of. Just uglyness.

comment:22 Changed 11 years ago by jef

Owner: jef deleted
Status: reopenednew

comment:23 in reply to:  21 ; Changed 11 years ago by jef

Replying to jef:

I think we shouldn't try to intercept fatal GRASS errors in the GRASS library, we shouldn't be using GRASS libraries in the first place.

IIRC the grass libraries also check for the grass version. So using the libraries directly also introduces a dependency to the exact GRASS version the plugin was build with.

comment:24 in reply to:  23 Changed 11 years ago by rblazek

Replying to jef:

IIRC the grass libraries also check for the grass version. So using the libraries directly also introduces a dependency to the exact GRASS version the plugin was build with.

I am writing GRASS raster provider which is an experiment using 'GRASS' modules instead of libs. I decided however to use new modules compiled within QGIS for more reasons:

  • There is no great discipline of GRASS modules options/output stability
  • The output from GRASS modules is not always suitable
  • If something in GRASS changes, it is better IMO to get compilation error instead of silently wrong results because an output format has changed
  • Changes in library are much less probable than those in modules because usually involve a lot of work because hundreds of modules depend on them. Anybody can however change a module in few minutes.

We will see which problems it brings us.

comment:25 Changed 10 years ago by rblazek

Resolution: fixed
Status: newclosed

I have added exceptions support as requirement for GRASS libs.

Note: See TracTickets for help on using tickets.