Tricore: add a FCALL calling convention #4
Conversation
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| void checkReturn(Program program, Instruction instr) { | ||
| String mnemonic = instr.getMnemonicString().toLowerCase(); | ||
|
|
||
| if (instr == null || !instr.getFlowType().isTerminal()) { | ||
| return; | ||
| } | ||
| if (mnemonic.equals("fret")) { | ||
| setPrototypeModel(program, instr, "__fastcall"); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential for Incomplete Calling Convention Detection
The checkReturn method first checks if the instruction is null, then attempts to access the mnemonic string from it. This creates a potential null pointer exception risk if instr is null. While the current implementation checks for null after using the object, this could lead to unexpected behavior if the code is modified in the future.
| void checkReturn(Program program, Instruction instr) { | |
| String mnemonic = instr.getMnemonicString().toLowerCase(); | |
| if (instr == null || !instr.getFlowType().isTerminal()) { | |
| return; | |
| } | |
| if (mnemonic.equals("fret")) { | |
| setPrototypeModel(program, instr, "__fastcall"); | |
| return; | |
| } | |
| } | |
| void checkReturn(Program program, Instruction instr) { | |
| if (instr == null || !instr.getFlowType().isTerminal()) { | |
| return; | |
| } | |
| String mnemonic = instr.getMnemonicString().toLowerCase(); | |
| if (mnemonic.equals("fret")) { | |
| setPrototypeModel(program, instr, "__fastcall"); | |
| return; | |
| } | |
| } |
Standards
- CWE-476
- Secure Coding Best Practices
Security Implications of Tricore FCALL Analyzer Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Tricore FCALL/FRET Calling Convention Analysis👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| void checkReturn(Program program, Instruction instr) { | ||
| String mnemonic = instr.getMnemonicString().toLowerCase(); | ||
|
|
||
| if (instr == null || !instr.getFlowType().isTerminal()) { | ||
| return; | ||
| } | ||
| if (mnemonic.equals("fret")) { | ||
| setPrototypeModel(program, instr, "__fastcall"); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing FCALL Analysis
The analyzer only handles FRET instructions but not FCALL instructions despite the class name and description. This creates inconsistent calling convention detection since only function returns are analyzed without corresponding call sites.
| void checkReturn(Program program, Instruction instr) { | |
| String mnemonic = instr.getMnemonicString().toLowerCase(); | |
| if (instr == null || !instr.getFlowType().isTerminal()) { | |
| return; | |
| } | |
| if (mnemonic.equals("fret")) { | |
| setPrototypeModel(program, instr, "__fastcall"); | |
| return; | |
| } | |
| } | |
| void checkInstruction(Program program, Instruction instr) { | |
| String mnemonic = instr.getMnemonicString().toLowerCase(); | |
| if (instr.getFlowType().isTerminal()) { | |
| if (mnemonic.equals("fret")) { | |
| setPrototypeModel(program, instr, "__fastcall"); | |
| return; | |
| } | |
| } else if (mnemonic.equals("fcall")) { | |
| Function calledFunction = program.getFunctionManager().getFunctionAt(instr.getFlows()[0]); | |
| if (calledFunction != null) { | |
| try { | |
| calledFunction.setCallingConvention("__fastcall"); | |
| } catch (InvalidInputException e) { | |
| Msg.error(this, "Unexpected Exception: " + e.getMessage(), e); | |
| } | |
| } | |
| } | |
| } |
Standards
- Logic-Verification-Completeness
- Algorithm-Correctness-Pattern-Matching
| void checkReturn(Program program, Instruction instr) { | ||
| String mnemonic = instr.getMnemonicString().toLowerCase(); | ||
|
|
||
| if (instr == null || !instr.getFlowType().isTerminal()) { |
There was a problem hiding this comment.
Null Check Redundancy
Null check on instr is redundant as the method already uses instr to call getMnemonicString() before this check. If instr were null, a NullPointerException would occur at line 60.
| if (instr == null || !instr.getFlowType().isTerminal()) { | |
| if (!instr.getFlowType().isTerminal()) { |
Standards
- Logic-Verification-Consistency
- Algorithm-Correctness-Control-Flow
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Calling Convention Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| return canDo; | ||
| } | ||
|
|
||
| void checkReturn(Program program, Instruction instr) { |
There was a problem hiding this comment.
Unused Method Parameter
The program parameter is passed to checkReturn() but never used within the method. This creates maintenance confusion and potential reliability issues if the parameter is intended for future functionality.
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- ISO-IEC-25010-Reliability-Maturity
While working with tricore binary I found that some small routines can be called through
FCALL(fast call) opcode, not by commonCALL.And even though it is already supported in sleigh, it produces a wrong decompiled code if caller function uses stack variables, because
CALLandFCALLhandle stack in a different manner.So, here is two patches:
some notes:
FCALLcan be used in the C++ code (to be honest, I have never seen C++ tricore binaries)