fix: Demangle deeply nested generics by increasing recursion limit

[Swatinem]

This helps with demangling some heavily-nested types.
I do see some types that have generics nested 7 levels deep or more.
Some boost and std types can also lead to very deep nesting.

[Swatinem]

We have been running this patch for ~24h at sentry, and we already see a massive reduction in demangling failures, in the ballpark of ~5-15% of the failures we saw before applying this patch.
We surely need more time to gather more reliable numbers, and I haven’t looked yet at some of the remaining demangling errors, its possible that increasing the recursion limit further will lower our failure rate even further.

Talking with @jan-auer we also noticed that the recursion limit is for the recursive descent parser, and thus an implementation detail that only roughly correlates with the complexity of the mangled identifiers themselves.

[Saldivarcher]

Do you think it'd be best to just add a command line option to change the limit instead?

[jan-auer]

@miguelsaldivar: Swatinem and I discussed this briefly and came to the conclusion that the recursion limit is an implementation detail protecting the demangler. To a user of the library, there is no indication as to what a sensible limit is. In the end, it just depends on whatever symbols you're encountering.

I haven't looked in detail, but probably the demangler could be rewritten to recurse less or behave differently when it hits the limit. If that happens, such a parameter would lose its meaning.

Of course, blindly increasing the limit is not the solution to all problems, so please let us know if you'd prefer us to look into alternative solutions to this.

[khuey]

I think that either:

a) This needs to be user-configurable OR
b) The max recursion value needs to be bumped to the maximum supported by libiberty (i.e. binutils's demangler) which is 1024 (provided that this does not generally exhaust the stack space on a normal thread).

[jan-auer]

The max recursion value needs to be bumped to the maximum supported by libiberty

Well, that we should've checked much earlier. Personally, I'd opt for this, but this is up to you all :)

[fitzgen]

I think that either:

a) This needs to be user-configurable OR
b) The max recursion value needs to be bumped to the maximum supported by libiberty (i.e. binutils's demangler) which is 1024 (provided that this does not generally exhaust the stack space on a normal thread).

I think both would be desirable.

[khuey]

Unfortunately setting the limits to 1024 blows out the stack in the recursion_limit test in debug builds.

[Swatinem]

I bumped both parsing and pretty-printing to 192 now, just to be a bit conservative.
Not sure how much we can, or should increase this to be safe vs stack overflows.

[Swatinem]

Because we have a bit more data now:

The initial increase from 64 to 96 has had the most impact, reducing the number of demangling failures we see from ~500k per day to ~35k. With the further increase to 192, numbers dropped to maybe ~25k. We have quite some fluctuations to its hard to tell.

With (most of) the recursion related failures gone, I was able to also identify missing features (#208), and some demangling failures coming from msvc-demangler.

So how should we proceed here? I can expose this as a user-configurable option, but I’m not sure how useful that actually is, or alternatively increase this further to a sensible number that does not cause any stack overflows.

[khuey]

I bumped the default value to 96. I still think I need to make this user-configurable though.

[khuey]

I pushed eac382e to make it configurable. @Swatinem can you confirm that this works for Sentry?

[Swatinem]

@Swatinem can you confirm that this works for Sentry?

Ah nice this looks very good, I think we can play around with this a bit and find a sweetspot for us.

[khuey]

I'm going to consider this fixed.