557270 - Build error in jstl.h on s390

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Hommey [:glandium]]

On s390, the build fails with the following message:
jstl.h: In function 'size_t js::RoundUpPow2(size_t)':
jstl.h:198: error: 'result' in class 'js::tl::StaticAssert<false>' does not name a type

This is due to the fact that on s390, pointers are 31 bits (not 32), and as such, JSUword, which is a pointer to a long int, is not equal to size_t, which is 32 bits.

I don't know exactly what this test is supposed to acheive, as it was not there before bug 511750 in the code it replaced...

Comment 1

[Mike Hommey [:glandium]]

Luke, Brendan, as you were both involved in bug 511750, would you have an answer to the last phrase in comment 0 ?

Comment 2

[Luke Wagner [:luke]]

Attachment: tweak

The purpose of the static assert is to check the assumed parameter type of the JS_CEILING_LOG2W macro which is stated, in comments, to be JSUword.  Vectors and things use a size_t, so one fix would be to change the parameter type of JS_{CEILING,FLOOR}_LOG2W and js_FloorLog2wImpl to be JSSize.

Comment 3

[Luke Wagner [:luke]]

Mike: does this patch fix the build errors you are seeing?

Comment 4

[Mike Hommey [:glandium]]

I could confirm it does.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 437081 [details] [diff] [review]
tweak

Great; Brendan does this seem like the right fix to you?  I guess the underlying issue is that jsuword means "an int that can be cast to and from a pointer" rather than "a machine-word-sized integer", which JSSize/size_t seem to mean.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 437081 [details] [diff] [review]
tweak

JSSize, JSWord, etc. are the bad old "NSPR 2" names forked into JS in 1998 from NSPR. Apart from the old NSPR code forked then, we use size_t not JSSize. So please do use size_t if you can. I'm fine with it being the type for bit-fiddling, even if it is not big enough to hold a pointer. Most of our bit macrology works only for uint32, or rather, assumes at most 32 bits, anyway (check me if I'm misremembering).

/be

Comment 7

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/f72cc6add994

Oops, accidentally left in the StaticAssert that size_t == JSSize... will take that out on my next commit.

Comment 8

[Mike Hommey [:glandium]]

(In reply to comment #7)
> http://hg.mozilla.org/tracemonkey/rev/f72cc6add994
> 
> Oops, accidentally left in the StaticAssert that size_t == JSSize... will take
> that out on my next commit.

Well, it doesn't hurt.

Comment 9

[Luke Wagner [:luke]]

True, but since JS_CEILNG_LOG2W now takes a size_t it was a bit of a non sequitur.
Gone with http://hg.mozilla.org/tracemonkey/rev/3e7cbba99492

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f72cc6add994