Vector35 / Vector35/binaryninja-api
two issues with new register calling convention
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 1.3k
- Forks
- 298
- Avg merge
- 5d 5h
- Merged PRs (30d)
- 19
Description
The x86 register calling convention added in the recent CC refactor needs two core fixes to be laid out
correctly: callee stack cleanup, and the hidden pointer for an indirectly returned result going in the argument slot following the declared parameters (EAX with no parameters, EDX after one, the stack once EAX/EDX/ECX
are taken). Without them, every Delphi prototype with a stack argument or a string/record/variant result has its arguments shifted.
Found this while working on a delphi plugin, it certainly produces better results but haven't validated it fully by hand:
diff --git a/arch/x86/arch_x86.cpp b/arch/x86/arch_x86.cpp
index 35d58a008..966f53c23 100644
--- a/arch/x86/arch_x86.cpp
+++ b/arch/x86/arch_x86.cpp
@@ -4913,11 +4913,56 @@ public:
return type && !type->IsFloat() && type->GetWidth() > 4;
}
+ bool IsStackAdjustedOnReturn() override
+ {
+ return true;
+ }
+
bool AreStackArgumentsPushedLeftToRight() override
{
return true;
}
+ /*! Delphi passes the hidden pointer for an indirectly returned result in the argument slot
+ *following* the declared parameters: EAX for a parameterless function, EDX after one
+ parameter, and the stack once EAX, EDX and ECX are taken. GetIndirectReturnValueLocation
+ cannot express that, because the slot depends on how many parameters precede it, so the
+ whole layout is computed here instead.
+ */
+ CallLayout GetCallLayout(BinaryView* view, const ReturnValue& returnValue, const vector<FunctionParameter>& params,
+ const std::optional<set<uint32_t>>& permittedRegs) override
+ {
+ CallLayout result = GetDefaultCallLayout(view, returnValue, params, permittedRegs);
+ if (!result.returnValue.has_value() || !result.returnValue->indirect)
+ return result;
+
+ // A caller that has pinned the return value to an explicit location keeps it.
+ // MarkNonDefaultParameterLocations clears defaultLocation and re-runs this until the
+ // layout agrees with the location it recorded, so moving a pinned location never
+ // terminates.
+ if (!returnValue.defaultLocation)
+ return result;
+
+ // Lay the call out a second time with the hidden pointer appended as an ordinary trailing
+ // parameter and nothing returned, then split that parameter back off as the return value.
+ vector<FunctionParameter> extended = params;
+ extended.push_back(FunctionParameter("", Type::PointerType(GetArchitecture(), Type::VoidType())));
+ CallLayout appended = GetDefaultCallLayout(view, ReturnValue(Type::VoidType()), extended, permittedRegs);
+ if (appended.parameters.size() != extended.size())
+ return result;
+
+ ValueLocation hidden = appended.parameters.back();
+ hidden.indirect = true;
+ hidden.returnedPointer = result.returnValue->returnedPointer;
+ appended.parameters.pop_back();
+
+ result.parameters = appended.parameters;
+ result.returnValue = hidden;
+ result.stackAdjustment = appended.stackAdjustment;
+ result.registerStackAdjustments = appended.registerStackAdjustments;
+ return result;
+ }
+
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in arch/x86/arch_x86.cpp at the x86 register calling-convention implementation, especially IsStackAdjustedOnReturn and GetCallLayout, and compare the proposed layout with GetDefaultCallLayout. Verify callee cleanup and that an indirect result pointer occupies the slot after declared parameters: EAX with none, EDX after one, and the stack after EAX, EDX, and ECX are used.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers, reverse-engineering
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 64/100