Closing this in favor of github PR: https://github.com/llvm/llvm-project/pull/65534

• rebase to trigger build

• wip on making the build green

• + clang-format
• cleanup
• + clang-format, fix libcxx.imp
• wip on making the build green
• wip on making the build green
• regenerate ignore_format.txt
• bits of formatting
• wip on making the build green

• regenerate ignore_format.txt

• wip on making the build green

• wip on making the build green

Inspired by what libstdc++ does: we don't have to throw + catch (thus unwinding) an exception when creating exception_ptr,
because libcxxabi knows exactly how to create an exception_ptr from the given object.

I can confirm that this patch (with parentheses added around comparison in ReadContextStack)
applied atop of clang-15.0.7 fixes https://github.com/userver-framework/userver/issues/170

Hi @vitalybuka! Thank you for getting back to this.

Hi @vitalybuka ! Pinging you about this

Hi @vitalybuka! Did you have a chance to look into makecontext issue?

In D137654#4101055, @vitalybuka wrote:

Actually, what's the state of this patch? It feels to me that there are 3 competing possibilities of what to do with the swapcontext issue:

1. Land this, see how it goes and what/where it breaks, if any
2. Completely abandon all cleanup machinery and make it users responsibility, as @vitalybuka suggested
3. Revert https://reviews.llvm.org/D129219 together with https://reviews.llvm.org/D130218

So I would prefer no.1, likely some cases would need some user side workaround like no.2
Do you see issues with the current state of the patch? If not, I can land it.
Would you prefer if I submit the patch into your name, as it was started by you? If you don't want to accept the blame for this version, I can land with my name.

Hi!
Is there anything i can do to help resolving the issue (https://github.com/llvm/llvm-project/issues/58633), be that this patch or reverting some other patches?
Right now i'm a bit confused as what to do with this and how to advance any further.

In D137654#4032187, @MaskRay wrote:

In D137654#4031095, @itrofimow wrote:

In D137654#4030513, @MaskRay wrote:

since it breaks ucp cleanup after swapcontext returns in some cases.

Can you elaborate a bit what is "ucp cleanup" and how it breaks?

Sure.
Say we have two contexts, A and B, and we swapcontext from A to B, do some work on Bs stack and then swapcontext back from B to A.
At this point shadow memory of Bs stack is in arbitrary state, but since we can't know whether B will ever swapcontext-ed to again we clean up it's shadow memory,
because otherwise it remains poisoned and blows in completely unrelated places when heap-allocated memory of Bs context gets reused later (see https://github.com/llvm/llvm-project/issues/58633 for example).
swapcontext prototype is swapcontext(ucontext* oucp, ucontext* ucp), so in this example A is oucp and B is ucp, and i refer to the process of cleaning up Bs shadow memory as ucp cleanup.

About how it breaks:
Take the same example with A and B: when we swapcontext back from B to A the oucp parameter of swapcontext is actually B, and current trunk resets its stack in a way that it becomes "uncleanupable" later.
It works fine if we do A->B->A, but if we do A->B->A->B->A no cleanup is performed for Bs stack after after B "returns" to A second time.
That's exactly what happens in the test i provided, and it's actually a pretty common real world scenario.

Hope this clarifies things at least a bit.

Thanks! You may fold the description into the commit message/Differential summary and perhaps add a brief comment (including A->B->A->B->A) to the test function Poll.

In D137654#4030513, @MaskRay wrote:

since it breaks ucp cleanup after swapcontext returns in some cases.

Can you elaborate a bit what is "ucp cleanup" and how it breaks?

@vitalybuka gently pinging you on this. Can you see this ever landing or am i going the wrong direction with the idea of using ss_flags?

• more exhaustive check for flags

In D137654#3925990, @vitalybuka wrote:

Using flags is to easy to find example when it fails.

Hi @vitalybuka!
Did you have a chance to make yourself familiar with my reasoning?

• cr fixes

Hi! Could you please have a look at this?

In D129219#3667281, @vitalybuka wrote:

rebase

In D129219#3633478, @vitalybuka wrote:

In D129219#3633460, @itrofimow wrote:

In D129219#3633411, @eugenis wrote:

I guess some sanity check would be good, but since we get are not guessing this value but getting it directly from the OS, it's fine either way?

In boost::Coroutine2 stack size might be user-provided and i am not sure if there is a limitation at all.

In D129219#3633402, @vitalybuka wrote:

Could you please add a test?

Sure, but could you please suggest an appropriate place for that and where to look for references? - i'm not familiar with the codebase

Take a look at:
compiler-rt/test/asan/TestCases/Linux/swapcontext_test.cpp
compiler-rt/test/asan/TestCases/Linux/swapcontext_annotation.cpp

You may consider to modify them, or just fork and throw away unneeded stuff.
I expect new test is just simplest piece of code which use this function and fails without patch and passes with the patch.

+ clang-format

now checking the test with a patch

making sure this test breaks on trunk

In D129219#3633411, @eugenis wrote:

I guess some sanity check would be good, but since we get are not guessing this value but getting it directly from the OS, it's fine either way?

Based on https://github.com/google/sanitizers/issues/1494

rebase on main

cr fixes

In D128021#3627048, @Mordante wrote:

In D128021#3627047, @itrofimow wrote:

binary size after changes: https://pastebin.com/raw/z23mLN93

 text	   data	    bss	    dec	    hex	filename
10323	    744	      8	  11075	   2b43	binary_size_main
10996	    744	      8	  11748	   2de4	binary_size_patch

Thanks! I like these numbers better! This about half of the size overhead of the previous version.
I assume the main values have changed due to rebasing, right?

binary size after changes: https://pastebin.com/raw/z23mLN93

Trying to force a rebuild

review fixes, updated benchmarks (https://pastebin.com/raw/AF0fa9VJ)

In D128021#3613221, @Mordante wrote:

I had a look at the code and I wonder whether a different approach might be better. Instead of adding a template argument we change __rehash to two function __rehash_unique and __rehash_multi. This seems the approach already taken for other functions in the __hash_table class.

This means the public functions rehash and resize also need two versions. The other functions calling rehash are already specific for unique and multi. I think we can keep the unique key argument on these rehash functions. So I would suggest something along the lines of.

// These were rehash, keep them public and adjust the callers
_LIBCPP_INLINE_VISIBILITY void __rehash_unique(size_type __n) { __rehash<true>(__n); }
_LIBCPP_INLINE_VISIBILITY void __rehash_multi(size_type __n) { __rehash<false>(__n); }

// These were reserve, keep them public and adjust the callers
_LIBCPP_INLINE_VISIBILITY void __reserve_unique(size_type __n)
      {__rehash_unique(static_cast<size_type>(ceil(__n / max_load_factor())));}                                              
_LIBCPP_INLINE_VISIBILITY void __reserve_multi(size_type __n)
      {__rehash_multi(static_cast<size_type>(ceil(__n / max_load_factor())));}


// rename __rehash to __do_rehash and make it templated
template<class _UniqueKeys>
void __do_rehash(size_type __n);

// rename rehash to __rehash, make it private and make it templated
template<class _UniqueKeys>
void __rehash(size_type __n);

Then we can keep the templated rehashing you added but only have the template arguments on these two function instead of the entire class.

WDYT?

Gently pinging this

In D128021#3603897, @Mordante wrote:

In D128021#3603708, @itrofimow wrote:

In D128021#3593922, @Mordante wrote:

Would this example be enough?
I could've messed up compile flags, and guidance would be appreciated if i did so

Thanks! Can you provide the output of the size command instead of the ls command for both binaries?

In D128021#3593922, @Mordante wrote:

One request, when you address a review comment, can you check the "done" button this makes reviewing easier.

I had a look at the changes and I'm wondering about the impact on the size of the binary. You add one template argument to "only" adjust the __rehash function. This means the __hash_table for unordered_map<K, V> and unordered_multimap<K, V> will be different after this change. Could you provide some numbers regarding how much the binary will grow when an applications uses both unordered_map<K, V> and unordered_multimap<K, V>`?

So i got some benchmarks: unordered_set_benches

code review fixes

Thank you guys for the valuable feedback. I'll get back to this near the end of the upcoming week

Can some please help me with clang-format builds failing?
This is what i used to update the patch arc diff HEAD~ --update D128021 --nolint, but format build are still failing.
What should i do to fix that?

cleanup, + benchmarks

In D128021#3591054, @jloser wrote:

I recommend against clang-format everywhere here. When uploading the patch, you can specify --nolint to arc so it won't complain about the lack of clang-formatting a particular changeset. The noise ratio compared with the actual content change here is high in my opinion.

+ clang-format