898936 - Fix some build warnings on ARM gcc 4.8.1

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: fix_arm_build_warnings-v0.diff

In particular, I'm surprised the changed lines on RIS and Pool were not reported as compile errors.

Comment 1

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 782356 [details] [diff] [review]
fix_arm_build_warnings-v0.diff

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

::: js/src/ion/RegisterAllocator.cpp
@@ +365,5 @@
>      for (size_t blockIndex = 0; blockIndex < graph.numBlocks(); blockIndex++) {
>          LBlock *block = graph.getBlock(blockIndex);
>          MBasicBlock *mir = block->mir();
>  
> +        printf("\nBlock %zd", blockIndex);

Does the 'z' length modifier exist in windows?

::: js/src/ion/arm/Assembler-arm.h
@@ +561,5 @@
>  };
>  
>  struct RIS
>  {
> +    uint32_t ShiftAmount;

This isn't attempting to set ShiftAmount to 5; it is declaring a bit field that is 5 bits wide.

@@ +571,5 @@
>        : ShiftAmount(imm)
>      {
>          JS_ASSERT(ShiftAmount == imm);
>      }
> +    explicit RIS(Reg r) : ShiftAmount(5) { }

... This one makes significantly less sense.  It is supposed to be r.ShiftAmount.

::: js/src/ion/shared/IonAssemblerBuffer.h
@@ -202,5 @@
>              cur = tail;
>              cur_off = bufferSize;
>          }
>          int count = 0;
> -        char sigil;

Is it possible that this is only used in debug code?

Comment 2

[Terrence Cole [:terrence]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #1)
> Comment on attachment 782356 [details] [diff] [review]
> fix_arm_build_warnings-v0.diff
> 
> Review of attachment 782356 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/RegisterAllocator.cpp
> @@ +365,5 @@
> >      for (size_t blockIndex = 0; blockIndex < graph.numBlocks(); blockIndex++) {
> >          LBlock *block = graph.getBlock(blockIndex);
> >          MBasicBlock *mir = block->mir();
> >  
> > +        printf("\nBlock %zd", blockIndex);
> 
> Does the 'z' length modifier exist in windows?

Oh my, of course not. MSDN seems to think 'z' should be spelled 'I' for some reason. I guess I'll just upcast then, unless we have the right macros lying about in mfbt or something.

> ::: js/src/ion/arm/Assembler-arm.h
> @@ +561,5 @@
> >  };
> >  
> >  struct RIS
> >  {
> > +    uint32_t ShiftAmount;
> 
> This isn't attempting to set ShiftAmount to 5; it is declaring a bit field
> that is 5 bits wide.
> 
> @@ +571,5 @@
> >        : ShiftAmount(imm)
> >      {
> >          JS_ASSERT(ShiftAmount == imm);
> >      }
> > +    explicit RIS(Reg r) : ShiftAmount(5) { }
> 
> ... This one makes significantly less sense.  It is supposed to be
> r.ShiftAmount.

Ah, sorry. I'm used to TitleCase for constants in the GC at least, so I assumed that was your intent here.

> ::: js/src/ion/shared/IonAssemblerBuffer.h
> @@ -202,5 @@
> >              cur = tail;
> >              cur_off = bufferSize;
> >          }
> >          int count = 0;
> > -        char sigil;
> 
> Is it possible that this is only used in debug code?

I greped for referenced and am building debug, so I hope not.

Comment 3

[Terrence Cole [:terrence]]

Attachment: fix_arm_build_warnings-v1.diff

Rebased and applied review comments.

Comment 4

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 784808 [details] [diff] [review]
fix_arm_build_warnings-v1.diff

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

::: js/src/ion/arm/Assembler-arm.h
@@ +574,1 @@
>          JS_ASSERT(ShiftAmount == imm);

Pretty sure these two are asserting the same thing.  If imm < (1 << 5) then ShiftAmount == imm.
if imm >= (1<<5) then ShiftAmount != imm.

Comment 5

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79684dc64173

Comment 6

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6db59b4778c0 - seems sort of odd, but only on debug 10.7, mochitest-4 was crashing like https://tbpl.mozilla.org/php/getParsedLog.php?id=26416799&tree=Mozilla-Inbound

Comment 7

[Phil Ringnalda (:philor)]

Or perhaps it's not you, since the backout has the same crash.

Comment 8

[Phil Ringnalda (:philor)]

Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/b8eddc7a49b0

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b8eddc7a49b0

Comment 10

[Terrence Cole [:terrence]]

Thanks for the investigation, philor! I would have been *really* surprised if it was this patch, given that this code only builds on ARM.

Comment 11

[Jacek Caban]

I pushed a small fixup for mingw-w64 (on win64, long is still 32-bit and the cast is considered as error in GCC):

https://hg.mozilla.org/integration/mozilla-inbound/rev/692acb03e357

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/692acb03e357