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

Aggressive use of inline function calls as parameters makes reading the code more difficult #6160

Open
op2786 opened this issue Nov 20, 2024 · 2 comments
Labels
Core: HLIL Issue involves High Level IL Effort: Medium Issue should take < 1 month Impact: Medium Issue is impactful with a bad, or no, workaround Type: Enhancement Issue is a small enhancement to existing functionality

Comments

@op2786
Copy link
Contributor

op2786 commented Nov 20, 2024

Version and Platform (required):

  • Binary Ninja Version: 4.2.6455-dev, 2c8da1e
  • OS: macos
  • OS Version: 15.1
  • CPU Architecture: arm64

I frequently encounter long lines in decompilation of C++ binaries due to the aggressive inlining of function calls as parameters to other functions. This practice not only makes the code harder to read aesthetically, but it also complicates comprehension, as it becomes difficult to determine what one function returns when it is passed as a parameter to another function.

I believe that, aside from a few edge cases, this practice should generally be avoided.

This is likely not a bug and may be related to issue #3289.

Here is another perfect example:

BN:

TFormatString(TFormatString::arg(TFormatString::arg(TFormatString::arg(TFormatString::TFormatString(&var_108, u"%1.%2%3"), sub_18056e670(arg3), 4, 0xa, 0x30), sub_18056e650(arg3), 2, 0xa, 0x30), sub_18056e5f8(arg3), 2, 0xa, 0x30))

IDA:

v11 = TFormatString::TFormatString((TFormatString *)v69, L"%1.%2%3");
v12 = sub_18056E670(a3);
v13 = TFormatString::arg(v11, v12, 4, 10, 0x30u);
v14 = sub_18056E650(a3);
v15 = TFormatString::arg(v13, v14, 2, 10, 0x30u);
v16 = sub_18056E5F8(a3);
v17 = TFormatString::arg(v15, v16, 2, 10, 0x30u);
TFormatString::operator TString(v17, v61);
@psifertex psifertex added the State: Duplicate Issue is a duplicate of another issue label Nov 20, 2024
@psifertex
Copy link
Member

psifertex commented Nov 20, 2024

I suspect this is just an example instance of #3289 which is already planned for the next release. I'll link to it here but close this one as duplicate since I believe our current plan for that issue will resolve this as well.

@psifertex psifertex closed this as not planned Won't fix, can't repro, duplicate, stale Nov 20, 2024
@plafosse
Copy link
Member

plafosse commented Nov 20, 2024

My thought that you don't want to see this kind of thing even when its well formatted

  bar(
    bas(a, b, c),
    d,
    e)
  f,
  g)```
Imagine d,e,f,g are all long lines
I'd be much better to see.
```x1 = bas(a,b,c)
x2 = bar(x1,
         d,
         e)
x3 = foo(x2,
         f,
         g)```
This is a bit tricky to do though because during il generation you have to be worried about the length of symbols and variable names, which is ... annoying but we should do it anyway.

Additionally users will probably want the ability to control if/when we do intermediate variable elimination.

@plafosse plafosse reopened this Nov 20, 2024
@plafosse plafosse added Core: HLIL Issue involves High Level IL Effort: Medium Issue should take < 1 month Impact: Medium Issue is impactful with a bad, or no, workaround Type: Enhancement Issue is a small enhancement to existing functionality and removed State: Duplicate Issue is a duplicate of another issue labels Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core: HLIL Issue involves High Level IL Effort: Medium Issue should take < 1 month Impact: Medium Issue is impactful with a bad, or no, workaround Type: Enhancement Issue is a small enhancement to existing functionality
Projects
None yet
Development

No branches or pull requests

3 participants