1029132 - Fix a vacuous-comparison warning for size_t(FAKE_JIT_TOP_FOR_BAILOUT) >= 0

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jeff Walden [:Waldo]]

Fix a vacuous-comparison warning for  size_t(FAKE_JIT_TOP_FOR_BAILOUT) >= 0

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Add mozilla::IsInRange method to write out in-range tests more simply. NOT REVIEWED YET

A little helper function that's useful in this patch.  Does the API seem reasonable/right?

We probably should also add a contains() method to Range.  But I don't need it here, there's a question about what behavior is right for fenceposting, and creating a Range to use that instead here is somewhat awkward.

(The *actual* bug patch just builds on this, hence the weird component for this.)

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Use IsInRange for FAKE_JIT_TOP_FOR_BAILOUT range assertions

Comment 3

[Jeff Walden [:Waldo]]

Oh, nbp, note that I changed the assertion in effect from a < to a <=.  It appears to me that that's a harmless change, because you don't actually care if the fake address plus layout struct bumped up *exactly* against 0x1000.  (Plus, your fake address is just an arbitrary constant, so it's doubtful you care about the address one past fake + amount.)  Tell me if it's not!

Comment 4

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8444689 [details] [diff] [review]
Use IsInRange for FAKE_JIT_TOP_FOR_BAILOUT range assertions

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

Indeed this is a constant, which is used to identify failures on crash-stat without having security issues.
I am fine with this patch as this provide some consistency :)

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8444686 [details] [diff] [review]
Add mozilla::IsInRange method to write out in-range tests more simply.  NOT REVIEWED YET

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

I think IsInRange should instead check for a half-open range [start, end).  That seems more consistent with how ArrayEnd is defined, and more consistent with the standard library.

f+'ing instead of r+'ing because I need to think about the T/U relationship and also because I am interested in your thoughts on that.

::: mfbt/ArrayUtils.h
@@ +85,5 @@
> +template<typename T, typename U>
> +inline bool
> +IsInRange(T* aPtr, U* aBegin, U* aEnd)
> +{
> +  return aBegin <= aPtr && aPtr <= aEnd;

...so |aPtr < aEnd| here.

Two template parameters here seems like we should be asserting which direction we want the subtyping relationship between T and U to flow.  I can't quite put my finger on it, but it seems like one direction is OK, while the other direction is a subtle footgun.

@@ +92,5 @@
> +template<typename T>
> +inline bool
> +IsInRange(T* aPtr, uintptr_t aBegin, uintptr_t aEnd)
> +{
> +  return aBegin <= uintptr_t(aPtr) && uintptr_t(aPtr) <= aEnd;

...and |uintptr_t(aPtr) < aEnd| here.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: v2, with type relationship considerations, alignment assertions

Hmm, yeah.  For IsInRange(T*, U*, U*), we want to be sure T* is aligned as a U*, we want that T* to convert to U*.

Comment 7

[Jeff Walden [:Waldo]]

...and we might as well assert various other alignment requirements on other things.  Oh, and dealing with void* (which has no alignment characteristics to assert in all this makes for funtimes.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: v3, with better docs

Also don't allow IsInRange(T*, void*, void*) because that's an obvious type hole.  No idea what I was thinking allowing it before!

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8445365 [details] [diff] [review]
v3, with better docs

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

Such tests.  Very completeness.

r=me with the changes below; I leave it to your judgement whether to templatize the test functions.

::: mfbt/ArrayUtils.h
@@ +110,5 @@
> +
> +/**
> + * Determines whether |aPtr| points at an object in the range [aBegin, aEnd).
> + *
> + * |aPtr| must be consistently aligned with |aBegin| and |aEnd|.  This usually

I think this would be better written as "|aPtr| must have the same alignment as |aBegin| and |aEnd|."

::: mfbt/tests/TestArrayUtils.cpp
@@ +116,5 @@
> +
> +struct EmptyBase {};
> +
> +static void
> +TestIsInRangeEmptyClass()

Seems like TestIsInRange{NonClass,Class,EmptyClass} could profitably be combined in a template function.

@@ +259,5 @@
> +
> +struct ExtraDerivedEmpty : EmptyBase { int y; };
> +
> +static void
> +TestIsInRangeClassExtraDerivedEmpty()

...and the same for all these derived class tests.

@@ +301,5 @@
> +  TestIsInRangeEmptyClass();
> +  TestIsInRangeClassDerived();
> +  TestIsInRangeClassDerivedEmpty();
> +  TestIsInRangeClassExtraDerived();
> +  TestIsInRangeClassExtraDerivedEmpty();

|return 0;| here so as to satisfy the signature of |main()|.  (I see that our current tests are inconsistent about this.)

Comment 10

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd5d26b8d29
https://hg.mozilla.org/integration/mozilla-inbound/rev/97daabb8edbb

While those functions could probably be combined with templates, I'm somewhat leery of doing so just in case the extra complexity of templates messes up the compiler.  We've eschewed templates in tests in a few other places in mfbt/tests, that I could summon to mind if I researched a little.  Probably not worth it.

It's perfectly legal in C++ to fall off the end of int main(), only -- in which case return 0 happens.  I honestly don't remember if I added it to this patch, or just left it as-is.  Doesn't make a difference either way really.

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5dd5d26b8d29
https://hg.mozilla.org/mozilla-central/rev/97daabb8edbb