AArch64 Fix getFrameRegs assertion during indirect fixup #7912

jim-saxman · 2017-07-11T23:52:35Z

On AA4ch64 plaforms when calling getFrameRegs() and an indirect
fixup is found, the RIP needs to be present at the correct
offset. It was not, because the destructor was compiled with a
tail-call. It needs to have a standard stack frame.

By disabling the tail-call via the compiler flag on all destructors
in g_destructors[], we can ensure that all destructors for AArch64
are walked correctly.

This bug occurred sometimes in OSS Mediawiki after a few minutes.
We tried to create a simplified unit test but were unsuccessful.

This patch introduces no new unit test failures for either DebugOpt
or Release build types on AArch64 platforms and doesn't affect x64
or PPC64 destructors.

On AA4ch64 plaforms when calling getFrameRegs() and an indirect fixup is found, the RIP needs to be present at the correct offset. It was not, because the destructor was compiled with a tail-call. It needs to have a standard stack frame. By disabling the tail-call via the compiler flag on all destructors in g_destructors[], we can ensure that all destructors for AArch64 are walked correctly. This bug occurred sometimes in OSS Mediawiki after a few minutes. We tried to create a simplified unit test but were unsuccessful. This patch introduces no new unit test failures for either DebugOpt or Release build types on AArch64 platforms and doesn't affect x64 or PPC64 destructors.

jim-saxman · 2017-07-11T23:55:52Z

@mxw Hi Max, can you lease review this PR.
@swalk-cavium Are you still seeing the errors in test/quick/*dtor.php unti tests, and does this patch help?

apinski-cavium · 2017-07-12T00:00:59Z

You just use __aarch64__ instead __AARCH64EL__ (not that matters but it is just a style thing).
Also it seems better to do something like:

inline void ArrayData::release() noexcept {
  assert(hasExactlyOneRef());
  g_array_funcs.release[kind()](this);
  AARCH64_WALKABLE_FRAME ();
 }

And have AARCH64_WALKABLE_FRAME (); expand to something like:
__asm__("":::"memory");

I am not a fan of the optimize attribute in this case :).

mxw · 2017-07-12T20:14:04Z

I think __aarch64__ is more consistent with what we're using elsewhere.

I chatted with @dave-estes offline who seems to like the memory barrier approach. You can continue ifdeffing that macro to do nothing on x86-64.

hhvm-bot · 2017-07-12T22:24:53Z

@jim-saxman updated the pull request - view changes

mxw

Just a couple of nits.

mxw · 2017-07-14T01:19:59Z

hphp/runtime/base/array-data-defs.h

-  return g_array_funcs.release[kind()](this);
+  g_array_funcs.release[kind()](this);
+  AARCH64_WALKABLE_FRAME();
+  return;


Just delete this return; this function returns void.

mxw · 2017-07-14T01:20:09Z

hphp/runtime/base/mixed-array.cpp

@@ -468,7 +468,6 @@ ArrayData* MixedArray::MakeDictFromAPC(const APCArray* apc) {

 //=============================================================================
 // Destruction
-


Put this back?

@mxw

This patch addresses @mxw's code review comments.

hhvm-bot · 2017-07-18T14:10:55Z

@jim-saxman updated the pull request - view changes

swalk-cavium · 2017-07-20T23:11:25Z

@jim-saxman - I ran a full MOP on this change.

These two tests now pass with these option sets
hphp/test/run -v -m jit -a '-vEval.JitPGO=1'
hphp/test/run -v -m jit -r -a '-vEval.JitPGO=1'
hphp/test/quick/setg-dtor.php
hphp/test/quick/sets-dtor.php

hphp/test/slow/async/simple_meth.php passes all option sets.

A bunch of failures have reappeared with this option set
hphp/test/run -v -m jit -r -a '-vEval.JitPGO=1'
hphp/test/zend/good/ext/spl/tests/iterator_002.php
hphp/test/zend/good/ext/spl/tests/iterator_033.php
hphp/test/zend/good/ext/spl/tests/iterator_040.php
hphp/test/zend/good/ext/spl/tests/iterator_048.php
hphp/test/zend/good/ext/spl/tests/observer_001.php
hphp/test/zend/good/ext/spl/tests/observer_002.php
hphp/test/zend/good/ext/spl/tests/RecursiveCallbackFilterIteratorTest.php

I think I saw them last between fixes for relocation.

hhvm-bot · 2017-07-20T23:19:10Z

@mxw has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

jim-saxman · 2017-07-25T15:00:18Z

Hi @swalk-cavium, Thanks for running tests on this patch. I re-ran all unit tests in all configurations and observed the following:

For PGO with and without repoAuth:
hphp/test/quick/setg-dtor.php
hphp/test/quick/sets-dtor.php
pass both before and after this patch.

hphp/test/slow/async/simple_meth.php
still fails with -r -m pgo

php/test/zend/good/ext/spl/tests/iterator_002.php
hphp/test/zend/good/ext/spl/tests/iterator_033.php
hphp/test/zend/good/ext/spl/tests/iterator_040.php
hphp/test/zend/good/ext/spl/tests/iterator_048.php
hphp/test/zend/good/ext/spl/tests/observer_001.php
hphp/test/zend/good/ext/spl/tests/observer_002.php
hphp/test/zend/good/ext/spl/tests/RecursiveCallbackFilterIteratorTest.php
never failed both before and after this patch, for -r -m pgo.

I performed these tests on both CentOS with GCC 6.2.1 and Ubuntu with GCC 5.4.

swalk-cavium · 2017-07-25T15:09:48Z

Hi @jim-saxman, I'm wondering if one of my machines needs a firmware upgrade.

vielmetti · 2017-07-25T23:46:22Z

Hi @swalk-cavium @jim-saxman - just did a build and test cycle and am seeing these errors:

HipHop VM 3.20.0-dev (rel)
Compiler: heads/master-0-g26a2f8c2795726e89dd636c16b2e1e6316dee7df
Repo schema: c9ccdb2b68ae4d61f3c1c0232b4b2426ff39c26e
gcc version 5.4.0 20160609 (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.4)

test/slow/ext_icu/uspoof.php
test/slow/ext_image/1794.php
test/slow/ext_posix/ext_posix.php
test/slow/ext_spl_file/1795.php
test/slow/ir_inlining/1.php
test/quick/setg-dtor.php
test/quick/sets-dtor.php

System under test is a 96-core Packet Type 2A server, with Cavium ThunderX, using Ubuntu 16.04. Rather than noting a new issue, since the set*-dtor failures seem to be related, I'll just report here.

swalk-cavium · 2017-07-26T21:10:34Z

Hi Ed, We have yet to have an all green run on the regression tests. Our Jenkins runs 6 different option sets. Which options did you use below? The uspoof ones depend on which version of the internationalization library is used. This one always fails our CI testing. It has something to do with libicui18n.so The ext_posix.php test assumes that there is a 'wheel' group. Try sudo groupadd wheel I was not able to get 1794.php or 1795.php to fail. I think these are related to the PR in the subject. ir_inlining/1.php, setg-dtor.php and sets-dtor.php Theobroma Systems makes their CI available here. One option set, multiple OSs. http://hhvm-arm64.theobroma-systems.com/ Steve Walk

…

________________________________ From: Edward Vielmetti <notifications@github.com> Sent: Tuesday, July 25, 2017 6:46 PM To: facebook/hhvm Cc: Walk, Steve; Mention Subject: Re: [facebook/hhvm] AArch64 Fix getFrameRegs assertion during indirect fixup (#7912) Hi @swalk-cavium<https://github.com/swalk-cavium> @jim-saxman<https://github.com/jim-saxman> - just did a build and test cycle and am seeing these errors: HipHop VM 3.20.0-dev (rel) Compiler: heads/master-0-g26a2f8c2795726e89dd636c16b2e1e6316dee7df Repo schema: c9ccdb2b68ae4d61f3c1c0232b4b2426ff39c26e gcc version 5.4.0 20160609 (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.4) test/slow/ext_icu/uspoof.php test/slow/ext_image/1794.php test/slow/ext_posix/ext_posix.php test/slow/ext_spl_file/1795.php test/slow/ir_inlining/1.php test/quick/setg-dtor.php test/quick/sets-dtor.php System under test is a 96-core Packet Type 2A server, with Cavium ThunderX, using Ubuntu 16.04. Rather than noting a new issue, since the set*-dtor failures seem to be related, I'll just report here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#7912 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AV_bZDSP3ewBlKj3mkRyFpX2V5u_MfhUks5sRn5pgaJpZM4OU9Jc>.

vielmetti · 2017-07-29T04:15:18Z

Steve -

I did a fresh build, with the following results.

test/quick succeeds with only two failures (the dtor failures). I have not yet tested this patch which should address those.

ext_posix succeeded after I added the wheel group.

uspoof failures are described in #3925 as a result of Facebook testing against a system with ICU 49 instead of the current ICU 55 (or so).

I was not able to get 1794.php or 1795.php to fail again in a second test.

vielmetti · 2017-07-29T05:23:03Z

After applying this patch all of test/quick passes ("SHIP IT").

All of test/slow passes except test/slow/ext_icu/uspoof.php which is described by #3925.

Looking good!

facebook-github-bot · 2017-08-18T01:03:43Z

@mxw has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

hhvm-bot added GH Review: review-needed CLA Signed labels Jul 11, 2017

mxw self-assigned this Jul 12, 2017

AArch64 Address code review suggestions

a87b757

mxw reviewed Jul 14, 2017

View reviewed changes

[AArch64] Addresses code review comments

a93f8df

This patch addresses @mxw's code review comments.

hhvm-bot added the Import Started label Jul 20, 2017

vielmetti mentioned this pull request Jul 29, 2017

Refactor test/slow/ext_icu/uspoof.php to split apart the icu_49 components #7939

Open

hhvm-bot added GH Review: accepted and removed GH Review: review-needed labels Aug 18, 2017

hhvm-bot closed this in 1ecac73 Aug 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AArch64 Fix getFrameRegs assertion during indirect fixup #7912

AArch64 Fix getFrameRegs assertion during indirect fixup #7912

jim-saxman commented Jul 11, 2017

jim-saxman commented Jul 11, 2017

apinski-cavium commented Jul 12, 2017 •

edited

Loading

mxw commented Jul 12, 2017

hhvm-bot commented Jul 12, 2017

mxw left a comment

mxw Jul 14, 2017

mxw Jul 14, 2017

hhvm-bot commented Jul 18, 2017

swalk-cavium commented Jul 20, 2017

hhvm-bot commented Jul 20, 2017

jim-saxman commented Jul 25, 2017

swalk-cavium commented Jul 25, 2017

vielmetti commented Jul 25, 2017

swalk-cavium commented Jul 26, 2017 via email

vielmetti commented Jul 29, 2017

vielmetti commented Jul 29, 2017

facebook-github-bot commented Aug 18, 2017

		@@ -468,7 +468,6 @@ ArrayData* MixedArray::MakeDictFromAPC(const APCArray* apc) {

		//=============================================================================
		// Destruction

AArch64 Fix getFrameRegs assertion during indirect fixup #7912

AArch64 Fix getFrameRegs assertion during indirect fixup #7912

Conversation

jim-saxman commented Jul 11, 2017

jim-saxman commented Jul 11, 2017

apinski-cavium commented Jul 12, 2017 • edited Loading

mxw commented Jul 12, 2017

hhvm-bot commented Jul 12, 2017

mxw left a comment

Choose a reason for hiding this comment

mxw Jul 14, 2017

Choose a reason for hiding this comment

mxw Jul 14, 2017

Choose a reason for hiding this comment

hhvm-bot commented Jul 18, 2017

swalk-cavium commented Jul 20, 2017

hhvm-bot commented Jul 20, 2017

jim-saxman commented Jul 25, 2017

swalk-cavium commented Jul 25, 2017

vielmetti commented Jul 25, 2017

swalk-cavium commented Jul 26, 2017 via email

vielmetti commented Jul 29, 2017

vielmetti commented Jul 29, 2017

facebook-github-bot commented Aug 18, 2017

apinski-cavium commented Jul 12, 2017 •

edited

Loading