Closed Bug 1194139 Opened 4 years ago Closed 4 years ago

IonMonkey: MIPS32: Split shareable code from mips32 to mips-shared

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: hev, Assigned: hev)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 22 obsolete files)

24.90 KB, patch
nbp
: review+
hev
: review?
rankov
RyanVM
: checkin+
Details | Diff | Splinter Review
21.64 KB, patch
nbp
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
2.32 KB, patch
nbp
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
5.82 KB, patch
nbp
: review+
cbook
: checkin+
Details | Diff | Splinter Review
165.46 KB, patch
nbp
: review+
Details | Diff | Splinter Review
161.70 KB, patch
hev
: review+
hev
: checkin+
Details | Diff | Splinter Review
Split files in 3-5 steps, do that one by one, may be easy review:
  1. Architecture-mips32.{h,cpp}
  2. Assember-mips32.{h,cpp}
  ...
Assignee: nobody → r
Blocks: 1140954
Attachment #8647414 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8647414 [details] [diff] [review]
0001-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch

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

This sounds fine to me, but I think Rankov might want to have a look at this patch too.

::: js/src/jit/mips-shared/Architecture-mips.cpp
@@ +44,5 @@
> +    isSet = true;
> +    return flags;
> +#endif
> +
> +    return false;

nit: uint32_t ?
Attachment #8647414 - Flags: review?(nicolas.b.pierron)
Attachment #8647414 - Flags: review?(branislav.rankov)
Attachment #8647414 - Flags: review+
Comment on attachment 8647414 [details] [diff] [review]
0001-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch

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

I see some places where #ifdef's could be avoided if you leave the code in the mips32 file.

I think that it would be easier to review if we can see how it will look like when mips64 is added. Can you attach the patch that will add mips64 version of these files?

::: js/src/jit/mips-shared/Architecture-mips.h
@@ +75,5 @@
> +        a3 = r7,
> +        t0 = r8,
> +        t1 = r9,
> +        t2 = r10,
> +        t3 = r11,

Are you planing an #ifdef here for MIPS64? You will have a4 - a7 here.

@@ +109,5 @@
> +
> +    static const char* GetName(Code code) {
> +        MOZ_ASSERT(code < Total);
> +        static const char * const Names[] = { "zero", "at", "v0", "v1", "a0", "a1", "a2", "a3",
> +                                              "t0",   "t1", "t2", "t3", "t4", "t5", "t6", "t7",

Seems like an future #idef also.

@@ +128,5 @@
> +    static const uint32_t Allocatable = 14;
> +
> +    typedef uint32_t SetType;
> +    static const SetType AllMask = 0xffffffff;
> +    static const SetType ArgRegMask = (1 << a0) | (1 << a1) | (1 << a2) | (1 << a3);

Future #ifdef?

@@ +137,5 @@
> +        (1 << Registers::a0) |
> +        (1 << Registers::a1) |
> +        (1 << Registers::a2) |
> +        (1 << Registers::a3) |
> +        (1 << Registers::t0) |

Future #ifdef?

@@ +162,5 @@
> +
> +    static const SetType WrapperMask =
> +        VolatileMask |         // = arguments
> +        (1 << Registers::t0) | // = outReg
> +        (1 << Registers::t1);  // = argBase

Another potential #ifdef.

@@ +283,5 @@
> +
> +    static const uint32_t Total = 64;
> +    static const uint32_t TotalDouble = 16;
> +    static const uint32_t TotalSingle = 32;
> +    static const uint32_t Allocatable = 42;

This is also different for mips64.

@@ +300,5 @@
> +         (1ULL << FloatRegisters::f28) |
> +         (1ULL << FloatRegisters::f30)) << 32;
> +
> +    // f20-single and f21-single alias f20-double ...
> +    static const SetType NonVolatileMask =

Another difference with mips64.
Attachment #8647414 - Flags: review?(branislav.rankov) → review-
Depends on: 1194588
(In reply to Branislav Rankov [:rankov] from comment #3)
> Comment on attachment 8647414 [details] [diff] [review]
> 0001-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch
> 
> Review of attachment 8647414 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see some places where #ifdef's could be avoided if you leave the code in
> the mips32 file.
> 
> I think that it would be easier to review if we can see how it will look
> like when mips64 is added. Can you attach the patch that will add mips64
> version of these files?
> 
> ::: js/src/jit/mips-shared/Architecture-mips.h
> @@ +75,5 @@
> > +        a3 = r7,
> > +        t0 = r8,
> > +        t1 = r9,
> > +        t2 = r10,
> > +        t3 = r11,
> 
> Are you planing an #ifdef here for MIPS64? You will have a4 - a7 here.
> 
> @@ +109,5 @@
> > +
> > +    static const char* GetName(Code code) {
> > +        MOZ_ASSERT(code < Total);
> > +        static const char * const Names[] = { "zero", "at", "v0", "v1", "a0", "a1", "a2", "a3",
> > +                                              "t0",   "t1", "t2", "t3", "t4", "t5", "t6", "t7",
> 
> Seems like an future #idef also.
> 
> @@ +128,5 @@
> > +    static const uint32_t Allocatable = 14;
> > +
> > +    typedef uint32_t SetType;
> > +    static const SetType AllMask = 0xffffffff;
> > +    static const SetType ArgRegMask = (1 << a0) | (1 << a1) | (1 << a2) | (1 << a3);
> 
> Future #ifdef?
> 
> @@ +137,5 @@
> > +        (1 << Registers::a0) |
> > +        (1 << Registers::a1) |
> > +        (1 << Registers::a2) |
> > +        (1 << Registers::a3) |
> > +        (1 << Registers::t0) |
> 
> Future #ifdef?
> 
> @@ +162,5 @@
> > +
> > +    static const SetType WrapperMask =
> > +        VolatileMask |         // = arguments
> > +        (1 << Registers::t0) | // = outReg
> > +        (1 << Registers::t1);  // = argBase
> 
> Another potential #ifdef.
> 
> @@ +283,5 @@
> > +
> > +    static const uint32_t Total = 64;
> > +    static const uint32_t TotalDouble = 16;
> > +    static const uint32_t TotalSingle = 32;
> > +    static const uint32_t Allocatable = 42;
> 
> This is also different for mips64.
> 
> @@ +300,5 @@
> > +         (1ULL << FloatRegisters::f28) |
> > +         (1ULL << FloatRegisters::f30)) << 32;
> > +
> > +    // f20-single and f21-single alias f20-double ...
> > +    static const SetType NonVolatileMask =
> 
> Another difference with mips64.

Thanks for your advice. ;)
Attachment #8647414 - Attachment is obsolete: true
Attachment #8647897 - Flags: review?(nicolas.b.pierron)
Attachment #8647897 - Flags: review?(branislav.rankov)
Attached patch Architecture-mips64.patch (obsolete) — Splinter Review
This is a temporary mips64 implement for Architecture. hopes help for review.
For refactor floatregisters updated.
Attachment #8647897 - Attachment is obsolete: true
Attachment #8647897 - Flags: review?(nicolas.b.pierron)
Attachment #8647897 - Flags: review?(branislav.rankov)
Attachment #8648484 - Flags: review?(nicolas.b.pierron)
Attachment #8648484 - Flags: review?(branislav.rankov)
Attached patch Architecture-mips64.patch (obsolete) — Splinter Review
This is a temporary mips64 implement for Architecture. hopes help for review.
Attachment #8647898 - Attachment is obsolete: true
Comment on attachment 8648484 [details] [diff] [review]
0001-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch

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

I think it would be easier to have 2 versions before refactoring the 2.
Implementing a shared version first might be appealing, but I think it make more sense to optimize both before trying to refactor the common parts.

What do you think?

::: js/src/jit/mips-shared/Architecture-mips.h
@@ +299,5 @@
> +    static const SetType SpreadSingle = SetType(1) << (uint32_t(Single) * TotalPhys);
> +    static const SetType SpreadDouble = SetType(1) << (uint32_t(Double) * TotalPhys);
> +    static const SetType SpreadScalar = SpreadSingle | SpreadDouble;
> +    static const SetType SpreadVector = 0;
> +    static const SetType Spread = SpreadScalar | SpreadVector;

As commented on Bug 1194588, this should not be used on mips32.

@@ +361,5 @@
> +    bool isFloat32x4() const { return false; }
> +
> +    Code code() const {
> +        MOZ_ASSERT(!isInvalid());
> +        return Code(reg_  | (kind_ << 5));

Assert that the return value is no larger than the total number of registers.

::: js/src/jit/mips32/Architecture-mips32.h
@@ +103,1 @@
>      static FloatRegister FromCode(uint32_t i) {

Assert that the code is no larger than the total number of registers.
Attachment #8648484 - Flags: review?(nicolas.b.pierron) → review-
Please review. thanks ;)
Attachment #8648484 - Attachment is obsolete: true
Attachment #8648484 - Flags: review?(branislav.rankov)
Attachment #8650320 - Flags: review?(nicolas.b.pierron)
Attachment #8650320 - Flags: review?(branislav.rankov)
Attached patch Architecture-mips64.patch (obsolete) — Splinter Review
Attachment #8648485 - Attachment is obsolete: true
Comment on attachment 8650320 [details] [diff] [review]
Part 1: Architecture-mips

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

Thanks, this is easier to review, but I do expect a follow-up patch to pack the register code properly on mips32.

::: js/src/jit/mips-shared/Architecture-mips.cpp
@@ +3,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "jit/mips-shared/Architecture-mips.h"

This file and the corresponding header should be named mips-shared.
This is not a big deal for this file at the moment, but this would be expected for the MacroAssembler-mips-shared.cpp file.

::: js/src/jit/mips-shared/Architecture-mips.h
@@ +239,5 @@
> +        f30,
> +        f31,
> +        invalid_freg
> +    };
> +    typedef FPRegisterID Code;

in a follow-up patch, you should change this to an integer type.

@@ +248,5 @@
> +        double d;
> +    };
> +
> +    static const char* GetName(Code code) {
> +        static const char * const Names[] = { "f0", "f1", "f2", "f3",  "f4", "f5",  "f6", "f7",

In a follow-up patch; As the array is not typed, you should use the Encoding instead of the code.

::: js/src/moz.build
@@ +462,2 @@
>      ]
> +    if CONFIG['JS_CODEGEN_MIPS32']:

Update the previous condition as well.
Attachment #8650320 - Flags: review?(nicolas.b.pierron) → review+
This patch just only rename Architecture-mips to Architrecture-mips-shared.
Attachment #8651808 - Flags: review?(nicolas.b.pierron)
Attachment #8651809 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8651808 [details] [diff] [review]
Part 1.1: Rename Architecture-mips -> Architecture-mips-shared

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

Fold this patch with the previous one.
Attachment #8651808 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8651809 - Attachment is obsolete: true
Attachment #8651809 - Flags: review?(nicolas.b.pierron)
Attachment #8651823 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8651823 [details] [diff] [review]
0003-IonMonkey-MIPS-Redefine-FloatRegisters-Code-and-use-.patch

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

::: js/src/jit/mips32/Architecture-mips32.h
@@ +182,4 @@
>      }
>      Encoding encoding() const {
>          MOZ_ASSERT(!isInvalid());
> +        return Encoding(code_ | (kind_ << 5));

Why do you need the kind_?
The encoding is a type-less register, it should only contain a value listed as part of the enum.
Attachment #8651823 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #17)
> Comment on attachment 8651823 [details] [diff] [review]
> 0003-IonMonkey-MIPS-Redefine-FloatRegisters-Code-and-use-.patch
> 
> Review of attachment 8651823 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips32/Architecture-mips32.h
> @@ +182,4 @@
> >      }
> >      Encoding encoding() const {
> >          MOZ_ASSERT(!isInvalid());
> > +        return Encoding(code_ | (kind_ << 5));
> 
> Why do you need the kind_?
> The encoding is a type-less register, it should only contain a value listed
> as part of the enum.

I'm surprised at this, I think just only the code contain type, but I'm not sure. I looked this part in arm and it same as mips32, so I don't modify it. ;)
Attachment #8651823 - Attachment is obsolete: true
Attachment #8651830 - Flags: review?(nicolas.b.pierron)
(In reply to Heiher from comment #18)
> (In reply to Nicolas B. Pierron [:nbp] from comment #17)
> > Comment on attachment 8651823 [details] [diff] [review]
> > 0003-IonMonkey-MIPS-Redefine-FloatRegisters-Code-and-use-.patch
> > 
> > Review of attachment 8651823 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/jit/mips32/Architecture-mips32.h
> > @@ +182,4 @@
> > >      }
> > >      Encoding encoding() const {
> > >          MOZ_ASSERT(!isInvalid());
> > > +        return Encoding(code_ | (kind_ << 5));
> > 
> > Why do you need the kind_?
> > The encoding is a type-less register, it should only contain a value listed
> > as part of the enum.
> 
> I'm surprised at this, I think just only the code contain type, but I'm not
> sure. I looked this part in arm and it same as mips32, so I don't modify it.
> ;)

Ok, then this is a bug in arm.
Attached patch Part 2: Assembler-mips (obsolete) — Splinter Review
Attachment #8653430 - Flags: review?(nicolas.b.pierron)
Attachment #8650320 - Attachment description: 0001-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch → Part 1: Architecture-mips
Attachment #8651808 - Attachment description: 0002-IonMonkey-MIPS-Rename-Architecture-mips-to-Architect.patch → Part 1.1: Rename Architecture-mips -> Architecture-mips-shared
Attachment #8651830 - Attachment description: 0003-IonMonkey-MIPS-Redefine-FloatRegisters-Code-and-use-.patch → Part 1.2: Redefine FloatRegisters::Code
Attachment #8653430 - Attachment description: 0004-IonMonkey-MIPS-Split-shareable-code-to-mips-shared-i.patch → Part 2: Assembler-mips
Attached patch Part 2: Assembler-mips (obsolete) — Splinter Review
Update for easier review.
Attachment #8653430 - Attachment is obsolete: true
Attachment #8653430 - Flags: review?(nicolas.b.pierron)
Attachment #8653508 - Flags: review?(nicolas.b.pierron)
Attached patch Assembler-mips64.patch (obsolete) — Splinter Review
Blocks: 1199535
Attachment #8653836 - Flags: review?(nicolas.b.pierron)
Blocks: 1090957
Status: NEW → ASSIGNED
Attachment #8650320 - Flags: checkin?
Attachment #8651808 - Flags: checkin?
Depends on: 1198606, 1199565
Depends on: 1199568
Attachment #8650320 - Flags: checkin? → checkin+
Attachment #8651808 - Flags: checkin? → checkin+
Attached patch Part 2: Assembler-mips (obsolete) — Splinter Review
Update with Bug 986680 changes. otherwise, can't apply it into master.
Attachment #8653508 - Attachment is obsolete: true
Attachment #8653508 - Flags: review?(nicolas.b.pierron)
Attachment #8654675 - Flags: review?(nicolas.b.pierron)
Attachment #8651830 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8653836 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8651830 - Flags: checkin?
Keywords: checkin-needed
Attachment #8651830 - Flags: checkin? → checkin+
(In reply to Carsten Book [:Tomcat] from comment #30)
> sorry had to back this out in
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=6782a855c243 seems one of this changes caused a bustage
> like:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=13496016&repo=mozilla-
> inbound

Sorry, it's my mistake, It caused by Bug 1198628. I will trying build with -Werror=sign-cmopare and fix that.
Flags: needinfo?(r)
Attachment #8653836 - Flags: checkin?
Keywords: checkin-needed
Depends on: 1202222
Comment on attachment 8654675 [details] [diff] [review]
Part 2: Assembler-mips

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

::: js/src/jit/mips-shared/Assembler-mips-shared.h
@@ +109,5 @@
> +static MOZ_CONSTEXPR_VAR FloatRegister ScratchFloat32Reg = SCRATCH_FLOAT32_REG;
> +static MOZ_CONSTEXPR_VAR FloatRegister ScratchDoubleReg = SCRATCH_DOUBLE_REG;
> +static MOZ_CONSTEXPR_VAR FloatRegister ScratchSimdReg = InvalidFloatReg;
> +static MOZ_CONSTEXPR_VAR FloatRegister SecondScratchFloat32Reg = SECOND_SCRATCH_FLOAT32_REG;
> +static MOZ_CONSTEXPR_VAR FloatRegister SecondScratchDoubleReg = SECOND_SCRATCH_DOUBLE_REG;

If this is architecture specific, do not create a macro to alias it in the generic file, but just move the line to the architecture specific file.
There is no value in having such macro, worse this might let other developers think that they can rely on such macro, which is not intended.

@@ +626,5 @@
> +        }
> +    }
> +};
> +
> +class BaseAssembler : public AssemblerShared

nit: s/BaseAssembler/AssemblerMIPSShared/g
Attachment #8654675 - Flags: review?(nicolas.b.pierron)
Please review https://github.com/heiher/gecko-dev/commit/mips32-shared-r1

Thank you!
Flags: needinfo?(nicolas.b.pierron)
Attachment #8653836 - Flags: checkin? → checkin+
(In reply to Heiher [:hev] from comment #35)
> Please review https://github.com/heiher/gecko-dev/commit/mips32-shared-r1

Can you still attach a patch, in addition to the link, such that I can set a review flag, and that sheriff can cherry-pick your patches.

Also, you might want to request commit access, for pushing to mozilla-inbound your-self.
Flags: needinfo?(nicolas.b.pierron)
https://github.com/heiher/gecko-dev/commit/mips32-shared-r1

thanks.
Attachment #8654675 - Attachment is obsolete: true
Attachment #8658120 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8658120 [details] [diff] [review]
Part 2: Assembler-mips

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

Apart from the following question, this patch looks good.

::: js/src/jit/mips32/Assembler-mips32.cpp
@@ +138,4 @@
>  }
>  
>  void
> +AssemblerMIPSShared::executableCopy(uint8_t* buffer)

Why do we have any AssemblerMIPSShared in the mips32.cpp file?
Comment on attachment 8658120 [details] [diff] [review]
Part 2: Assembler-mips

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

::: js/src/jit/mips32/Assembler-mips32.cpp
@@ +138,4 @@
>  }
>  
>  void
> +AssemblerMIPSShared::executableCopy(uint8_t* buffer)

Because executableCopy for mips32 and mips64 are same prototype, but implementation are different. so declare it in AssemblerMIPSShared and implement in architecture file.
Comment on attachment 8658120 [details] [diff] [review]
Part 2: Assembler-mips

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

::: js/src/jit/mips32/Assembler-mips32.cpp
@@ +138,4 @@
>  }
>  
>  void
> +AssemblerMIPSShared::executableCopy(uint8_t* buffer)

I think this would be less confusing if you keep the declaration of this function in the Assembler of mips32 and mips64, and not in mips-shared.

Also do the same for any AssemblerMIPSShared function which are present in this file, otherwise this would be confusing when you would be looking for these functions.  The MacroAssembler is kind of the opposite exception as we are supposed to have only one interface, and this is the reasons why I am adding the macro-annotations to locate the definitions.
Attachment #8658120 - Flags: review?(nicolas.b.pierron) → review+
Attached patch Part 2: Assembler-mips v2 (obsolete) — Splinter Review
Add review opinion.
Attachment #8659682 - Flags: review+
Attachment #8659682 - Flags: checkin?
Keywords: checkin-needed
Attached patch Part 4: LIR-mips (obsolete) — Splinter Review
https://github.com/heiher/gecko-dev/commit/mips32-shared-r2
Attachment #8659731 - Flags: review?(nicolas.b.pierron)
Attached patch Part 5: Lowering-mips (obsolete) — Splinter Review
https://github.com/heiher/gecko-dev/commit/mips32-shared-r3
Attachment #8659739 - Flags: review?(nicolas.b.pierron)
Attached patch Part 6: BaselineIC-mips (obsolete) — Splinter Review
Attachment #8659742 - Flags: review?(nicolas.b.pierron)
Attached patch Part 7: Bailouts-mips (obsolete) — Splinter Review
https://github.com/heiher/gecko-dev/commit/mips32-shared-r4
Attachment #8659750 - Flags: review?(nicolas.b.pierron)
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=13938933&repo=mozilla-inbound
Flags: needinfo?(r)
Fix check_spidermonkey_style.py failure.
Attachment #8659682 - Attachment is obsolete: true
Attachment #8659682 - Flags: checkin?
Flags: needinfo?(r)
Attachment #8659766 - Flags: review+
Attachment #8659766 - Flags: checkin?
Keywords: checkin-needed
Attached patch Part 5: Lowering-mips (obsolete) — Splinter Review
Update v2, Fix check_spidermonkey_style.py failure.
Attachment #8659739 - Attachment is obsolete: true
Attachment #8659739 - Flags: review?(nicolas.b.pierron)
Attachment #8659768 - Flags: review?(nicolas.b.pierron)
Attached patch Part 7: Bailouts-mips (obsolete) — Splinter Review
Update v2, Fix check_spidermonkey_style.py failure.
Attachment #8659750 - Attachment is obsolete: true
Attachment #8659750 - Flags: review?(nicolas.b.pierron)
Attachment #8659770 - Flags: review?(nicolas.b.pierron)
I have do build test at every step, I'm sure the builds are passed from part 2 to 7.

When I extract MoveEmitter-mips32, and build failed in Assembler-mips32, some symbols are missing in Assembler-mips32.cpp and declare in Assembler-mips-shared.cpp. In face, I think these classes are in same unifeid cpp source file, so build ok before this.
Attachment #8659805 - Flags: review?(nicolas.b.pierron)
Attached patch Part 8: MoveEmitter-mips (obsolete) — Splinter Review
https://github.com/heiher/gecko-dev/commit/mips32-shared-r5
Attachment #8659808 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8659805 [details] [diff] [review]
Part 2.1: Fix build failure caused by extract MoveEmitter-mips32

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

::: js/src/jit/mips-shared/Assembler-mips-shared.h
@@ +1247,5 @@
> +
> +    bool read() {
> +        if (!reader_.more())
> +            return false;
> +        offset_ = reader_.readUnsigned();

Couldn't we inline this code in the TraceJumpRelocations function?
Attachment #8659805 - Flags: review?(nicolas.b.pierron)
I think this leave-open madness should end after the part 2.1.
Make a meta-bug, rename this one, and make blockers of the meta bug to handle each step.
Attachment #8659766 - Flags: checkin? → checkin+
Attachment #8659805 - Attachment is obsolete: true
Attachment #8650321 - Attachment is obsolete: true
Attachment #8653546 - Attachment is obsolete: true
Attachment #8659684 - Attachment is obsolete: true
Depends on: 1204187
Attachment #8659808 - Attachment is obsolete: true
Attachment #8659808 - Flags: review?(nicolas.b.pierron)
Attachment #8659770 - Attachment is obsolete: true
Attachment #8659770 - Flags: review?(nicolas.b.pierron)
Attachment #8659742 - Attachment is obsolete: true
Attachment #8659742 - Flags: review?(nicolas.b.pierron)
Attachment #8659768 - Attachment is obsolete: true
Attachment #8659768 - Flags: review?(nicolas.b.pierron)
Attachment #8659731 - Attachment is obsolete: true
Attachment #8659731 - Flags: review?(nicolas.b.pierron)
Depends on: 1204189
Depends on: 1204191
Depends on: 1204192
Depends on: 1204193
Depends on: 1204194
Depends on: 1204214
Depends on: 1204422
Depends on: 1205134
Depends on: 1205135
Keywords: leave-open
Depends on: 1205229
Blocks: 1205524
Depends on: 1218652
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.