Better fix for ARM64 native calls

[apinski-cavium]

I am new to this pull request so I might have messed this up.

[facebook-github-bot]

This pull request has been imported into Phabricator, and discussion and review of the diff will take place at https://reviews.facebook.net/D37299

[facebook-github-bot]

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 cla@fb.com. Thanks!

[apinski-cavium]

Note with “test/run --mode 'interp' all”:
71 tests failed

Which is much better than any other patch.

[edwinsmith]

Please see my comments on https://reviews.facebook.net/D37299

[apinski-cavium]

isn't char signed by default?
No, please read below of the reason why it is not and is allowed not to be by the C/C++ standard.

Here is what the C standard says (C++ is similar):
Which of signed char or unsigned char has the same range, representation, and behavior as “plain” char (C90 6.1.2.5, C90 6.2.1.1, C99 and C11 6.2.5, C99 and C11 6.3.1.1).

GCC has this to say (https://gcc.gnu.org/onlinedocs/gcc/Characters-implementation.html#Characters-implementation):
Determined by ABI. The options -funsigned-char and -fsigned-char change the default. See Options Controlling C Dialect.

The AARCH64 ABI (http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf) says this:
7.1.1 Arithmetic Types
The mapping of C arithmetic types to Fundamental Data Types is shown in Table 3, Mapping of C & C++ built-in data types.
char unsigned byte
unsigned char unsigned byte
signed char signed byte

So it is not about ARM GCC but rather all ARM compilers.

is the problem that gcc with aarch64 treats char as unsigned by default? I am concerned this may cause a rash of bugs all over the place, if so.

That is correct all AARCH64 treats char as unsigned char as referenced above and all the code needs to be audited for this as C says it is an implemented defined if char is signed or unsigned (as referenced above). PowerPC ABI says the same as AARCH64. x86 is really the old man out here :).

[apinski-cavium]

Also I can't figure out how to comment on https://reviews.facebook.net/D37299

strncmp_variation6.php

This change is not related to char being unsigned but rather just a different implementation of memcmp which returns 0x80 rather than 0x1 when the difference between the bytes are 1. The C standard is clear that positive values should be treated all the same here.

[apinski-cavium]

this should be ifndef aarch64, or something

Actually that code can be removed as callFuncPODImpl (which really should be called callFuncNonPODImpl but I was too quick in getting the fix in) is the place which is handling the return value "correctly" for both AARCH64 and x86 ABIs rather than inside native.cpp.

[apinski-cavium]

compact-tagged-ptrs.h

The correct fix is most likely a sign extend issue, I will look into that today. I would rather see 8bit tags rather than 16bit tags and not remove them for AARCH64.

Also the VA might be extended in future versions of ARMv8 to greater than 48bit so 16bit tags are going to break but 8bit tags won't break as that is now part of the ABI and architecture.

[edwinsmith]

signed char

Ok, I agree we should do the necessary auditing work then, and the fix for different memcmp() results makes sense. We often prefer the <stdint.h> types (e.g. int8_t) when the signed-ness and bit-size need to be explicit, but "signed char" should be ok.

unable to comment on Phabricator

What happens when you click the link and try to reply to a comment inline? do you get an error? there might be a setup step that we missed, to associate your github account with phabricator.

CompactTaggedPtr

I think everywhere we use CompactTaggedPtr, we either use no tag at all, or a tag that would fit in 8 bits. It should be possible to specialize the template class as follows:

1. tag > 16 bits => compile time error because the class is no longer "compact"
2. tag == 16 bits => do masking everywhere
3. tag = 8 bits => no masking on arm, masking on x64
4. tag = void => no masking anywhere.

I think case 2 is the least amount of work for now, we could pursue case 3 and 4 as an optimization later.

[edwinsmith]

Hi, to comment on the phabricator diffs, please just register your github username at reviews.facebook.net. then we can use phabricator for review feedback (our preferred workflow).

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[JoelMarcey]

@apinski-cavium Normally we have a week timeout when changes are requested on a pull request where we close the pull request if there has been no action.

Do you plan on addressing the comments of @edwinsmith in order to continue to move this pull request forward?

https://reviews.facebook.net/D37299

[JoelMarcey]

@apinski-cavium I am going to close this pull request out due to lack of activity, per our timeout policy. That said, at any time, you can re-open this pull request (or start a new one) that provides the changes requested.

Thanks!