Opened 9 years ago

Closed 8 years ago

#12855 closed defect (fixed)

FLINTQS fails to build on Solaris (with GCC 4.7.x)

Reported by: leif Owned by: tbd
Priority: major Milestone: sage-5.1
Component: packages: standard Keywords: spkg GCC 4.7.0 C++ g++ log() math.h overloaded SunOS sd40.5
Cc: ppurka Merged in: sage-5.1.rc0
Authors: Jeroen Demeyer Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

This is a rather subtle issue, related to Solaris' headers, and the fact that GCC (4.7.0) now defines __cplusplus to 199711L, while previous versions defined it to just 1.

If __cplusplus >= 199711L, a lot of overloaded (originally C) functions like div() and log() get defined in the std namespace (if one includes math.h), e.g.

float std::log(float);             // logf() in C
double std::log(double);           // log() in C
long double std::log(long double); // logl() in C

which is correct in the first place, but on Solaris these also end up in the global namespace:

// /usr/include/math.h

...

#if __cplusplus >= 199711L
...
using std::log;
...
#endif

Now FLINTQS happens to call log() with an unsigned long (without a cast) in one place, and with the overloaded functions present as well, the call gets ambiguous. The obvious, trivial fix is to just cast the argument to a double, such that double log(double) gets called (as before).

spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/flintqs-20070817.p7.spkg

flintqs-20070817.p7 (Jeroen Demeyer, 25 May 2012)

  • Trac #12855: Only call log() on float or double types. This works around a problem with Solaris' libc where log() in C++ is overloaded for float, double and long double but not integer types.
  • Use patch for patching, use standard template for spkg-install.
  • Don't retry the build upon failure on x86_64, which seems pointless after #11351.

Attachments (1)

flintqs-20070817.p7.diff (7.4 KB) - added by jdemeyer 9 years ago.
Diff for the flintqs spkg. For reference / review only.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 9 years ago by jdemeyer

  • Keywords sd40.5 added
  • Status changed from new to needs_info
Last edited 9 years ago by jdemeyer (previous) (diff)

comment:2 Changed 9 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Status changed from needs_info to needs_review

Changed 9 years ago by jdemeyer

Diff for the flintqs spkg. For reference / review only.

comment:3 Changed 9 years ago by jdemeyer

New spkg builds fine on Skynet mark with gcc-4.7.0, needs review.

comment:4 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.0.1

comment:5 Changed 8 years ago by kcrisman

Just for reference, #9544 has a very small FreeBSD-only patch which perhaps could be integrated into this. Or is this almost ready for positive review and we should base on that?

#ifdef __FreeBSD__ 
#include <stdint.h> 
#endif 

comment:6 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.0.1 to sage-5.1

comment:7 follow-up: Changed 8 years ago by kcrisman

See also #11792?

comment:8 in reply to: ↑ 7 Changed 8 years ago by jdemeyer

Replying to kcrisman:

See also #11792?

I'm aware of that ticket, but I don't know enough about flint/flintqs to do something about that.

comment:9 follow-up: Changed 8 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to needs_info

The diff looks good as far as it goes, and #9544 (based on this) installs fine and passes relevant tests on OS X 10.6.

I can't test whether this actually works on Solaris or know whether these fixes are right, have to take your word for it. The Mac fix isn't mentioned on this ticket, either, and I can't check if it's necessary or actually fixes anything.

Finally, is flintqs-gcc-4.3-fix.patch still in use? It apparently was not actually applied before? At least that's what the diff seems to indicate. That patch was added over 4 years ago, and I'm not even sure whether we allow people to build with gcc-4.3... In fact,

$ hg log -p spkg-install | less 

indicates that maybe the patch was never applied. ??? Or maybe the fix was simply applied in source (against regulations)?

Anyway, what do you think about that patch? Obviously it still applies, and maybe doesn't cause any trouble.


For positive review, should someone else try it on Solaris? Also we would need just informational update on the Mac and extra patch as mentioned above.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by jdemeyer

Replying to kcrisman:

The Mac fix isn't mentioned on this ticket, either, and I can't check if it's necessary or actually fixes anything.

You mean lanczos.h.patch? That existed before, so it's not subject to review for this ticket.

Finally, is flintqs-gcc-4.3-fix.patch still in use? Or maybe the fix was simply applied in source (against regulations)?

That was indeed the case, you can compare the src/ directories of the old and new spkg if you want. So that patch itself isn't subject to review.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by kcrisman

  • Status changed from needs_info to needs_review

The Mac fix isn't mentioned on this ticket, either, and I can't check if it's necessary or actually fixes anything.

You mean lanczos.h.patch? That existed before, so it's not subject to review for this ticket.

Ok, that makes sense, and I should have figured that out.

Finally, is flintqs-gcc-4.3-fix.patch still in use? Or maybe the fix was simply applied in source (against regulations)?

That was indeed the case, you can compare the src/ directories of the old and new spkg if you want. So that patch itself isn't subject to review.

Ok.

So it seems fine, except for the issue that perhaps someone other than jdemeyer should test it on Solaris with gcc-4.7? I think that's all that's needed here.

comment:12 in reply to: ↑ 11 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Replying to kcrisman:

So it seems fine, except for the issue that perhaps someone other than jdemeyer should test it on Solaris with gcc-4.7?

I just built GCC-4.7.1 (see #13150) on mark (Solaris SPARC) and then built this FLINTQS spkg with GCC-4.7.1 and it worked. This was starting from sage-5.1.beta0.

comment:13 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.1.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.