Closed Bug 898936 Opened 6 years ago Closed 6 years ago

Fix some build warnings on ARM gcc 4.8.1

Categories

(Core :: JavaScript Engine, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: terrence, Assigned: terrence)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch fix_arm_build_warnings-v0.diff (obsolete) — Splinter Review
In particular, I'm surprised the changed lines on RIS and Pool were not reported as compile errors.
Attachment #782356 - Flags: review?(mrosenberg)
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?
Attachment #782356 - Flags: review?(mrosenberg)
(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.
Rebased and applied review comments.
Attachment #782356 - Attachment is obsolete: true
Attachment #784808 - Flags: review?(mrosenberg)
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.
Attachment #784808 - Flags: review?(mrosenberg) → review+
Or perhaps it's not you, since the backout has the same crash.
https://hg.mozilla.org/mozilla-central/rev/b8eddc7a49b0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Thanks for the investigation, philor! I would have been *really* surprised if it was this patch, given that this code only builds on ARM.
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
You need to log in before you can comment on or make changes to this bug.