Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decompiler / Function: mark function parameter as "unused", reflect that in decompiler #5743

Open
slippycheeze opened this issue Sep 5, 2023 · 1 comment
Assignees
Labels
Feature: Decompiler Status: Triage Information is being gathered Type: Enhancement New feature or request

Comments

@slippycheeze
Copy link

Is your feature request related to a problem? Please describe.
Functions sometimes have unused arguments; if the x86_64 optimizer is aware of this it can leave the register or stack slot "uninitialized", effectively passing a random value based on prior code, to that call.

Minimally, this leads to somewhat confusing decompilation where, eg, what is clearly a std::string is cast to some other object type entirely (matching the unused parameter.)

Earlier, when types were less clear, this back-propagated the type information from the function argument and mis-typed a whole pile of stuff because of it.

While the arguments can happen naturally, I run into this with MSVC-compiled C++ code with inlining, optimization, and templates can generate concrete function instantiations that:

  1. internally, "bind" one function argument to some global value or constant, and
  2. ignore the passed in value completely, and
  3. in the caller, are not inlined, presumably due to some constraint on size and/or number of inlined fns
  4. in the caller, ARE aware the function argument is unused, and do NOT clear or set the register / stack slot before calling.

This happens in "constructor with bulk construction of children" most often; the current example DisplayWidgetConsole, 9148 bytes of code, and creates 51 instantiations of ConsoleCommand<...> representing the various commands that can be run. It stops inlining the template constructor instatiation at the 14th ConsoleCommand<>, 3017 (0xBC9) bytes into the function body.

In other places, NULL is passed in that slot, despite the compiler having access to the appropriate variable to fill it, because the optimizer "knows" it is unused in the function body.

(in the concrete example below, the this argument to a std::string constructor was still in RCX during the next call, so it seemed like the same thing was passed to the function twice, as two different types.)

Describe the solution you'd like
In writing code, developers often adopt conventions like void foo(void* /*unused*/, uint bar) to flag this to other humans. This helps avoid any confusion.

For the decompiler, especially in the face of the optimizer generating these, it'd be great to be able to say "use of this function parameter is irrelevant, do not consider it for any purpose in understanding the code."

Ideally, it'd end up looking like this in the decompiled code:

// CURRENT
p_command = GAME::ConsoleCommand::ConsoleCommand
    (p_void,(PhysicsEngine2 *)help_cstr,(mem_fn<void,char*> *)&p_command_impl,&help_str);
// DESIRED
p_command = GAME::ConsoleCommand::ConsoleCommand
    (p_void,/* unused */,(mem_fn<void,char*> *)&p_command_impl,&help_str);

Describe alternatives you've considered
I was naming the parameter __unused or __ignored in the function prototype for that concrete instantiation.

This made the body of the function easier to understand, because I could see that I already did the work to make sure it was genuinely unused, not just "bad prototype of a called function makes it look unused."

OTOH, the decompiler back-propagated that name to local variables, and __ignored was often reused for meaningful values in other instantiations of the same template function.

Additional context
This, along with #2573, are the two main "features of code" I associate with a function that will never stop spitting out the decompiler warning that type propagation never settled, or that it exceeded the maximum restart count.

Unused arguments are less prone to triggering that, but they seem to push functions toward it. I presume this is because the "unused" argument is treated as a valid "cast", and a stack slot suddenly changes from std::string to PhysicsEngine2, a conflict that can't be resolved without ignoring the "unused" parameter.

@slippycheeze
Copy link
Author

note: I can provide concrete code / "decompiler debug XML" for these, if you want. let me know what chunks of whatevs are desirable.

@ryanmkurtz ryanmkurtz added Type: Enhancement New feature or request Feature: Decompiler Status: Triage Information is being gathered labels Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Decompiler Status: Triage Information is being gathered Type: Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants