Closed Bug 1309552 Opened 3 years ago Closed Last year

Specify buffer size when freeing data in AllocPolicy

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox52 --- wontfix
firefox63 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
AllocPolicy has a free() method, which specifies the base pointer being freed but not its size.  This patch changes 'free_(void*)' to 'template <typename T> free_(T*, size_t numElems)' in all AllocPolicy classes and their users.

This is needed for Web Replay (bug 1207696), which (now) adds an alloc policy that needs to know the size of the buffer being freed (it ends up calling the system unmap routine, which on some platforms requires a size parameter).  It would also be nice for AllocPolicy classes which want to keep an accurate count of how much memory they have allocated but not deallocated, however.
Attachment #8800221 - Flags: review?(jwalden+bmo)
Assignee: nobody → bhackett1024
Comment on attachment 8800221 [details] [diff] [review]
patch

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

I'd like to review the comment change, but otherwise this seems plausible.

::: mfbt/AllocPolicy.h
@@ +113,5 @@
>      return maybe_pod_realloc<T>(aPtr, aOldSize, aNewSize);
>    }
>  
> +  template <typename T>
> +  void free_(T* aPtr, size_t aNumElems)

You haven't updated the documentation comment above this class that explains the general AllocPolicy concept.  Please do so.

The docs should clarify exactly what |aNumElems| is.  Presumably it's the originally passed-in number of elements, for most allocations, but you should also explain that this value *won't* be passed in if the pointer is successfully passed to pod_realloc, in which case the new number of elements will be passed to free_.

Also -- can |aNumElems| ever *not* be one of these values?  I can imagine container classes that might want to depend on the AllocPolicy concept, that might not know this exact number -- they might only know the exact number is "not greater than" |aNumElems|.  (Similar to how malloc implementations may only know the size class of an allocation, i.e. malloc_usable_size, but not the exact number the original caller passed to malloc.)  Please make sure this is documented as well.

::: mfbt/Vector.h
@@ +143,5 @@
>      for (; src < aV.endNoCheck(); ++dst, ++src) {
>        new_(dst, Move(*src));
>      }
>      VectorImpl::destroy(aV.beginNoCheck(), aV.endNoCheck());
> +    aV.template free_<T>(aV.mBegin, aV.mCapacity);

I believe you're required to have |template| here but are *not* required to have |<T>| here, now that function template default arguments are part of C++11 (and are available in all our supported compilers per https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code "function template default arguments").

Please do a try-run with all these redundant |<T>| removed.  If it passes, land the patch that way -- otherwise what you have is fine.
Attachment #8800221 - Flags: review?(jwalden+bmo) → review-
Too late for firefox 52, mass-wontfix.
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INACTIVE
Attached patch patch (obsolete) — Splinter Review
Updated/rebased patch per review comments.
Attachment #8800221 - Attachment is obsolete: true
Attachment #8981232 - Flags: review?(jwalden+bmo)
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Blocks: 1422587
Can you point me at a patch that requires this API change?
Flags: needinfo?(bhackett1024)
(In reply to Jeff Walden [:Waldo] from comment #5)
> Can you point me at a patch that requires this API change?

Bug 1464903 part 1 requires this for the AllocPolicy class it defines.  This alloc policy uses record/replay memory allocation functions that need to know the size of the region being freed.  Those memory allocation functions are defined in part 8 of the same bug, and use the size of the freed region to maintain a set of memory regions available for future allocations.
Flags: needinfo?(bhackett1024)
Comment on attachment 8981232 [details] [diff] [review]
patch

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

::: mfbt/AllocPolicy.h
@@ +42,5 @@
>   *  - template <typename T> T* pod_realloc(T*, size_t, size_t)
>   *      Responsible for OOM reporting when null is returned.  The old allocation
>   *      size is passed in, in addition to the new allocation size requested.
> + *  - template <typename T> void free_(T*, size_t)
> + *      The capacity passed in should match the original allocation.

s/should/must/ and s/original allocation/count of allocated elements/

::: mfbt/Vector.h
@@ +1525,5 @@
>      mLength = aLength;
>      mTail.mCapacity = kInlineCapacity;
>      Impl::moveConstruct(mBegin, aP, aP + aLength);
>      Impl::destroy(aP, aP + aLength);
> +    this->template free_(aP, aLength);

Shouldn't this be using the old value of |mTail.mCapacity|, before it was overwritten three lines up?

::: mozglue/misc/Printf.h
@@ +112,5 @@
>  struct AllocPolicyBasedFreePolicy
>  {
>    void operator()(const void* ptr) {
>      AllocPolicy policy;
> +    policy.free_(const_cast<void*>(ptr), 0);

If you can just pass 0 to some non-specifically-selected AllocPolicy on which further restrictions were clearly imposed, then the numElements passed to |free_| is only advisory.  That doesn't seem to be clearly documented in AllocPolicy.h as permissible, "should" suggesting sometimes you don't have to but not saying when.

I'm inclined to say we should require it be correct (and, of course, document it as such in AllocPolicy.h's description of free_'s semantics).  If as it appears/you suggest this is the only place that can't know the allocation count, we should just do the work to fix it.

Ultimately, tho, our AllocPolicy exposing raw memory operations (rather than letting you create Ts with invoked constructors on them) is probably a bad look, that doesn't fit C++ as the language and its object model intend/require.  Some other time/place than here, I guess.
Attachment #8981232 - Flags: review?(jwalden+bmo) → review-
froydnj and I agreed last week on having this be freeElements instead of free_, but in light of the entire API exposing |aNumElements| controls, the only things ever allocated *are* elements.  So a rename to disambiguate seems somewhat redundant.  Just to record my change-of-position on the matter since that discussion...
(In reply to Jeff Walden [:Waldo] from comment #7)
> ::: mfbt/Vector.h
> @@ +1525,5 @@
> >      mLength = aLength;
> >      mTail.mCapacity = kInlineCapacity;
> >      Impl::moveConstruct(mBegin, aP, aP + aLength);
> >      Impl::destroy(aP, aP + aLength);
> > +    this->template free_(aP, aLength);
> 
> Shouldn't this be using the old value of |mTail.mCapacity|, before it was
> overwritten three lines up?

This call is freeing the buffer passed in by the caller, so should use the value the caller passed.  This is actually supposed to be using aCapacity, though (when I wrote the patch there was only an aLength parameter; I looked through the patch for other rebasing problems but didn't see any).
Attached patch patchSplinter Review
I tried fixing the sprintf stuff to pass in the capacity when freeing its data, but ran into problems because sometimes that data is passed in from outside as a UniquePtr and quite a few callers would need to be fixed, e.g. the 30-odd callers of JS_sprintf_append.  Sprintf also violates the alloc policy requirements in its use of realloc, which I fixed here.  It seems simpler to make the capacity an optional argument to free_, with alloc policies allowed to require the capacity argument, so that we'd get compile errors instead of incorrect behavior if an incompatible alloc policy is used with the sprintf code.
Attachment #8981232 - Attachment is obsolete: true
Attachment #8987653 - Flags: review?(jwalden+bmo)
Comment on attachment 8987653 [details] [diff] [review]
patch

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

Bah.  The std::unique_ptr interface should have been reconciled with the standard allocation policy trait when they standardized it.  :-(  I guess okay.
Attachment #8987653 - Flags: review?(jwalden+bmo) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c8c2d8a6003
Specify buffer size when freeing data in AllocPolicy, r=waldo.
https://hg.mozilla.org/mozilla-central/rev/5c8c2d8a6003
Status: REOPENED → RESOLVED
Closed: 2 years agoLast year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.