Skip to content

Simplify locking constructs #6359

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

Open
themaks opened this issue Jan 24, 2025 · 2 comments
Open

Simplify locking constructs #6359

themaks opened this issue Jan 24, 2025 · 2 comments
Labels
Component: Core Issue needs changes to the core Core: HLIL Issue involves High Level IL Core: LLIL Issue involves Low Level IL Core: MLIL Issue involves Medium Level IL Effort: Medium Issue should take < 1 month Impact: Medium Issue is impactful with a bad, or no, workaround

Comments

@themaks
Copy link
Contributor

themaks commented Jan 24, 2025

What is the feature you'd like to have?
The idea would be to add a (non-default) option in the settings to change the way some Load/Store operations with synchronization/lock mechanisms are lifted, to voluntarily reduce the correctness of the lifting for the sake of readability.

For instance, on arm64, the LDX{RB,RH,R,P} instruction family (and its STX{RB,RH,R,P} counterpart) is used to make sure a sequence of LOAD and STORE is atomic, i.e. the data at the targeted address was not modified between operations. For the moment, these operations are lifted as intrinsics (see here, but the lifted code is quite cumbersome to read (and often located in a small while loops).

We could implement an optional path in il.cpp that lifts these instructions as simple load and store operations (the latter always setting the status register to 0, indicating no synchronization problem was encountered), that could ultimately change the decompiled code of an atomic increment (for example) from:

do {
    x0 = __ldxr(addr);
    x0 += 1;
} while (__stxr(x0, addr));

to:

*(addr).q += 1

Ideally, the ILs would also display a comment indicating the presence of a locking/synchronization mechanism in the original assembly; but else the user would just have to check the disassembly view themselves to make sure.

There are options already to change the way some instructions are lifted (e.g. "AARCH64 Prefer Intrinsics for Vector Operations"), so I guess this would not be technically difficult to implement.

Moreover, I think AARCH64 is not the only architecture to implement this kind of operations, the concept could be generalized to more instructions.

Thanks in advance for your consideration,

Have a great day

@themaks
Copy link
Contributor Author

themaks commented Jan 24, 2025

Here is an example of the type of code pattern I am mentioning:
#6062 (comment)

@xusheng6 xusheng6 added Component: Architecture Issue needs changes to an architecture plugin Arch: ARM64 Issues with the AArch64 architecture plugin Effort: Medium Issue should take < 1 month Impact: Medium Issue is impactful with a bad, or no, workaround labels Feb 4, 2025
@plafosse
Copy link
Member

plafosse commented Feb 4, 2025

I think ultimately what we want here is our outlining system to be a bit more flexible so it can emit. Additionally we need direct support for locking in our il for this to happen.

locked_increment(&value)

@plafosse plafosse changed the title Feature request: Option to ignore synchronization/locking mechanism during lifting Simplify locking constructs Feb 4, 2025
@plafosse plafosse added Component: Core Issue needs changes to the core Core: LLIL Issue involves Low Level IL Core: MLIL Issue involves Medium Level IL Core: HLIL Issue involves High Level IL and removed Component: Architecture Issue needs changes to an architecture plugin Arch: ARM64 Issues with the AArch64 architecture plugin labels Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Issue needs changes to the core Core: HLIL Issue involves High Level IL Core: LLIL Issue involves Low Level IL Core: MLIL Issue involves Medium Level IL Effort: Medium Issue should take < 1 month Impact: Medium Issue is impactful with a bad, or no, workaround
Projects
None yet
Development

No branches or pull requests

3 participants