Giac miscompiles on non-x86_64 platforms

[jpflori]

Executing 1+1 with giac-1.2.2.103 on ppc64le POWER8:

0>> 1+1;
[New Thread 0x3fff7443f180 (LWP 173710)]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x3fff7443f180 (LWP 173710)]
0x00003fffb7b9e544 in giac::gen::in_eval (this=0x3fff7443e768, level=25, evaled=..., contextptr=0x3fffffffda40) at gen.cc:2109
2109              evaled=(*_SYMBptr->sommet.ptr())(evaled,contextptr);
(gdb) bt
#0  0x00003fffb7b9e544 in giac::gen::in_eval (this=0x3fff7443e768, level=25, evaled=..., contextptr=0x3fffffffda40) at gen.cc:2109
#1  0x00003fffb7b9bcdc in giac::gen::eval (this=0x3fff7443e768, level=25, contextptr=0x3fffffffda40) at gen.cc:1899
#2  0x00003fffb789e6ac in giac::protecteval (g=..., level=25, contextptr=0x3fffffffda40) at prog.cc:6881
#3  0x00003fffb767ff44 in giac::in_thread_eval (arg=0x100a9cd8) at global.cc:3463
#4  0x00003fffb5b18070 in start_thread () from /lib/powerpc64le-linux-gnu/libpthread.so.0
#5  0x00003fffb5763230 in clone () from /lib/powerpc64le-linux-gnu/libc.so.6
(gdb) list
2104              evaled=_SYMBptr->feuille;
2105            }
2106            if (is_ifte)
2107              evaled=ifte(evaled,true,contextptr);
2108            else
2109              evaled=(*_SYMBptr->sommet.ptr())(evaled,contextptr);
2110            elevel=slevel;
2111          }
2112          else
2113            evaled=_SYMBptr->eval(level,contextptr);

Upgrading to giac-1.2.3.25 (#22101) does not fix this problem.

---

Use > -45 version:
The upstream tarball is at http://www-fourier.ujf-grenoble.fr/~parisse/debian/dists/stable/main/source/giac_1.2.3-47.tar.gz
Repackaged tarball is at:

• http://perso.telecom-paristech.fr/~flori/giac-1.2.3.47.tar.bz2
  And pass -Dx86_64 in CPPFLAGS.

Depends on #22101

Upstream: Fixed upstream, in a later stable release.

CC: @rwst @frederichan-IMJPRG @vbraun

Component: packages: standard

Author: Jean-Pierre Flori

Branch: 8dc4c88

Reviewer: Thierry Monteil, Jean-Pierre Flori, François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/22280

[kiwifb]

comment:1

I can confirm the issue. The trick fells very dirty but I can easily test it I think.

[kiwifb]

comment:2

Has expected passing -D__x86_64__ -DSMARTPTR64 choke the build. Fails as soon as we load gmp headers which complain about the ABI being unsupported.
Now by default the thing builds but fails in calling pari. We could temporarily disable building against pari on non x86 (--disable-pari in giac-1.2.3-13).

More fundamentally I suspect the way pari is called is what is really wrong here, as discussed on sage-packaging https://groups.google.com/d/msgid/sage-packaging/ac9575ae-4dd3-4d29-97aa-a3ef785d3509%40googlegroups.com

[frederichan-IMJPRG]

comment:3

I don't know if it helps but the 1.2.3.25 version such as in #22101 contains the modifications for pari asked there:

http://xcas.e.ujf-grenoble.fr/XCAS/viewtopic.php?f=4&t=1776

[kiwifb]

comment:4

Still failing. Those changes are, I think are a progress but not the full changes that are needed - not that I would really be able to do them myself. The fact that we still have

#ifdef ENABLE_TLS
  extern THREAD void *PARI_stack_limit;
#else
   extern void *PARI_stack_limit;
#endif

which is a private function is a sign you are doing something wrong.

[jdemeyer]

comment:5

See http://pari.math.u-bordeaux.fr/archives/pari-users-1702/msg00002.html for a suggestion from the PARI developers on how this should be fixed.

[jdemeyer]

comment:6

(1) What does this refer to:

One can maybe cheat and pass -D__x86_64__ -DSMARTPTR64 in CPPFLAGS but that's dirty, see:

• http://xcas.e.ujf-grenoble.fr/XCAS/viewtopic.php?f=19&t=1723

(2) From the comments it seems that you think that there is a problem with PARI. Why do you think that?

[jdemeyer]

comment:8

Note that compiling with -D__x86_64__ obviously does not work due to the use of inline assembly in ifactor.cc.

[jdemeyer]

comment:9

And compiling with -DSMARTPTR64 (whatever that means) does not change anything.

[frederichan-IMJPRG]

comment:10

There is a related topic (about SMARTPTR64 that should not be undefined by first.h)here:
http://xcas.e.ujf-grenoble.fr/XCAS/viewtopic.php?f=4&t=1785

[sagetrac-tmonteil]

comment:11

I am not sure if it is the same error, but i cannot build+check giac on Debian Jessie 32bits, see the attached log. It fails at chk_fhan1 and before there are tons of "bug in PARI/GP (Segmentation Fault), please report."

Since giac is a standard package, i set the priority to blocker.

[frederichan-IMJPRG]

comment:12

Attachment: giac-1.2.2.103.log.7.6.rc0.gz

Replying to @sagetrac-tmonteil:

I am not sure if it is the same error, but i cannot build+check giac on Debian Jessie 32bits, see the attached log. It fails at chk_fhan1 and before there are tons of "bug in PARI/GP (Segmentation Fault), please report."

Since giac is a standard package, i set the priority to blocker.

On linux 32bits there is the following problem #22101 and it should be already solved by giac-1.2.3.25 but I am waiting the next one to have a new config.guess as it was requested in #22101.

[vbraun]

comment:13

Unless something happens real soon I'll ignore this for sage 7.6. If it isn't fixed we should think about downgrading giac to optional.

The snippet in the description is hopefully not repesentative of the giac code quality:

• Randomly mixed French / English names for identifiers.
• Use of reserved identifiers (starts with leading underscore + capital letter)

[sagetrac-tmonteil]

comment:14

If the upgrade can not be done soon (i hope it could!), another possibility would be to downgrade giac/giacpy_sage to a previously working version. Indeed, pynac is standard and depends on giac.

[kiwifb]

comment:15

Replying to @sagetrac-tmonteil:

If the upgrade can not be done soon (i hope it could!), another possibility would be to downgrade giac/giacpy_sage to a previously working version. Indeed, pynac is standard and depends on giac.

pynac doesn't currently depend on giac. Ralf wants it to happen, but things keep getting in the way. Enabling giac in pynac will cause at least two doctests to fail.

[sagetrac-tmonteil]

Dependencies: #22101

[sagetrac-tmonteil]

comment:17

If people running powerpc and other platforms coudl check #22101, it seems to solve the issue.

[jpflori]

comment:18

Trying it on ppc64.

[frederichan-IMJPRG]

comment:19

Replying to @sagetrac-tmonteil:

If people running powerpc and other platforms coudl check #22101, it seems to solve the issue.

Really? I thought it was only improving for i386. (cf comment4)
The only (small) news I have for other architecture are about
arm64 from this topic:
http://xcas.e.ujf-grenoble.fr/XCAS/viewtopic.php?f=4&t=1785

where it seems that the static built of icas is functionnal while the original built crashes with 1+1

[jpflori]

comment:20

I can confirm nothing changed on ppc64.

[kiwifb]

comment:21

I think SMARTPTR64 is unrelated to our problems. It is added in some pre-made makefile, some of them for the debian package specifically. But there is no way I can see for it to make its way in the build system on a regular build - any system. Same story for DOUBLEVAL but this time with windows and OS X - it could be of interest since on OS X it included ppc.

[frederichan-IMJPRG]

comment:22

does

./configure --enable-shared=no

changes things?

[jpflori]

comment:58

An old one:

gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/ppc64-redhat-linux/4.8.3/lto-wrapper
Target: ppc64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-languages=c,c++,objc,obj-c++,java,fortran,ada,go,lto --enable-plugin --enable-initfini-array --enable-java-awt=gtk --disable-dssi --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre --enable-libgcj-multifile --enable-java-maintainer-mode --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libjava-multilib --with-isl=/builddir/build/BUILD/gcc-4.8.3-20140911/obj-ppc64-redhat-linux/isl-install --with-cloog=/builddir/build/BUILD/gcc-4.8.3-20140911/obj-ppc64-redhat-linux/cloog-install --enable-secureplt --with-long-double-128 --build=ppc64-redhat-linux
Thread model: posix
gcc version 4.8.3 20140911 (Red Hat 4.8.3-7) (GCC)

I did not run giac testsuite though.

[kiwifb]

comment:59

Replying to @jpflori:

An old one:

gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/ppc64-redhat-linux/4.8.3/lto-wrapper
Target: ppc64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-languages=c,c++,objc,obj-c++,java,fortran,ada,go,lto --enable-plugin --enable-initfini-array --enable-java-awt=gtk --disable-dssi --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre --enable-libgcj-multifile --enable-java-maintainer-mode --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libjava-multilib --with-isl=/builddir/build/BUILD/gcc-4.8.3-20140911/obj-ppc64-redhat-linux/isl-install --with-cloog=/builddir/build/BUILD/gcc-4.8.3-20140911/obj-ppc64-redhat-linux/cloog-install --enable-secureplt --with-long-double-128 --build=ppc64-redhat-linux
Thread model: posix
gcc version 4.8.3 20140911 (Red Hat 4.8.3-7) (GCC)

I did not run giac testsuite though.

Ah, I never had trouble building it. Just running/testing it. It will be some work to get things to build with gcc 4.8.x, will have to find all the C++ dependencies before building.

[jpflori]

comment:60

Most tests seem to pass on my machine except for some numerical noise and some very long or looping tests I killed.

[sagetrac-tmonteil]

comment:61

Replying to @jpflori:

Replying to @sagetrac-tmonteil:

Did you suggest the fix for libpng/lpng1x too ([/22159#comment:29 suggested on this comment])?

Nope, I did not...

Could you please give me some hints on how to achieve this (which file to modify, and how) ?

(sorry for not being able to help on the review, i do not have any ppc machine, my intel 32bit VM is trying it right now, just in case)

[jpflori]

comment:62

Just patch the Makefile to use -lpng16 rather than -lpng.

[kiwifb]

comment:63

Currently giac configure png with

CONFIG_PNG="yes"
AC_ARG_ENABLE(png,
        [AS_HELP_STRING([--enable-png], [Enable PNG library])],
        [ if test "x$enableval" = "xno"; then CONFIG_PNG="no"; fi], [])
  
if test "x$CONFIG_PNG" = "xyes"; then
   AC_CHECK_HEADERS(png.h, AC_CHECK_LIB(png,main))
fi

in configure.in. An option is to use AC_CHECK_LIBS(main,[png16,png14,png12,png]) instead of AC_CHECK_LIB. For Jean-Pierre suggestion you have to do the replacement after configure is run. Because AC_CHECK_LIB is used -lpng is added to LIBS by configure.

[sagetrac-tmonteil]

comment:64

Replying to @sagetrac-tmonteil:

(sorry for not being able to help on the review, i do not have any ppc machine, my intel 32bit VM is trying it right now, just in case)

For what it worth, giac-1.2.3.47 from this ticket compiles and pass self-tests on both my 64bit laptop and 32bit VM (both running Debian GNU/Linux jessie).

[sagetrac-tmonteil]

comment:65

Ragarding building giac with png support, thanks Jean-Pierre and François for your hints, i opened #23262.

[kiwifb]

comment:66

Unfortunately, I still have no luck on ppc64 with gcc-5.4.0. It is better than before in that some tests are now succeeding. But things seems to get stuck in a C++ allocator for a number of them.

[jpflori]

comment:67

Is giac itself functional?
That is: is it only failing its testsuite or is it completely broken as well?

[kiwifb]

comment:68

Fair question. "1+1" works, the fact that some tests work means that it is working up to a point. I haven't tried to run the sage testsuite with it. Any tests I should run?

[jpflori]

comment:69

Maybe sage -t src/sage/interfaces/giac.py and rings and symbolic would tell you if it looks functional or not.

On my side, giac also fails a lot of its own testsuite, inlcuding bunch of numerical noise, but also more worrying stuff i did not investigate, but running tests as above seems to indicate it is good enough for sage.

[kiwifb]

comment:70

OK, I did run those tests and the few failures I have are unrelated to giac (I'll have to investigate some of them more deeply but they are definitely not giac). I guess we can go ahead with this.

[kiwifb]

Reviewer: Thierry Monteil, Jean-Pierre Flori, François Bissey

[vbraun]

Changed branch from public/giac to 8dc4c88

[infinity0]

Changed commit from 8dc4c88 to none

[infinity0]

comment:72

Do you guys know why the -Dx86_64 is needed on ppc64? That is still a bug that should be fixed upstream. In theory upstream should now be detecting 64-bit in a cross-platform way so I'm confused why it fails on ppc64.

[infinity0]

comment:73

(sorry, I have no idea why my comment there deleted the commit field)

(I can't seem to fix it either, even by Modifying the ticket with that commit)

[infinity0]

comment:74

All tests pass for me for 1.2.3-51 on arm64 on Debian without further changes or extra flags, btw.

[kiwifb]

comment:75

I'll have to try 1.2.3-51 on ppc64 without extra flags then. 1.2.3-47 builds but a lot of tests fail for me. We may want to consider a further upgrade.

The removal of the commit when you comment further is a "feature" of track. It is annoying but not harmfull.

[kiwifb]

comment:76

Well, you probably don't have that problem but I allow compilation without gui, and sage itself doesn't depend on fltk

  bool has_graph3d(Fl_Widget * widget){
#ifdef HAVE_LIBFLTK
    Fl_Group * g=dynamic_cast<Fl_Group *>(widget);

Oooops! in src/Xcas.cc (line 3360) there goes your compilation failure if you don't have fltk.

[infinity0]

comment:77

I have found that annoying too, in Debian I am forced to ship the non-GUI icas program (which is what Sage actually wants) together with the GUI xcas program, and it even links against X libraries. I've filed a bug about it upstream, hopefully they can fix it.

[kiwifb]

comment:78

Well, I pilled up some more in http://xcas.e.ujf-grenoble.fr/XCAS/viewtopic.php?f=4&t=1814 because your request, I felt, was much broader than mine (but yes something should be done about all that X stuff).

[infinity0]

comment:79

Unfortunately giac fails to compile for me on a arm64 (aarch64) 4.9 Linux kernel:

http://xcas.e.ujf-grenoble.fr/XCAS/viewtopic.php?p=8941#p8941

It succeeded on a 3.16 Linux kernel though, as I reported above. Could someone reproduce this and if so re-open this ticket or open another one?

[infinity0]

comment:80

A potential solution to this has been suggested on the Debian bug tracker: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=855078#29

I'll try to test it next week, in the meantime feel free to jump in ahead.