LGTM with comment

In D88457#2301551, @pcc wrote:

I'm not really a fan of long lists of template arguments. So I would favor something like

3. Have the SizeClassAllocator classes take something like a Config type (see allocator_config.h) as a template argument and have it read the configuration out of that.

Actually fix all the instances of this issue :/

Thanks Peter!

In D88170#2290714, @mcgrathr wrote:

LGTM modulo clang-format. Thanks for the quick fixes!

In D86694#2287242, @russell.gallop wrote:

I'm considering a slight change of plan. @cryptoad, in the name of incremental development, would you be happy for me to put the scudo sanitizer version up (with working tests), or is this deprecated now? It may not be the best for performance long term but it's a smaller step than the standalone port and seems to work.

In D86694#2277371, @aganea wrote:

@cryptoad What happens if the primary was much smaller? Or if pages were reserved in much smaller ranges?

In D86694#2274548, @russell.gallop wrote:

I guess using scudo as a general purpose allocator that could set a limit on the number of cores that can be used at once as @aganea found. Would there be any problem with making this very small (e.g. a couple of GB)?

In D86694#2273815, @aganea wrote:

If 4.4 TB of virtual pages are mapped in each process (this happens on startup), then we quickly exaust the 48-bit (256 TB) addressable space with 72+ programs running (on a 36-core). Any idea where this 4.4 TB mapping comes from?

In D86694#2268182, @russell.gallop wrote:

I think that I one point in time I tried thread_local but a compiler (can't remember which one) was complaining about the constructors not being trivial or something to that extent.
This is fairly blurry at this point, so I am sorry if this is not very helpful, but I just want to make sure this would work on all platforms with gcc/clang.

Initialize a variable that the Fuchsia gcc build complains about.

I was wrong about the base paramater to use with releasePagesToOS on Fuchsia,
it's the base of the committed mapping, which is not the base of the reserved
mapping. So change that back to what it was.

FYI I got to run the tests and some things are failing, I am looking into what could be wrong.

In D86694#2242150, @russell.gallop wrote:

In D86694#2242140, @cryptoad wrote:

That's awesome! Is it meant to eventually be committed or only be used for comparison purposes?

I'd like it to be committed, but can't claim I know the code from https://reviews.llvm.org/D42519 well enough. The good news is that I can build LLVM on Windows with this. Is there a good sanity check that it is actually using Scudo rather than silently using the standard alloc?

That's awesome! Is it meant to eventually be committed or only be used for comparison purposes?

In D86399#2232864, @vitalybuka wrote:

Interesting that GCC does not inline capture by ref https://godbolt.org/z/exfP69

In D86399#2231990, @cryptoad wrote:

I am unclear if there is a way for the lambda to be inlined in the callee.
If anyone has a suggestion, I'll gladly take it.

I am unclear if there is a way for the lambda to be inlined in the callee.
If anyone has a suggestion, I'll gladly take it.

In D84667#2176494, @cferris wrote:

Is there any major changes from the other mallopt change? In other words, should I rerun performance data?

Addressing Chrisopher's comments:

• fixed a couple of comments
• reworked setOption to avoid short circuiting

I created a new one @ https://reviews.llvm.org/D84667.
The content is the same but I had some issues rebasing/merging while keeping the same Phabricator revision, so I ended up redoing a new one.

In D82397#2171020, @pcc wrote:

Where does the configuration need to live? If it just needs to be a build-time configuration, that may be simpler than exposing new mallopt options.

Thanks!

Corrected a bunch of things as apparently getting right into things
post vacation yielded a bunch of errors.

With proper merge of the primary*.h.

With proper merge of the primary*.h.

I didn't merge properly, updating the CL.

In D82031#2112360, @cferris wrote:

Overall, this seems to be reasonable, but I am a bit worried about the odd 32 bit performance. Is there an explanation for what could be causing the 32 bit calls to increase?

Fix the PackedCounter static buffer check.
We were using the count of elements instead of the size of the buffer :(
Also make the count public so that release.cpp can use it.

I did the commit and followed the instructions @ https://llvm.org/docs/Phabricator.html#committing-someone-s-change-from-phabricator.
Let me know if they are no longer relevant.

Only do the BytesPushed check is not doing a forced release.

One of the things with this behavior is that it could have security repercussions, in the sense that we can lose the property of the separation per class size.
eg: by filling in all the previous class sizes, one could end up in another, allowing to properly exploit a UAF or overflow.
This is probably more theoretical than practical, but something to keep in mind for the future.

In D62368#2030005, @craig.topper wrote:

I've deleted the branch in the github web interface. I didn't know I could do that.

In D62368#2029752, @craig.topper wrote:

I noticed when I pulled this morning that this seems to have been pushed as a branch to the repo

From https://github.com/llvm/llvm-project

eb7d32e..63a4fdd  master     -> origin/master

• [new branch] arcpatch-D62368 -> origin/arcpatch-D62368

In D62368#2027895, @fanjinke wrote:

Hi Cryptoad，

Could you help me to commit the patch? Because I don't have access.

Thanks.