1477742 - Silence a -Wtautological-constant-compare in MacroAssembler.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[(Away)]

13:55:42     INFO -  z:/build/build/src/js/src/jit/MacroAssembler.cpp(1064,19):  error: comparison 'intptr_t' (aka 'int') < -2147483648 is always false [-Werror,-Wtautological-constant-compare]
13:55:42     INFO -      if (maxOffset < (1 << 31)) {
13:55:42     INFO -          ~~~~~~~~~ ^ ~~~~~~~~~
13:55:42     INFO -  1 error generated.

clang-cl currently has -Werror disabled due to too much noise. Cleanup is ongoing, but Spidermonkey is already -Werror clean except for this one issue.

Most clang-cl builds don't mind the above code because this diagnostic was split up into off-by-default parts in https://github.com/llvm-mirror/clang/commit/b5b3d436a1831ddb4fdf2e4c9c07fb60807b70e5.

But our static analysis builders are stuck on an old version of clang, so they still complain.

Comment 1

[(Away)]

Well, I tried to cheat my way out of this by writing `1u << 31`, which trades the -Wtautological-constant-compare for a -Wsign-compare, which is disabled in our clang-cl builds, but the Android builders won't let me get away with it.

sfink, have any better ideas?

Comment 2

[Steve Fink [:sfink] [:s:]]

(In reply to David Major [:dmajor] from comment #1)
> sfink, have any better ideas?

Yes. Rip out my code. The minor optimization isn't worth the complexity. Maybe if we did it in a more general way (eg statically track the contents of various registers to see if there's a nearby pointer that we could use), but a special case for a small piece of code (that isn't even enabled by default) is mostly pointless.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Remove dubious pointer computation optimization

jandem, sending review to you since we've discussed this in the past (I think you even suggested doing this.) Thanks!

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8994252 [details] [diff] [review]
Remove dubious pointer computation optimization

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

I agree this makes sense.

Comment 5

[(Away)]

Is it ok if I push this for you?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/721e491ecd01
Remove dubious pointer computation optimization, r=jandem

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/721e491ecd01