Opened 4 years ago

Closed 4 years ago

#25118 closed defect (fixed)

gfan fails when compiled with XCode 9.3

Reported by: vbraun Owned by:
Priority: critical Milestone: sage-8.3
Component: packages: standard Keywords:
Cc: dimpase Merged in:
Authors: Dima Pasechnik Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: ab26648 (Commits, GitHub, GitLab) Commit: ab26648b54a45f87e5cb8a18411298e8350a130b
Dependencies: Stopgaps:

Status badges

Description

On certain architectures only, gfan compiled with XCode 9.3 fails

sage -t --long src/doc/de/tutorial/tour_advanced.rst  # 2 doctests failed
sage -t --long src/doc/en/tutorial/tour_advanced.rst  # 2 doctests failed
sage -t --long src/doc/fr/tutorial/tour_advanced.rst  # 2 doctests failed
sage -t --long src/doc/ja/tutorial/tour_advanced.rst  # 2 doctests failed
sage -t --long src/doc/pt/tutorial/tour_advanced.rst  # 2 doctests failed
sage -t --long src/doc/ru/tutorial/tour_advanced.rst  # 2 doctests failed
sage -t --long src/sage/rings/polynomial/groebner_fan.py  # 70 doctests failed
sage -t --long src/sage/rings/polynomial/multi_polynomial_ideal.py  # 1 doctest failed

There are also reported Cython testsuite failures, so it might be a compiler issue.

Change History (20)

comment:1 Changed 4 years ago by dimpase

  • Cc dimpase added

comment:2 Changed 4 years ago by dimpase

See testing results for the "stand-alone" gfan-6.2 on https://github.com/rwst/gfan06/issues/2

Basically, one test fails on OSX Xeon with Xcode 9.3 (same as on Ubuntu with gcc 5.4), while a slew of tests fail on OSX i5/7 with Xcode 9.3.

comment:3 follow-up: Changed 4 years ago by dimpase

gfan-6.2 source assumes that cdd headers are in cdd/, so one can e.g.

cd $SAGE_LOCAL/include
mkdir cdd
cp cdd*.h cdd/
cp setoper.h cdd/

before hitting make. Also, the makefile needs to be modified not to use -fno-guess-branch-probability on clang.

comment:4 in reply to: ↑ 3 Changed 4 years ago by fbissey

Replying to dimpase:

gfan-6.2 source assumes that cdd headers are in cdd/, so one can e.g.

cd $SAGE_LOCAL/include
mkdir cdd
cp cdd*.h cdd/
cp setoper.h cdd/

before hitting make. Also, the makefile needs to be modified not to use -fno-guess-branch-probability on clang.

For cdd's header you can pass -DNOCDDPREFIX that's one of the things I have learnt from looking at the new spkg at #23353. We probably should depend on it.

comment:5 Changed 4 years ago by dimpase

Can one try building/testing gfan with -O0 on affected Xcode/CPUs, see if this provides a fix?

(i.e. with CXXFLAGS (and CFLAGS?) having -O0 instead of the default?)

Last edited 4 years ago by dimpase (previous) (diff)

comment:6 Changed 4 years ago by dimpase

Travis CI can now do Xcode 9.3 for you,so it's matter of writing a suitable yaml file to test gfan there...

comment:7 Changed 4 years ago by dimpase

The crashes in gfan's 0.6.2 testsuite come from line 208 of app_main.cpp

    delete ep;//<--- In next release, make the class virtual

It's a pointer to an object of type EnumerationFilePrinter handling output.

it's perhaps safe to just delete it, as it is at the end of main() member function of GCats class. (I'll figure it out exactly, anyway, it's a small thing to fix).

If I delete this line then 44 out of 45 tests pass (same as on Ubunty).

This was tried on OSX 10.13.4 on very old Core2 Duo CPU, with clang from XCode 9.3, which is Apple LLVM version 9.1.0 (clang-902.0.39.1).

Please, please, try this on i5/i7.

comment:8 Changed 4 years ago by jhpalmieri

I won’t be able to try until the weekend.

comment:9 Changed 4 years ago by fbissey

Nice work Dima. So I took my broken 8.2.rc1. Digged the source for gfan-0.5, found the line in those sources, created a patch and put it in the build tree. Check ed that the multi_polynomial_ideal.py test is broken - yup. Rebuild gfan with my patch, gfan do not produces libraries, so there is no need to rebuild sage. We really want to upgrade and hope stuff like these is fixed by the way

[gfan-0.5.p2] ./intsinpolytope.h:1:9: warning: 'INTSINPOLYTOPE_H_INCLUDED' is used as a header guard here, followed by #define of a different macro [-Wheader-guard]
[gfan-0.5.p2] #ifndef INTSINPOLYTOPE_H_INCLUDED
[gfan-0.5.p2]         ^~~~~~~~~~~~~~~~~~~~~~~~~
[gfan-0.5.p2] ./intsinpolytope.h:2:9: note: 'INTSINPOLYTOPE_H_INLCUDED' is defined here; did you mean 'INTSINPOLYTOPE_H_INCLUDED'?
[gfan-0.5.p2] #define INTSINPOLYTOPE_H_INLCUDED
[gfan-0.5.p2]         ^~~~~~~~~~~~~~~~~~~~~~~~~
[gfan-0.5.p2]         INTSINPOLYTOPE_H_INCLUDED
[gfan-0.5.p2] 1 warning generated.

Tried the multi_polynomial_ideal.py test again. Success! I checked the other tests (well only one of the .rst, after all they are all copies of the same tests) and they work too.

I am doing full doctesting now in case something broke in the process.

And the patch

  • app_main.cpp

    diff --git a/app_main.cpp b/app_main.cpp
    index f6fcb66..e24b0b2 100644
    a b public: 
    180180    }
    181181
    182182    ep->close();
    183     delete ep;
    184183
    185184    printf("\n");
    186185

comment:10 Changed 4 years ago by fbissey

All doctests passed. We can go ahead with this.

comment:11 Changed 4 years ago by dimpase

Yep, I also tried the same as in comment 9 patch on 8.2.rc1 with latest Xcode on my museum Core2 Duo, and only some timeouts from tests, no real problems.

comment:12 Changed 4 years ago by dimpase

  • Authors set to Dima Pasechnik

comment:13 Changed 4 years ago by dimpase

  • Branch set to u/dimpase/gfan_xcode63_fix
  • Commit set to 21464333b254d31bfe95801d575b203d9a9731cf
  • Priority changed from major to blocker
  • Status changed from new to needs_review
  • Stopgaps #25113 deleted

Done. Hopefully this can still make it into 8.2.


New commits:

7e13822Revert "Trac #25113: OSX is f*ed up sometimes"
2146433Fixes miscompilation by clang from XCode 6.3, see #25118

comment:14 Changed 4 years ago by fbissey

I am going to be a pain in your ass. Please make it a revision bump of gfan.

comment:15 Changed 4 years ago by git

  • Commit changed from 21464333b254d31bfe95801d575b203d9a9731cf to ab26648b54a45f87e5cb8a18411298e8350a130b

Branch pushed to git repo; I updated commit sha1. New commits:

ab26648gfan version bump

comment:16 Changed 4 years ago by dimpase

Done, sorry for forgetting this.

comment:17 Changed 4 years ago by dimpase

I wonder if something needs to be done with configure, as this changes configure.ac.

comment:18 Changed 4 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

Depends on the flow of the release manager, but Volker didn't do a bump for #25113 so hopefully he has a procedure for that now.

comment:19 Changed 4 years ago by vbraun

  • Milestone changed from sage-8.2 to sage-8.3
  • Priority changed from blocker to critical

comment:20 Changed 4 years ago by vbraun

  • Branch changed from u/dimpase/gfan_xcode63_fix to ab26648b54a45f87e5cb8a18411298e8350a130b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.