Skip to content

Convert ext/standard/math to HNI#1684

Closed
simonwelsh wants to merge 10 commits into
facebook:masterfrom
simonwelsh:math_hni
Closed

Convert ext/standard/math to HNI#1684
simonwelsh wants to merge 10 commits into
facebook:masterfrom
simonwelsh:math_hni

Conversation

@simonwelsh

Copy link
Copy Markdown
Contributor

All changes and deletions to the tests came from running the import script again. It looks like the deletions are all because of a 32bit check.

Part of #1480

@simonwelsh

Copy link
Copy Markdown
Contributor Author

I have one test failing locally from what appears to be a bug with HNI. doubles being passed back are being rounded to 14 dp, not truncated. This is happening with log(). I tried to find where this was happening, but couldn't.

@simonwelsh simonwelsh mentioned this pull request Feb 1, 2014
73 tasks
@simonwelsh

Copy link
Copy Markdown
Contributor Author

And that test passes on Travis.

@scannell

scannell commented Feb 3, 2014

Copy link
Copy Markdown
Contributor

This breaks if you build with ext_zend_compat enabled as that expects to be able to call f_rand() (or HHVM_FN(rand) really) with no parameters have it work.

@simonwelsh

Copy link
Copy Markdown
Contributor Author

Thought I missed a spot. I've changed it to call hphp_rand which, like hphp_mt_rand, has the expected behaviour.

Comment thread hphp/runtime/ext/math/ext_math.cpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

native.h

@scannell

scannell commented Feb 4, 2014

Copy link
Copy Markdown
Contributor

Seeing more problems with this too:

hphp/test/zend/good/Zend/tests/bug30394.php

+2 - HipHop Warning: Too many arguments for max(), expected 1

@scannell

scannell commented Feb 4, 2014

Copy link
Copy Markdown
Contributor

hphp/test/zend/good/ext/standard/tests/array/max_variation2.php also hits an AddressSanitizer fault in the native invocation, as does min_variation1.php -- I believe these are all the same root cause as the above one.

@simonwelsh

Copy link
Copy Markdown
Contributor Author

Hmm, not seeing them with Travis or locally. Has something changed recently with how varargs are handled with HNI functions? (I'm rebasing/building now to check, but it'll probably be faster if someone knows)

@simonwelsh

Copy link
Copy Markdown
Contributor Author

I fixed the two tests that were failing.

I've got no idea why the -r tests are failing. I haven't looked into it, as I've never managed to figure out that part of the code before.

Running the --hphp command locally gives:

/hphp/hiphop-php/hphp/compiler/expression/simple_function_call.cpp:643: bool HPHP::SimpleFunctionCall::isDefineWithoutImpl(HPHP::AnalysisResultConstPtr): assertion `!m_extra' failed.

And the stack trace:

Host: local-hhvm
ProcessID: 8678
ThreadID: 7f471ebaaf40
ThreadPID: 8678
Name: ./hphp/hhvm/hhvm
Type: Aborted
Runtime: hhvm
Version: heads/errorfunc_hni-0-g39e1fafcc38f777205adcb933f629a73d8a1684f
DebuggerCount: 0


# 0  HPHP::bt_handler(int) at crash-reporter.cpp:0
# 1  killpg at /lib/x86_64-linux-gnu/libc.so.6:0
# 2  __GI_raise at /home/aurel32/eglibc/eglibc-2.17/signal/../nptl/sysdeps/unix/sysv/linux/raise.c:56
# 3  __GI_abort at /home/aurel32/eglibc/eglibc-2.17/stdlib/abort.c:92
# 4  HPHP::impl_assert_fail(char const*, char const*, unsigned int, char const*) at ./hphp/hhvm/hhvm:0
# 5  HPHP::SimpleFunctionCall::isDefineWithoutImpl(std::shared_ptr<HPHP::AnalysisResult const>) at ./hphp/hhvm/hhvm:0
# 6  HPHP::Compiler::EmitterVisitor::visit(std::shared_ptr<HPHP::FileScope>) at ./hphp/hhvm/hhvm:0
# 7  HPHP::Compiler::emitHHBCUnitEmitter(std::shared_ptr<HPHP::AnalysisResult>, std::shared_ptr<HPHP::FileScope>, HPHP::MD5 const&) at emitter.cpp:0
# 8  hphp_compiler_parse at ./hphp/hhvm/hhvm:0
# 9  HPHP::compile_string(char const*, unsigned long, char const*) at ./hphp/hhvm/hhvm:0
# 10 HPHP::compile_systemlib_string(char const*, unsigned long, char const*) at ./hphp/hhvm/hhvm:0
# 11 HPHP::Extension::loadSystemlib(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) at ./hphp/hhvm/hhvm:0
# 12 HPHP::StandardMathExtension::moduleInit() at ./hphp/hhvm/hhvm:0
# 13 HPHP::Extension::InitModules() at ./hphp/hhvm/hhvm:0
# 14 HPHP::hphp_process_init() at ./hphp/hhvm/hhvm:0
# 15 HPHP::process(HPHP::CompilerOptions const&) at ./hphp/hhvm/hhvm:0
# 16 HPHP::compiler_main(int, char**) at ./hphp/hhvm/hhvm:0
# 17 main at ./hphp/hhvm/hhvm:0
# 18 __libc_start_main at /home/aurel32/eglibc/eglibc-2.17/csu/libc-start.c:310
# 19 _start at ./hphp/hhvm/hhvm:0

PHP Stacktrace:

Command run was:

./hphp/hhvm/hhvm --hphp --config hphp/test/slow/hphp_config.hdf  -thhbc -l0 -k1 -o hphp/test/slow/ext_math/round.php.repo hphp/test/slow/ext_math/round.php

@scannell

scannell commented Feb 6, 2014

Copy link
Copy Markdown
Contributor

Something strange is going on here in repo mode with HNI. @sgolemon, can you take a look here?

@sgolemon

sgolemon commented Mar 5, 2014

Copy link
Copy Markdown
Contributor

@simonwelsh As mentioned on IRC, since the PR is causing otherwise inexplicable perf regressions, I'm going to take over doing the math functions a few at a time to try to narrow down the issue.

@sgolemon sgolemon closed this Mar 5, 2014
@simonwelsh simonwelsh deleted the math_hni branch May 20, 2014 05:09
hhvm-bot pushed a commit that referenced this pull request Dec 5, 2014
Summary: fixes #1684

Convert ext/standard/math to HNI

Reviewed By: @ptarjan, @JoelMarcey

Differential Revision: D1646888

Signature: t1:1646888:1414613111:207e8c5ce9c600857a98461a4848aec6bf739647

Pulled By: @nigelchanyk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants