Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More robust fenv_constants #19384

Merged
merged 1 commit into from
Nov 28, 2016
Merged

More robust fenv_constants #19384

merged 1 commit into from
Nov 28, 2016

Conversation

yuyichao
Copy link
Contributor

@yuyichao yuyichao commented Nov 22, 2016

The constant is platform dependent and the macro definition may not expand to the value itself.

By moving these from the Makefile/perl script to C, it is much more robust to detect if these are defined and is also slightly easier to define the fallback values for each platforms.

Fixes #10127

@yuyichao yuyichao added the system:arm ARMv7 and AArch64 label Nov 22, 2016
@yuyichao
Copy link
Contributor Author

(The arm label is because the fallback value defined in the perl script is x86 specific so it causes the rounding tests to fail on arm when the fallback is used).

@yuyichao
Copy link
Contributor Author

Tentatively mark as backport

@yuyichao
Copy link
Contributor Author

With this PR and #19378, all tests (other than the skipped profile test) passed on the buildbot https://buildtest.e.ip.saba.us/#/builders/66/builds/10 🎉

@vtjnash
Copy link
Member

vtjnash commented Nov 23, 2016

Since these are hardware constants, would it be simpler to just enumerate them in Julia rather than doing this dance with the preprocessor?

@yuyichao
Copy link
Contributor Author

I'd still like to query it from the libc if available. I can move the fallback to julia though.

@yuyichao yuyichao force-pushed the yyc/arm/fenv branch 4 times, most recently from 8959b29 to dfdc7a4 Compare November 24, 2016 00:52
@yuyichao
Copy link
Contributor Author

I decided to remove the fallback entirely. According to cppreference, all of these constants are required by c99 and c++11 which is the minimum requirement for this file. The perl fallback was added original in 773b60e with the comment

I think we could check whether the preprocessor output parsed correctly, and if not, use the default x86/x64 values. I only found this issue on Ubuntu 12.04 (I didn't test it on other versions, I haven't got any other Ubuntu VMs handy), but not on Debian testing or on Arch Linux, so it might not be that big of an issue.

So I'm going to assume that the original issue is exactly the same as the issue we hit on master on arm buildbot (due to old glibc version that (legally) uses the #define FE_* FE_* pattern) which is what this PR solves properly.

@yuyichao
Copy link
Contributor Author

AFAICT the osx failure was a network issue. Restarted.

@tkelman
Copy link
Contributor

tkelman commented Nov 24, 2016

let me check whether this version works on msvc before merging

@tkelman
Copy link
Contributor

tkelman commented Nov 25, 2016

strange, seems to freeze segfault during bootstrap at number.jl? https://ci.appveyor.com/project/tkelman/julia/build/1.0.966 not sure what would cause that

@yuyichao
Copy link
Contributor Author

number.jl? And it doesn't happen without this commit?

I'll expect any possible platform dependent failure to be in C not julia though so that's indeed very strange.

@tkelman
Copy link
Contributor

tkelman commented Nov 25, 2016

Or maybe the file after that, I'm not sure how file name printing during bootstrap is ordered w.r.t. errors, start of processing a file or end of it. I was comparing this commit backported to release-0.5, since master has some other MSVC issues that I'm looking into.

Compare the above build 966 to https://ci.appveyor.com/project/tkelman/julia/build/1.0.965 which gets through bootstrap and stops at ccalltest, as expected. That was at the parent commit of tkelman@7356cc7, which is just the following commits on top of release-0.5 release-0.5...9c11546 (misc build hacks that shouldn't be upstreamed)

@yuyichao
Copy link
Contributor Author

I fixed a wrong include order (#include <cfenv> should not be in extern "C") although I don't understand how it can cause a runtime error rather than a compile time one... maybe try again?

It doesn't really make sense if this causes error in bootstrap before including rounding.jl. A few things that I would try to debug this if this happens again

  1. Putting sth like a jl_safe_printf("in jl_get_fenv_consts\n"); in the C function to see if it is actually executed
  2. Print the global values in rounding.jl to see if they are the same.
  3. Include only the include change in jlapi.c to check if that causes any issues. (This should be the only change that could in theory make a difference before including rounding.jl if there's a compiler bug)

@tkelman
Copy link
Contributor

tkelman commented Nov 25, 2016

https://ci.appveyor.com/project/tkelman/julia/build/1.0.982 didn't get to the prints
making just the include changes worked okay though https://ci.appveyor.com/project/tkelman/julia/build/1.0.983

Any other ideas?

@yuyichao
Copy link
Contributor Author

yuyichao commented Nov 25, 2016

Should be fixed (worked around). The issue seems to be that the MSVC build is somehow now happy about the use of Cint here. It might have got stuck in some loop/recursion when trying to report the actual error.

The error actually happen in rounding.jl but the printing of filename does not appear to be flushed correctly so the output stops at bool.jl. See my test commit(s) on my test branch yuyichao@b87c5cc to make sure the filename is printed before the file is included on windows (not a proper fix...).

@yuyichao
Copy link
Contributor Author

yuyichao commented Nov 26, 2016

AppVeyor failure was git network issue, restarted.

I think the MSVC issue was actually due to the difference between master and 0.5 (edit: i.e. I can reproduce it locally on linux too.) (iirc Cint was moved to much earlier in bootstrap recently). The stuck was likely due to the lack of #18438. With the commit cherry-picked on the MSVC branch it correctly reports the Cintmissing error instead of causing infinite recursion/stackoverflow/whatever when an error occurs.

The constant is platform dependent and the macro definition may not expand to
the value itself.
@yuyichao
Copy link
Contributor Author

I revert back to the Cint version and removed the incorrect comment about MSVC build. I created a 0.5 backport version at https://github.com/JuliaLang/julia/tree/yyc/arm/fenv-0.5 which uses Int32 instead.

@yuyichao yuyichao merged commit dae92d3 into master Nov 28, 2016
@yuyichao yuyichao deleted the yyc/arm/fenv branch November 28, 2016 15:34
tkelman pushed a commit that referenced this pull request Mar 1, 2017
The constant is platform dependent and the macro definition may not expand to
the value itself.

(cherry picked from commit fa0ab21)
ref #19384

Use Int32 instead of Cint due to different bootstrap order on release-0.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system:arm ARMv7 and AArch64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants