Enforce arenas to match on moz_arena_{realloc,free}

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

2 years ago
No description provided.
Assignee

Updated

2 years ago
Assignee: nobody → mh+mozilla
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8926722 [details]
Bug 1402283 - Rename extent_node_t fields.

https://reviewboard.mozilla.org/r/197956/#review203208
Attachment #8926722 - Flags: review?(n.nethercote) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8926723 [details]
Bug 1402283 - Make arena_ralloc use the same arena as the original pointer when none is provided.

https://reviewboard.mozilla.org/r/197958/#review203210
Attachment #8926723 - Flags: review?(n.nethercote) → review+

Comment 12

2 years ago
mozreview-review
Comment on attachment 8926724 [details]
Bug 1402283 - Replace isalloc/isalloc_validate with static methods of a new AllocInfo class.

https://reviewboard.mozilla.org/r/197960/#review203212
Attachment #8926724 - Flags: review?(n.nethercote) → review+

Comment 13

2 years ago
mozreview-review
Comment on attachment 8926725 [details]
Bug 1402283 - Associate an arena to each huge allocation.

https://reviewboard.mozilla.org/r/197962/#review203214

::: memory/build/mozjemalloc.cpp:632
(Diff revision 1)
>    void* mAddr;
>  
>    // Total region size.
>    size_t mSize;
>  
> +  union {

It would be nice to have a comment here explaining when the different parts of the union are valid.

::: memory/build/mozjemalloc.cpp:3343
(Diff revision 1)
> +  arena_t* Arena()
> +  {
> +    if (mSize <= gMaxLargeClass) {
> +      return mChunk->arena;
> +    }
> +    return mNode->mArena;

You could use ?: here.

::: memory/build/mozjemalloc.cpp:3348
(Diff revision 1)
> +    return mNode->mArena;
> +  }
> +
>  private:
>    size_t mSize;
> +  union {

ditto
Attachment #8926725 - Flags: review?(n.nethercote) → review+

Comment 14

2 years ago
mozreview-review
Comment on attachment 8926726 [details]
Bug 1402283 - Enforce arena matching on moz_arena_{realloc,free}.

https://reviewboard.mozilla.org/r/197964/#review203218

::: memory/build/Utils.h:50
(Diff revision 5)
> +constexpr size_t operator"" _MiB(unsigned long long int aNum)
> +{
> +  return size_t(aNum) * 1024_KiB;
> +}
> +
> +constexpr size_t operator"" _MiB(long double aNum)

What's the rounding behaviour here? Might be worth describing in a comment.

::: memory/build/mozjemalloc.cpp:1260
(Diff revision 5)
>  static void*
>  huge_palloc(size_t aSize, size_t aAlignment, bool aZero, arena_t* aArena);
>  static void*
>  huge_ralloc(void* aPtr, size_t aSize, size_t aOldSize, arena_t* aArena);
>  static void
> -huge_dalloc(void* aPtr);
> +huge_dalloc(void* aPtr, arena_t* aArena=nullptr);

Nit: spaces around '='? (Or does clang-format remove them?)

::: memory/gtest/TestJemalloc.cpp:278
(Diff revision 5)
> +  // For convenience, realloc can also be used to reallocate arena pointers.
> +  // The result should be in the same arena. Test various size class transitions.
> +  size_t sizes[] = { 1, 42, 80, 1_KiB, 1.5_KiB, 72_KiB, 129_KiB, 2.5_MiB, 5.1_MiB };
> +  for (size_t from_size : sizes) {
> +    for (size_t to_size : sizes) {
> +      if (from_size != to_size) {

I'd remove this condition and test the same size cases as well.
Attachment #8926726 - Flags: review?(n.nethercote) → review+
Assignee

Comment 15

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #13)
> It would be nice to have a comment here explaining when the different parts
> of the union are valid.

Maybe it's not entirely clear, but "used by chunk recycling code" and "for huge allocations" does describe when the fields are valid.

> ::: memory/build/mozjemalloc.cpp:1260
> (Diff revision 5)
> >  static void*
> >  huge_palloc(size_t aSize, size_t aAlignment, bool aZero, arena_t* aArena);
> >  static void*
> >  huge_ralloc(void* aPtr, size_t aSize, size_t aOldSize, arena_t* aArena);
> >  static void
> > -huge_dalloc(void* aPtr);
> > +huge_dalloc(void* aPtr, arena_t* aArena=nullptr);
> 
> Nit: spaces around '='? (Or does clang-format remove them?)

Err, I didn't run this one against clang-format, and in fact, the default value is not necessary.
Assignee

Comment 16

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #14)
> > +constexpr size_t operator"" _MiB(long double aNum)
> 
> What's the rounding behaviour here? Might be worth describing in a comment.

I'm not sure what could be said in a comment that is not clear from the one line of code that is just below.
Flags: needinfo?(n.nethercote)
> I'm not sure what could be said in a comment that is not clear from the one
> line of code that is just below.

Depends how well you remember C++'s rounding rules :)  I think it truncates in this case?
Flags: needinfo?(n.nethercote)
Assignee

Comment 18

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #17)
> > I'm not sure what could be said in a comment that is not clear from the one
> > line of code that is just below.
> 
> Depends how well you remember C++'s rounding rules :)  I think it truncates
> in this case?

It does. That however doesn't matter much unless you're passing something that you intend to be precise to the byte. On a KiB or a MiB...
Assignee

Comment 19

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #14)
> ::: memory/gtest/TestJemalloc.cpp:278
> (Diff revision 5)
> > +  // For convenience, realloc can also be used to reallocate arena pointers.
> > +  // The result should be in the same arena. Test various size class transitions.
> > +  size_t sizes[] = { 1, 42, 80, 1_KiB, 1.5_KiB, 72_KiB, 129_KiB, 2.5_MiB, 5.1_MiB };
> > +  for (size_t from_size : sizes) {
> > +    for (size_t to_size : sizes) {
> > +      if (from_size != to_size) {
> 
> I'd remove this condition and test the same size cases as well.

This leads to an interesting question: should moz_arena_realloc fail when it doesn't do anything (in-place realloc) but the arena it's given is wrong. I think it probably should. What do you think?
Flags: needinfo?(n.nethercote)
Assignee

Comment 20

2 years ago
It actually makes things simpler.
Flags: needinfo?(n.nethercote)
> This leads to an interesting question: should moz_arena_realloc fail when it
> doesn't do anything (in-place realloc) but the arena it's given is wrong. I
> think it probably should. What do you think?

realloc() is a function that leads to many interesting questions :)

I agree that failing is the right thing. Worth documenting in a comment.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

2 years ago
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2df7f0bf182
Rename extent_node_t fields. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f44c449e280
Make arena_ralloc use the same arena as the original pointer when none is provided. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0e82c6b316
Replace isalloc/isalloc_validate with static methods of a new AllocInfo class. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/a82bf86d86fd
Associate an arena to each huge allocation. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee9d4052e949
Enforce arena matching on moz_arena_{realloc,free}. r=njn

Comment 29

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/50d59c3a291b
Fix non-Nightly build bustage. r+a=RyanVM
No longer depends on: 1416473
You need to log in before you can comment on or make changes to this bug.