Closed Bug 963024 Opened 10 years ago Closed 10 years ago

AArch64 support for XPCOM

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: marcin, Unassigned)

References

Details

Attachments

(1 file, 8 obsolete files)

Attached patch aarch64-xpcom.patch (obsolete) — Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140122030521

Steps to reproduce:

This patch adds AArch64 support.
Blocks: 962534
Attachment #8364272 - Flags: review?(nfroyd)
Comment on attachment 8364272 [details] [diff] [review]
aarch64-xpcom.patch

Review of attachment 8364272 [details] [diff] [review]:
-----------------------------------------------------------------

I have a lot of comments here, but they are mostly stylistic ones.  I would like to see the patch again once you apply the feedback.

::: mozilla-central.orig/xpcom/reflect/xptcall/src/md/unix/Makefile.in
@@ +55,5 @@
>  endif
>  endif
>  
>  ######################################################################
> +# AARCH64

All the rules here look fine, but you should be modifying moz.build instead of Makefile.in.  The syntax of the moz.build file shouldn't be too hard to figure out; please let me know if you have any problems.

@@ +64,5 @@
> +ifeq ($(OS_ARCH),Linux)
> +ifneq (,$(filter aarch64,$(OS_TEST)))
> +CPPSRCS		:= xptcinvoke_aarch64.cpp xptcstubs_aarch64.cpp
> +ASFILES		:= xptcinvoke_asm_aarch64.s xptcstubs_asm_aarch64.s
> +CXXFLAGS += -O2

You should not be setting CXXFLAGS at all.  Please remove this bit.

::: mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_aarch64.cpp
@@ +57,5 @@
> +  if (freg_args < end) {
> +    *freg_args = data;
> +    freg_args++;
> +  } else {
> +    *(double *)stack_args = data;

Please do a memcpy(stack_args, &data, sizeof(data)); here to avoid aliasing violations.

@@ +68,5 @@
> +                               double* end,
> +                               float  data)
> +{
> +  if (freg_args < end) {
> +    *(float *)freg_args = data;

I think this is the only place in this file where endianness matters.  You could rewrite to say:

#if defined(__AARCH64EL__)
  float freg[2] = { data, 0.0f }
#else
  float freg[2] = { 0.0f, data }
#endif
  if (freg_args < end) {
    memcpy(freg_args, &freg, sizeof(freg));
    freg_args++;
  } else {
    memcpy(stack_args, &freg, sizeof(freg));
    stack_args++;
  }
}

(I think I got the endianness right.)  Your choice on whether you handle endianness, but please use memcpy to avoid aliasing violations.

@@ +83,5 @@
> +                     uint32_t paramCount, nsXPTCVariant* s)
> +{
> +  uint64_t *ireg_args = stk;
> +  uint64_t *ireg_end  = ireg_args + 8;
> +  double *freg_args = (double *)ireg_end;

Please use the appropriate C++-style cast here.

@@ +85,5 @@
> +  uint64_t *ireg_args = stk;
> +  uint64_t *ireg_end  = ireg_args + 8;
> +  double *freg_args = (double *)ireg_end;
> +  double *freg_end  = freg_args + 8;
> +  uint64_t *stack_args = (uint64_t *)freg_end;

Please use the appropriate C++-style cast here.

@@ +127,5 @@
> +}
> +
> +extern "C" nsresult _NS_InvokeByIndex(nsISupports* that, uint32_t methodIndex,
> +                                        uint32_t paramCount,
> +                                        nsXPTCVariant* params);

Nit: please line this up like so:

extern "C" nsresult _NS_InvokeByIndex(...
                                      uint32_t paramCount, nsXPTCVariant* params);

@@ +131,5 @@
> +                                        nsXPTCVariant* params);
> +
> +EXPORT_XPCOM_API(nsresult)
> +NS_InvokeByIndex(nsISupports* that, uint32_t methodIndex,
> +                   uint32_t paramCount, nsXPTCVariant* params)

Likewise on the alignment here.

::: mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_aarch64.s
@@ +7,5 @@
> +	.globl _NS_InvokeByIndex
> +	.type  _NS_InvokeByIndex,@function
> +
> +/*
> + * NS_InvokeByIndex(nsISupports* that, uint32_t methodIndex,

Nit: this should be _NS_InvokeByIndex.

::: mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcstubs_aarch64.cpp
@@ +24,5 @@
> +    nsXPTCMiniVariant paramBuffer[PARAM_BUFFER_COUNT];
> +    nsXPTCMiniVariant* dispatchParams = NULL;
> +    const nsXPTMethodInfo* info;
> +    uint8_t paramCount;
> +    uint8_t i;

Please declare these two integer variables where they are first defined, e.g. |i| can be declared in the for loop itself.  Please also make them uint32_t.

@@ +37,5 @@
> +
> +    // setup variant array pointer
> +    if(paramCount > PARAM_BUFFER_COUNT)
> +        dispatchParams = new nsXPTCMiniVariant[paramCount];
> +    else

Please brace the else and the if here.

@@ +45,5 @@
> +    uint64_t* ap = args;
> +    uint32_t next_gpr = 1; // skip first arg which is 'self'
> +    uint32_t next_fpr = 0;
> +    for(i = 0; i < paramCount; i++)
> +    {

Nit on formatting, braces cuddle with the control structure, so:

for (...) {
}
if (...) {
}
switch (...) {
}

Please fix this and instances below.

@@ +52,5 @@
> +        nsXPTCMiniVariant* dp = &dispatchParams[i];
> +
> +        if(param.IsOut() || !type.IsArithmetic())
> +        {
> +            if (next_gpr < PARAM_GPR_COUNT)

All of this code could be simplified by doing:

uint64_t value = (next_gpr < PARAM_GPR_COUNT) ? gprData[next_gpr++] : *ap++;

and then just using |value| in the appropriate places below.

@@ +53,5 @@
> +
> +        if(param.IsOut() || !type.IsArithmetic())
> +        {
> +            if (next_gpr < PARAM_GPR_COUNT)
> +                dp->val.p = (void*)gprData[next_gpr++];

C++-style casts, please.

@@ +63,5 @@
> +        switch(type)
> +        {
> +        case nsXPTType::T_I8:
> +            if (next_gpr < PARAM_GPR_COUNT)
> +                dp->val.i8  = (int8_t)gprData[next_gpr++];

C++-style casts, please, here and all the way through this switch.

@@ +119,5 @@
> +             break;
> +
> +        case nsXPTType::T_FLOAT:
> +              if (next_fpr < PARAM_FPR_COUNT)
> +                  dp->val.f  = *(float *)(fprData + next_fpr++);

Please use memcpy in this entire case and the double case below to avoid aliasing violations.  Bonus points for handling endianness if you want.

@@ +121,5 @@
> +        case nsXPTType::T_FLOAT:
> +              if (next_fpr < PARAM_FPR_COUNT)
> +                  dp->val.f  = *(float *)(fprData + next_fpr++);
> +              else
> +              {

Braces with the else, please.

@@ +167,5 @@
> +
> +    result = self->mOuter->CallMethod((uint16_t)methodIndex, info, dispatchParams);
> +
> +    if(dispatchParams != paramBuffer)
> +        delete [] dispatchParams;

Single-line ifs get braced.
Attachment #8364272 - Flags: review?(nfroyd) → feedback+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Lot of suggestions ;)

AArch64 port was based on code for some other architecture and that's why it looks like it looks - it just follows current code.

I will take a look and adapt code here and there...
(In reply to Marcin Juszkiewicz from comment #2)
> AArch64 port was based on code for some other architecture and that's why it
> looks like it looks - it just follows current code.
> 
> I will take a look and adapt code here and there...

Yeah, apologies for all the style nits.  The other "current" code in here is several years old at least and attitude towards certain practices have...changed over time.  So it seems worthwhile to have newer code be mostly consistent with how newer code elsewhere in the tree is done.
Attached patch aarch64-xpcom.patch (obsolete) — Splinter Review
I admit that my C/C++ knowledge is rusty. Still some memcpy() needs to be done and C++ casting as well.
Attachment #8364272 - Attachment is obsolete: true
Attachment #8366677 - Flags: review?(nfroyd)
Comment on attachment 8366677 [details] [diff] [review]
aarch64-xpcom.patch

Review of attachment 8366677 [details] [diff] [review]:
-----------------------------------------------------------------

Closer!  Just a few more things to clear up.

::: mozilla-central.orig/xpcom/reflect/xptcall/src/md/unix/Makefile.in
@@ +55,5 @@
>  endif
>  endif
>  
>  ######################################################################
> +# AARCH64

You don't need this hunk in the Makefile.in if you've modified moz.build.

::: mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcstubs_aarch64.cpp
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "xptcprivate.h"
> +#include "xptiprivate.h"
> +

Please add a check for __AARCH64EL__ here too, so people don't think this file has been checked for endianness concerns.

@@ +35,5 @@
> +
> +    // setup variant array pointer
> +    if(paramCount > PARAM_BUFFER_COUNT) {
> +        dispatchParams = new nsXPTCMiniVariant[paramCount];
> +	}

Braces are aligned under the 'if':

if (...) {
  ...
}

No tabs either, please.

@@ +38,5 @@
> +        dispatchParams = new nsXPTCMiniVariant[paramCount];
> +	}
> +    else {
> +        dispatchParams = paramBuffer;
> +	}

Same brace alignment here too.  And the 'else' should be like so:

if (...) {
} else {
}

@@ +53,5 @@
> +
> +        if(param.IsOut() || !type.IsArithmetic())
> +        {
> +            if (next_gpr < PARAM_GPR_COUNT) {
> +                dp->val.p = (void*)gprData[next_gpr++];

Ah, I see why you didn't refactor like I suggested; unconditionally doing it would pull stuff out of gpr locations when you have floats, say.

It would be better to do this like x86-64 does it, but we can let this slide for now.

@@ +54,5 @@
> +        if(param.IsOut() || !type.IsArithmetic())
> +        {
> +            if (next_gpr < PARAM_GPR_COUNT) {
> +                dp->val.p = (void*)gprData[next_gpr++];
> +			}

Brace alignment.  Here and the rest of the file.
Attachment #8366677 - Flags: review?(nfroyd) → feedback+
Attached patch aarch64-xpcom.patch v.28 (obsolete) — Splinter Review
Makefile.in dropped
AARCH64EL check added
braces and tabs fixed

And I see why x86-64 is nicer ;)
Attachment #8366677 - Attachment is obsolete: true
Attachment #8366910 - Flags: review?(nfroyd)
Comment on attachment 8366910 [details] [diff] [review]
aarch64-xpcom.patch v.28

Review of attachment 8366910 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for perservering!  Just a few minor tweaks; r=me with the tweaks added.

::: mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_aarch64.cpp
@@ +58,5 @@
> +        *freg_args = data;
> +        freg_args++;
> +    } else {
> +        *(double *)stack_args = data;
> +        memcpy(stack_args, &data, sizeof(data));

No need to copy twice; please delete the first assignment to stack_args and leave the memcpy.

@@ +69,5 @@
> +                               double* end,
> +                               float  data)
> +{
> +    if (freg_args < end) {
> +        *(float *)freg_args = data;

memcpy here please.

::: mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcstubs_aarch64.cpp
@@ +37,5 @@
> +
> +    uint32_t paramCount = info->GetParamCount();
> +
> +    // setup variant array pointer
> +    if(paramCount > PARAM_BUFFER_COUNT) {

Space after the |if|.

@@ +48,5 @@
> +    uint64_t* ap = args;
> +    uint32_t next_gpr = 1; // skip first arg which is 'self'
> +    uint32_t next_fpr = 0;
> +    for(uint32_t i = 0; i < paramCount; i++)
> +    {

Space after the |for|, bring the brace up.

@@ +54,5 @@
> +        const nsXPTType& type = param.GetType();
> +        nsXPTCMiniVariant* dp = &dispatchParams[i];
> +
> +        if(param.IsOut() || !type.IsArithmetic())
> +        {

Space after the |if|, bring the brace up.

@@ +63,5 @@
> +            }
> +            continue;
> +        }
> +
> +        switch(type) {

Space after the |switch|.

@@ +132,5 @@
> +            case nsXPTType::T_FLOAT:
> +                if (next_fpr < PARAM_FPR_COUNT) {
> +                    dp->val.f  = *(float *)(fprData + next_fpr++);
> +                } else {
> +                    dp->val.f  = *(float *)ap;

Use memcpy, please (for both sides of the conditional).

@@ +141,5 @@
> +            case nsXPTType::T_DOUBLE:
> +                if (next_fpr < PARAM_FPR_COUNT) {
> +                    dp->val.d  = fprData[next_fpr++];
> +                } else {
> +                    dp->val.d  = *(double*)ap;

Here too.

@@ +178,5 @@
> +    }
> +
> +    result = self->mOuter->CallMethod((uint16_t)methodIndex, info, dispatchParams);
> +
> +    if(dispatchParams != paramBuffer) {

Space after the |if|.
Attachment #8366910 - Flags: review?(nfroyd) → review+
Attached patch xpcom-v29.patch (obsolete) — Splinter Review
I hope that memcpy() are done properly.
Attachment #8366910 - Attachment is obsolete: true
Attachment #8367381 - Flags: review+
Attached patch xpcom v0212 (obsolete) — Splinter Review
reverted memcpy() for T_FLOAT and T_DOUBLE
Attachment #8367381 - Attachment is obsolete: true
Attachment #8374786 - Flags: review?(nfroyd)
Comment on attachment 8374786 [details] [diff] [review]
xpcom v0212

Review of attachment 8374786 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the two changes below.

::: mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcstubs_aarch64.cpp
@@ +130,5 @@
> +            case nsXPTType::T_FLOAT:
> +                if (next_fpr < PARAM_FPR_COUNT) {
> +                    dp->val.f  = *(float *)(fprData + next_fpr++);
> +                } else {
> +                    dp->val.f  = *(float *)ap;

memcpy here please, both branches.

@@ +140,5 @@
> +            case nsXPTType::T_DOUBLE:
> +                if (next_fpr < PARAM_FPR_COUNT) {
> +                    dp->val.d  = fprData[next_fpr++];
> +                } else {
> +                    dp->val.d  = *(double*)ap;

memcpy here please.
Attachment #8374786 - Flags: review?(nfroyd) → review+
Sorry Nathan but I have no idea how to make proper memcpy() in those two places.
Something like:

if (next_fpr < PARAM_FPR_COUNT) {
  memcpy(&dp->val.f, &fprData[next_fpr++], sizeof(dp->val.f));
} else {
  memcpy(&dp->val.f, ap, sizeof(dp->val.f));
}

and

memcpy(&dp->val.d, ap, sizeof(dp->val.d));

ought to work.
Attached patch xpcom v0217 (obsolete) — Splinter Review
With this patch I am able to build firefox v27
Attachment #8374786 - Attachment is obsolete: true
Attachment #8377120 - Flags: review?(nfroyd)
Comment on attachment 8377120 [details] [diff] [review]
xpcom v0217

Review of attachment 8377120 [details] [diff] [review]:
-----------------------------------------------------------------

Did you just make sure that it built, or did you make sure that it runs?  Very different things! :)

r=me with the changes below.

::: mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcstubs_aarch64.cpp
@@ +128,5 @@
> +                break;
> +
> +            case nsXPTType::T_FLOAT:
> +                if (next_fpr < PARAM_FPR_COUNT) {
> +	memcpy(&dp->val.f, &fprData[next_fpr++], sizeof(dp->val.f));

Please don't use tabs to indent all these memcpys; use spaces instead. :)
Attachment #8377120 - Flags: review?(nfroyd) → review+
Attached patch mozilla-963024-v0218.patch (obsolete) — Splinter Review
Attachment #8377120 - Attachment is obsolete: true
Attachment #8377717 - Flags: review+
Keywords: checkin-needed
Why was this not added to moz.build? <https://hg.mozilla.org/integration/mozilla-inbound/rev/14dc29b1586b#l1.12>
Flags: needinfo?(nfroyd)
Flags: needinfo?(marcin)
When I had them in moz.build my build failed so I checked how other arches solved issue and followed them. 

IIRC with v24 it was fine to have .cpp and .s in one entry in moz.build
Flags: needinfo?(nfroyd)
Flags: needinfo?(marcin)
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #17)
> Why was this not added to moz.build?
> <https://hg.mozilla.org/integration/mozilla-inbound/rev/14dc29b1586b#l1.12>

My mistake as a reviewer; they were in moz.build in a previous version of the patch, so I assumed that that continued.

(In reply to Marcin Juszkiewicz from comment #18)
> When I had them in moz.build my build failed so I checked how other arches
> solved issue and followed them. 
> 
> IIRC with v24 it was fine to have .cpp and .s in one entry in moz.build

Well, it seems to be OK in current trunk with at least one other architecture that builds:

http://mxr.mozilla.org/mozilla-central/source/xpcom/reflect/xptcall/src/md/unix/moz.build#259

Perhaps it's a configuration problem on your end?
Um, also, as-landed the patch only seems to have added the build bits and none of the actual source files.  While it's not actively harmful to have the aarch64-specific build bits in the tree, I've elected to back the patch out so that everything can land together as one piece of work:

https://hg.mozilla.org/integration/mozilla-inbound/rev/37de5a3f1a93
https://hg.mozilla.org/mozilla-central/rev/14dc29b1586b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch mozilla-963024-v0219.patch (obsolete) — Splinter Review
This time it is full.

I moved ASM files back to moz.build like it should be - it built fine. Also all files are included in patch.
Attachment #8377717 - Attachment is obsolete: true
Attachment #8378207 - Flags: review+
Comment on attachment 8378207 [details] [diff] [review]
mozilla-963024-v0219.patch

Review of attachment 8378207 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/reflect/xptcall/src/md/unix/xptcstubs_aarch64.cpp
@@ +130,5 @@
> +            case nsXPTType::T_FLOAT:
> +                if (next_fpr < PARAM_FPR_COUNT) {
> +   memcpy(&dp->val.f, &fprData[next_fpr++], sizeof(dp->val.f));
> +                } else {
> +   memcpy(&dp->val.f, ap++, sizeof(dp->val.f));

These calls and the ones below are still wrongly indented.
Oops. Mea culpa.
Attachment #8378207 - Attachment is obsolete: true
Attachment #8378572 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d56b5c1a5573
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Blocks: 1330119
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: