(inactive) Jim Chen [:jchen] [:darchons] Reporter
	Description • 6 years ago

I see this error when compiling using a newer version of Clang under x86.

> MacroAssembler.cpp:1058:19: error: comparison 'intptr_t' (aka 'int') < -2147483648 is always false [-Werror,-Wtautological-constant-compare]

Seems like the values at [1] should be unsigned. Otherwise `1 << 31` becomes -2147483648.

[1] https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/js/src/jit/MacroAssembler.cpp#1058

	Steve Fink [:sfink] [:s:] Assignee
	Comment 1 • 6 years ago

(In reply to Jim Chen [:jchen] [:darchons] from comment #0)
> I see this error when compiling using a newer version of Clang under x86.
> 
> > MacroAssembler.cpp:1058:19: error: comparison 'intptr_t' (aka 'int') < -2147483648 is always false [-Werror,-Wtautological-constant-compare]
> 
> Seems like the values at [1] should be unsigned. Otherwise `1 << 31` becomes
> -2147483648.

Wow, you're absolutely correct. I'm not sure why all the builds didn't scream and complain about this. Thank you!

	Steve Fink [:sfink] [:s:] Assignee
	Comment 2 • 6 years ago

My first thought was to just switch to 1u << 31. But that seems a little weird, since it compares a signed value to an unsigned value. I switched to <= 0x7fffffff to keep everything in consistent types.

But I'm not entirely sure about this stuff anymore.

    intptr_t maxOffset = std::max(std::abs(nurseryPosAddr - zoneAddr),
                                  std::abs(nurseryEndAddr - zoneAddr));

Subtracting two pointers gives a ptrdiff_t, which is signed. std::abs keeps the ptrdiff_t, though it only works if the value is representable. std::max doesn't mess anything up.

On 64-bit, it seems like we depend on pointers sharing some high bits in other places, so it doesn't seem like there's any risk of overflow.

On 32-bit, I'm not entirely sure what will happen. Technically, I can't find an argument where this is guaranteed to be defined behavior. On the other hand, the only purpose of this goop is to see if adding the offset back will retrieve the original value, and it's hard for me to see how a sane compiler would do anything different. Specifically, if this test passes, then we'll be assuming that

  zoneAddr + (nurseryPosAddr - zoneAddr) is the same as nurseryPosAddr

with an added constraint that (nurseryPosAddr - zoneAddr) fits into a signed 32 bit operand. I'm pretty sure that this should be safe now, and I'm also pretty sure that a language lawyer would not be happy.

Perhaps it would be best to remove this optimization to avoid thinking too hard on it?

	Jan de Mooij [:jandem]
	Comment 3 • 6 years ago

(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> Perhaps it would be best to remove this optimization to avoid thinking too
> hard on it?

Do you know if doing that regresses perf (measurably)? It's very subtle and we probably don't hit the interesting edge cases a lot, so removing it is probably a good idea...

	Jan de Mooij [:jandem]
	Comment 4 • 6 years ago

Comment on attachment 8964642 [details] [diff] [review]
Keep pointer comparisons within same types

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

Cancelling per comment 3, unless you think we should keep it.

	Steve Fink [:sfink] [:s:] Assignee
	Comment 5 • 6 years ago

This code has been deleted.