sc2ad / beatsaber-hook Goto Github PK

Delegates are currently made in a fairly hacky way, which makes it such that when a bs-hook created Delegate is added to a multicast delegate and then attempted to be removed, it results in UB and ultimately will crash due to how the MethodInfo that is attached to the Delegate is garbage.

A solution to this problem would involve reworking delegate creation. Specifically, more properly creating the MethodInfo and attaching it, as well as properly supporting references to the MethodInfo and even cleanup of it after its usage is no longer assured. The "simplest" way this could be done would be to GC allocate (or otherwise be aware of) this MethodInfo which would then be cleaned up on the Delegate's destruction. This might work best if moved to custom types, ideally in a fashion that is templated, which could be interesting.

Point is, this has several solutions and the current implementation is just bad. The first thing that should be tried should be to fix the MethodInfo being provided.

Hook installs should have the methodinfo* parameter. Ideally we have a scenario such that for hooks that do not have a methodinfo*, (match or find hooks specifically) we can implicitly add one, and for hooks that have them as the final parameter, do not change the signature at all.

Two template specializations on final match for hooking constructs. Each will perform their work under the hood.

Possibly out of scope but relevant: force swap everything to use wrapper hooks?

sc2bad

So because we just introduced the concept of wrapper hook types, it has become increasingly obvious that we can actually abstract this out further. We should consider making/moving hook installations to Flamingo proper, such that we can perform more of these arbitrary wrappers in ONE place, instead of in a mod-per-mod location.

It should also be MORE capable of handling installing hooks to the same location, and how to order them.

Also, it should be the case that we completely avoid hook related installation and usage crashes

Unity Version: 2021.3.16f1 (4016570cf34f)

In this version 2021.3.16f1, struct Il2CppDefaults dosen't have mono_method_class property, so I can't get mono_method_class in file typedefs.h here is the relevant code:

DEFINE_IL2CPP_DEFAULT_TYPE(Il2CppReflectionMethod*, mono_method);

In il2cpp_utils::MakeGenericMethod() method, it calls the MakeGenericMethod method in Il2CppClass mono_method_calss:

auto res = il2cpp_utils::RunMethod<Il2CppReflectionMethod*, false>(infoObj, "MakeGenericMethod", arr);

So, how should I make a method from generic method in this version?

// C#

// Invoke the method get from generic method
System.Reflection.MethodInfo mi = typeof(Component).GetMethod("GetComponent", System.Type.EmptyTypes);
System.Reflection.MethodInfo miConstructed = mi.MakeGenericMethod(typeof(Rigidbody));
Rigidbody rigidbody = (Rigidbody)miConstructed.Invoke(this, null);

// Invoke the method normally.
Rigidbody rigidbody = GetComponent<Rigidbody>();

https://discord.com/channels/441805394323439646/583108561396170837/959534636277575720

What do I mean? Well, if we have an RunMethodRethrow call, and the exception thrown in the C# method (which then bubbles to a C++ method) is uncaught, the exception will travel all the way to terminate, but with a completely unwound stack.

This makes it virtually impossible to tell where the damn exception was actually thrown, thus making this form of throwing horrible from a debugging perspective-- all uncaught exceptions will bubble to the caller's handler, which could be libcodegen, libil2cpp, or even libmod (perhaps even ANOTHER mod, depending on who called who). Point is, it's horrible and tells us LESS than if we just caused a forced crash.

So, how do we fix this? I see a few high level fixes (for common cases, we will do some more analysis of some stuff in a moment)

1. Don't throw anymore, lets just crash again (maybe more optionally?)
2. Throw exceptions that have stacktrace info, and later log it/keep track of it (maybe on exception dtor or something) This is actually pretty hard because we need to somehow handle cases where uncaught exceptions bubble across domains (ex: exception thrown bubbles to libil2cpp as opposed to bubbling to libcodegen)
3. Throw exceptions that are just instantly caught and handle them somehow

All of these have some pretty glaring issues--

1. The first approach sounds great for most cases (especially having crashing be optional, perhaps allowing for UB), but when you realize that you want to be able to catch C# exceptions, things become much trickier-- ALL WHILE still allowing them to be caught in an exceptional case (completely uncaught)
2. This sounds like it solves some issues, we can keep track of exceptions with stack trace info and maybe even display that info! Sounds great on paper, and realistically would work for MOST cases (when exceptions bubble up to a library we are in charge of), but outside of that things become just as bad. This is actually not the worst possible solution, but grabbing relevant stack trace info is not actually truly trivial-- getting our calling function isn't TOO bad, but functions above that become much more obnoxious, and doing all of this just to simply catch the exception seems like a bit of a waste in the C#-caught case described earlier.
3. This also sounds great, but there are some tricky issues to deal with here as well. Namely, we need to add additional boilerplate for exceptions that bubble up that would be ABOUT to cross a boundary, but then we run into a similar issue of losing stack trace info, so we would need these exceptions to hold SOME information about their stacktrace so that we could at least log that in our overall catches before we would otherwise throw to libil2cpp which would (probably inevitably) crash.

Soooooooo, yeah. Tough problems, tough solutions. This is also relevant with #125 which has some information-- also errs on the side of just logging useful debugging info all the time, which I'd like to avoid if possible.

I feel like there's probably a clever approach here that could take advantage of multiple of these approaches, or perhaps a new one altogether (maybe we implement our own pseudo-throw system? One where if the exception would bubble, we handle it accordingly)

Or we could maybe take advantage of hooking something like __cxa_throw to set our stacktrace somehow, and then use that if we need to except, but that still runs the same problems as before, where we need the stacktrace info.

Ultimately, open to ideas and feedback, hopefully this gives a bit of understanding to those curious and sparks some discussion around interesting thoughts that might be possible.

Now that ConstString has made it into master and has some pretty significant benefits over previous methods of allocating manual strings (constexpr, entirely static memory, allocation-less, known AOT), it is perhaps a foregone conclusion that allowing this functionality for more types would be worthwhile.

The easiest compile-time typedef that I think would get the most value would be a compile-time Array/List implementation. List becomes a bit more tricky, but perhaps hardcoding the capacity to be what is used is what can make adds and removes applicable. (Akin to static_vector) Ultimately, there should be some room for improvement, which, while it may be GC-unaware, can perhaps be made to be aware over a set of different ways, or perhaps with an early gc allocation (since we have access to that fairly early on, perhaps even before il2cpp_init, just that we would have to scan for it)

Currently, exception handling in il2cpp differs from the exception handling we compile with.

This leads us to a conundrum-- we can throw our own exceptions, but if we don't catch them (and we do them in a method that is called by C#) then we will crash without good error logging. If we want to throw an exception for il2cpp to have, we can do so (il2cpp_utils::raise) but then we can't catch it ourselves.

The common error point here is code that's called by C# somewhere.

This could be a hook, a custom type method, etc. But essentially, if we were to add some checks around these C# facing things, we could be much more confident in our ability to handle exceptions through this layer-- namely, we could, for example, wrap all of our hooks and custom type methods with an (opt-out-able) try-catch that catches and raises the exception in il2cpp land, or otherwise aborts right then and there.

Ultimately, this would probably be quite the breaking change, but it certainly seems like one of the (many) safety features we can add to make development easier for those who are solely looking to debug their development. Exceptions SHOULD exist and be used more commonly when building non-release, and should be opt-out-able for those who care a lot about performance.

All in all, this seems doable with the current macro approach, or we could do it in flamingo by wrapping our trampolines in try-catches ourselves (somehow need to distinguish on a hook-by-hook basis though, and also be templated/header-only) but it would be breaking regardless.

Maybe a 4.x goal?

Tracking the issue here because why not

Complete the ListWrapper type. See typedefs-list.hpp and cross compare to typedefs-array.hpp

🦩
🐉

So due to special method names, our current implementation relies on method name lookups which is bad.

An alternative to this would be to create a method, say, RunVirtualMethod which would take in an instance (that must have a vtable) and MethodInfo* that is looked at for its SLOT and declaring type + params/ret, and will forward to RunMethod... of some variant AFTER the correct method is deduced via slot and instance vtable. The way this will have to be done is by class resolution + interface/base type analysis--

• A base type must be the first vtable_count slots of the target instance's vtable
• Interfaces are arranged in the order they are declared in after this
• Target slot should be found by taking initial offset (0 for base types, or non-zero for any interface, must be deduced) and adding our input slot to that result.
• Target slot should be looked up in the target vtable ex: target_vtable[target_slot] and the corresponding method pointer should be extracted, then passed into any of the remaining invokes.

This functionality probably should encompass several methods (forwards to multiple RM calls) and probably also support a "conversion" call without invoke (to resolve a given input slot on a given instance to a method pointer)

Some corners can be cut that can be reasonably inferred, but it will be tricky to cut out some others (ex, vtable distributions can't be encoded in an incredibly sane way AOT, deduction almost certainly has to take place on runtime)

Improve StringW to support more ergonomic usage-- add functionality for different methods and things.

Add xref traces for il2cpp_gc_register_thread and il2cpp_gc_unregister_thread at the very least. This should allow us to make stack locals gc-safe, which hardly matters at all (since nearly all code is run in a managed thread of some kind)

In addition, should add a better way of keeping static fields gc aware.

Making an issue of this since I am unsure how to proceed with it.

it seems that operator std::string consistently returns the contained string to be off by 1, meaning the last character is erased or somehow inaccessible

Only the operator std::string itself seems to be suffering from this, the rest of the operators are fine

I think this could be solved by either removing the -1 here or by not assigning the \0 here

The reason I am unsure on how to proceed is that I do not know if it's the first thing that is broken, or the second thing

So currently, we use ByRef<T> which works fine for ref/out but in the case that it is in passed, we don't need it to be T but rather const T. While this isn't strictly necessary, it could be nice since it's obvious we can or cannot modify T.

Hopefully, there can be an approach that also does not affect method resolution forcing you to choose between ConstRef and MutRef (coining these names today)

I tried to install on 1.19 but it did not work!!!! Please fix or else...

Not too much info here, here's a crash log from 1.19: https://pastebin.com/KFFWbe51