[Aarch64] Remove -fsigned-char flag and fix resulting failure #7458
Conversation
Remove the -fsigned-char flag for Aarch64 systems. As a result, the unit test zend/good/ext/fileinfo/tests/finfo_file_002.php began failing. Thus, this patch changes a cast to (signed char), and the unit test passes again.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@aorenste has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
What is the higher-level goal of this change? |
|
Patch #7361 introduced the -fsigned-char flag and has the backstory for why I am removing it. |
|
Hmmm, I don't think I ever saw that one. Looping in @dave-estes and @apinski-cavium. Have you guys done any more testing w.r.t. signed chars since back then? As I just commented on the previous PR, the concern about mismatched ABIs is pretty compelling to me, and I think we should make this change; I'm just curious if anyone else has had any problems with it since then. |
|
We've been running with and without signed chars for a while. In light of @apinski-cavium concerns, I think we should go ahead with this change to revert back to signed chars for ARM. If there are any latent bugs in HHVM, then we'll just have to find them. |
|
@mxw, is there anything else needed to land this PR? |
|
@dave-estes—I need to do some more testing internally. The CMake build doesn't affect us, but our internal tests stress the system more. Sorry for putting this off; it didn't seem to be blocking other changes, so I've been punting on it. I'll look into it this week. |
|
@mxw ping.... This patch still applies cleanly for me, can we get it merged? |
|
Tested here as well -> no regressions. |
|
thought this had landed months ago... |
|
@Orvid Me too! I can give it another round of testing on AArch64 if needed. |
|
This patch is still clean for AArch64 tests. There are no new failures for test/all with {"","--mode interp","--mode pgo","-r --mode pgo"}. |
Remove the -fsigned-char flag for Aarch64 systems. As a result,
the unit test zend/good/ext/fileinfo/tests/finfo_file_002.php
began failing. Thus, this patch changes a cast to (signed char),
and the unit test passes again.