Added inspection to warn about unexpected condition code unaffecting instructions before conditional instructions.
Extended documentation. Bugfix in M68kDeadWriteInspection.
This commit is contained in:
parent
2abb5af8b0
commit
1dcf288d27
67
README.md
67
README.md
@ -37,6 +37,70 @@ good enough" to get started, and I can return to demo coding with its current st
|
||||
- Structure view
|
||||
- Documentation provider for symbol definitions and mnemonics (listing available addressing modes etc.).
|
||||
|
||||
### Inspections
|
||||
|
||||
#### M68kSyntaxInspection - Assembly instruction validity
|
||||
|
||||
Checks the validity of the current instruction. If an instruction is not recognized, you may get one of the following errors:
|
||||
|
||||
- Instruction _mnemonic_ not supported on selected cpu (you won't get this currently as only MC68000 is supported)
|
||||
- No operands expected for _mnemonic_
|
||||
- Second operand _op_ unexpected for _mnemonic_
|
||||
- Unsupported addressing mode for _mnemonic_
|
||||
- Unsupported addressing mode _op_ for first operand of _mnemonic_
|
||||
- Unsupported addressing mode _op_ for second operand of _mnemonic_
|
||||
- Unsupported addressing modes for operands in this order for _mnemonic_ (try swapping)
|
||||
- Instruction _mnemonic_ is unsized (you tried to specify `.b`, `.w` or `.l`)
|
||||
- Operation size _(.b,.w,.l)_ unsupported for _mnemonic_
|
||||
- Operation size _(.b,.w,.l)_ unsupported (should be _(.b,.w,.l)_)
|
||||
|
||||
#### M68kDeadWriteInspection - Dead writes to registers
|
||||
|
||||
This inspection looks at register writes and tries to find instructions that renders a write moot because it was overwritten by another instruction before
|
||||
anything useful was done with it.
|
||||
|
||||
Analysis is aborted at global labels, flow control instructions, directives
|
||||
(e.g. conditional assembly) and macros with the register names as parameter.
|
||||
|
||||
The inspection tries to take condition code changing into account and puts out a weak warning if the statement merely changes condition codes before the
|
||||
contents of the register are overwritten. In this case, it is sometimes better to replace `move` by `tst`.
|
||||
|
||||
#### M68kUnexpectedConditionalInstructionInspection - Unaffected condition codes before conditional instruction
|
||||
|
||||
Especially for novice coders, it is not clear that some instructions do not affect the condition codes for a subsequent condition branch or `scc` instruction.
|
||||
`movea`, `adda` and `suba` come to my mind.
|
||||
|
||||
The inspection will report such suspicious instruction sequences.
|
||||
|
||||
However, this does not need to be a programming error. Advanced coders sometimes make use of the fact that instructions do not change condition codes and thus
|
||||
optimize the order of execution.
|
||||
|
||||
### Documentation provider
|
||||
|
||||
#### M68kSymbolDefinitionDocumentationProvider
|
||||
|
||||
Provides the assigned value of a `=`, `set` or `equ` symbol definition when hovering over a symbol.
|
||||
|
||||
#### M68kRegisterFlowDocumentationProvider
|
||||
|
||||
When hovering over or placing the cursor at a data or address register, the documentation will scan through the instructions backwards and forwards and will
|
||||
show all read, changes of the register contents. It does this until an instruction is found that defines (sets) the contents of the register
|
||||
(according to the size of the instruction where the cursor was placed).
|
||||
|
||||
The analysis ignores all code flow instructions and might be inaccurate for branches, macro use, etc. It also stops at global labels.
|
||||
|
||||
The documentation will search up to 100 instructions in each direction, but only four when hovering over the register
|
||||
(so if you need the whole analysis, use the documentation window).
|
||||
|
||||
#### M68kInstructionDocumentationProvider
|
||||
|
||||
When hovering over a mnemonic, it will show a short description of the assembly instruction.
|
||||
|
||||
For the documentation window, affected condition codes, allowed operation sizes and addressing modes are listed for the selected instruction
|
||||
(so only stuff from `cmpa` is listed when you're looking at a `cmp.w a0,a1` instruction).
|
||||
|
||||
If the current statement has no valid syntax, the instruction details of all matching mnemonics will be shown instead.
|
||||
|
||||
## Known issues
|
||||
|
||||
- `Find Usages` always shows _"Unclassified"_ though it shouldn't (?)
|
||||
@ -80,7 +144,8 @@ make it work with JUnit 5. Feel free to use the code (in package ```de.platon42.
|
||||
- Performance: Optimized mnemonic lookup.
|
||||
- Enhancement: Reworked Instruction Documentation provider, now shows condition codes.
|
||||
- Bugfix: In ISA `exg` is no longer treated as setting a definitive value.
|
||||
- New: Added inspection find dead writes to registers!
|
||||
- New: Added inspection to find dead writes to registers!
|
||||
- New: Added inspection to warn about unexpected condition code unaffecting instructions before conditional instructions.
|
||||
|
||||
### V0.4 (03-Aug-21)
|
||||
|
||||
|
||||
@ -66,7 +66,8 @@ patchPluginXml {
|
||||
<li>Performance: Optimized mnemonic lookup.
|
||||
<li>Enhancement: Reworked Instruction Documentation provider, now shows condition codes.
|
||||
<li>Bugfix: In ISA exg is no longer treated as setting a definitive value.
|
||||
<li>New: Added inspection find dead writes to registers!
|
||||
<li>New: Added inspection to find dead writes to registers!
|
||||
<li>New: Added inspection to warn about unexpected condition code unaffecting instructions before conditional instructions.
|
||||
</ul>
|
||||
<h4>V0.4 (03-Aug-21)</h4>
|
||||
<ul>
|
||||
|
||||
@ -19,8 +19,8 @@ class M68kDeadWriteInspection : AbstractBaseM68kLocalInspectionTool() {
|
||||
companion object {
|
||||
private const val DISPLAY_NAME = "Dead writes to registers"
|
||||
|
||||
private const val DEAD_WRITE_MSG = "Register %s is overwritten later without being used"
|
||||
private const val POSSIBLY_DEAD_WRITE_MSG = "Register %s is overwritten later (only CC evaluated?)"
|
||||
private const val DEAD_WRITE_MSG_TEMPLATE = "Register %s is overwritten later without being used"
|
||||
private const val POSSIBLY_DEAD_WRITE_MSG_TEMPLATE = "Register %s is overwritten later (only CC evaluated?)"
|
||||
}
|
||||
|
||||
override fun getDisplayName() = DISPLAY_NAME
|
||||
@ -69,7 +69,7 @@ class M68kDeadWriteInspection : AbstractBaseM68kLocalInspectionTool() {
|
||||
val currAsmInstruction = PsiTreeUtil.getChildOfType(currStatement, M68kAsmInstruction::class.java) ?: continue
|
||||
val (isaData, currAdrMode) = findExactIsaDataAndAllowedAdrModeForInstruction(currAsmInstruction) ?: continue
|
||||
if (isaData.changesControlFlow) break
|
||||
val testedCc = getConcreteTestedCcFromMnemonic(currAsmInstruction.asmOp.mnemonic, isaData, adrMode)
|
||||
val testedCc = getConcreteTestedCcFromMnemonic(currAsmInstruction.asmOp.mnemonic, isaData, currAdrMode)
|
||||
if (((testedCc and ccModification) > 0) && !ccOverwritten) ccTested = true
|
||||
if (currAdrMode.affectedCc != 0) ccOverwritten = true
|
||||
if (checkIfInstructionUsesRegister(currAsmInstruction, register)) {
|
||||
@ -89,7 +89,7 @@ class M68kDeadWriteInspection : AbstractBaseM68kLocalInspectionTool() {
|
||||
manager.createProblemDescriptor(
|
||||
asmInstruction,
|
||||
asmInstruction,
|
||||
(if (ccTested) POSSIBLY_DEAD_WRITE_MSG else DEAD_WRITE_MSG).format(register.regname),
|
||||
(if (ccTested) POSSIBLY_DEAD_WRITE_MSG_TEMPLATE else DEAD_WRITE_MSG_TEMPLATE).format(register.regname),
|
||||
if (ccTested) ProblemHighlightType.WEAK_WARNING else ProblemHighlightType.WARNING,
|
||||
isOnTheFly
|
||||
)
|
||||
|
||||
@ -0,0 +1,54 @@
|
||||
package de.platon42.intellij.plugins.m68k.inspections
|
||||
|
||||
import com.intellij.codeInspection.InspectionManager
|
||||
import com.intellij.codeInspection.ProblemDescriptor
|
||||
import com.intellij.codeInspection.ProblemHighlightType
|
||||
import com.intellij.psi.util.PsiTreeUtil
|
||||
import de.platon42.intellij.plugins.m68k.asm.M68kIsa.findMatchingInstructions
|
||||
import de.platon42.intellij.plugins.m68k.psi.*
|
||||
import de.platon42.intellij.plugins.m68k.utils.M68kIsaUtil
|
||||
import de.platon42.intellij.plugins.m68k.utils.M68kIsaUtil.findExactIsaDataAndAllowedAdrModeForInstruction
|
||||
|
||||
class M68kUnexpectedConditionalInstructionInspection : AbstractBaseM68kLocalInspectionTool() {
|
||||
|
||||
companion object {
|
||||
private const val DISPLAY_NAME = "Unaffected condition codes before conditional instruction"
|
||||
|
||||
private const val UNAFFECTED_CONDITION_CODES_MSG_TEMPLATE = "Condition codes unaffected by instruction (%s - %s)"
|
||||
}
|
||||
|
||||
override fun getDisplayName() = DISPLAY_NAME
|
||||
|
||||
override fun checkAsmInstruction(asmInstruction: M68kAsmInstruction, manager: InspectionManager, isOnTheFly: Boolean): Array<ProblemDescriptor>? {
|
||||
val asmOp = asmInstruction.asmOp
|
||||
if (asmInstruction.addressingModeList.isEmpty()) return emptyArray()
|
||||
|
||||
val isaDataCandidates = findMatchingInstructions(asmOp.mnemonic)
|
||||
if (isaDataCandidates.isEmpty()) return emptyArray()
|
||||
val (isaData, adrMode) = findExactIsaDataAndAllowedAdrModeForInstruction(asmInstruction) ?: return emptyArray()
|
||||
if ((adrMode.affectedCc > 0) || (adrMode.testedCc > 0) || isaData.changesControlFlow) return emptyArray()
|
||||
|
||||
var currStatement = asmInstruction.parent as M68kStatement
|
||||
while (true) {
|
||||
currStatement = PsiTreeUtil.getNextSiblingOfType(currStatement, M68kStatement::class.java) ?: break
|
||||
val globalLabel = PsiTreeUtil.findChildOfType(currStatement, M68kGlobalLabel::class.java)
|
||||
if (globalLabel != null) break
|
||||
if (PsiTreeUtil.findChildOfAnyType(currStatement, M68kMacroCall::class.java, M68kPreprocessorDirective::class.java) != null) break
|
||||
val currAsmInstruction = PsiTreeUtil.getChildOfType(currStatement, M68kAsmInstruction::class.java) ?: continue
|
||||
val (currIsaData, currAdrMode) = findExactIsaDataAndAllowedAdrModeForInstruction(currAsmInstruction) ?: break
|
||||
val testedCc = M68kIsaUtil.getConcreteTestedCcFromMnemonic(currAsmInstruction.asmOp.mnemonic, currIsaData, currAdrMode)
|
||||
if (testedCc == 0) break
|
||||
|
||||
return arrayOf(
|
||||
manager.createProblemDescriptor(
|
||||
asmInstruction,
|
||||
asmInstruction,
|
||||
UNAFFECTED_CONDITION_CODES_MSG_TEMPLATE.format(isaData.mnemonic, isaData.description),
|
||||
ProblemHighlightType.WARNING,
|
||||
isOnTheFly
|
||||
)
|
||||
)
|
||||
}
|
||||
return emptyArray()
|
||||
}
|
||||
}
|
||||
@ -53,6 +53,9 @@
|
||||
<localInspection implementationClass="de.platon42.intellij.plugins.m68k.inspections.M68kDeadWriteInspection"
|
||||
displayName="Dead writes to registers" groupName="M68k"
|
||||
enabledByDefault="true" level="WARNING"/>
|
||||
<localInspection implementationClass="de.platon42.intellij.plugins.m68k.inspections.M68kUnexpectedConditionalInstructionInspection"
|
||||
displayName="Unaffected condition codes before conditional instruction" groupName="M68k"
|
||||
enabledByDefault="true" level="WARNING"/>
|
||||
</extensions>
|
||||
|
||||
<actions>
|
||||
|
||||
@ -0,0 +1,18 @@
|
||||
<html>
|
||||
<body>
|
||||
Usually, it is expected that an instruction checking the condition codes or using
|
||||
the condition codes is preceded by an instruction that actually affects the condition
|
||||
codes. This inspection checks that this is the case.
|
||||
|
||||
For example, the 'movea' and 'adda' instructions (which can be (and are!) often written
|
||||
as 'move' and 'add') do not affect the condition codes and a conditional branch
|
||||
will not work as expected.
|
||||
|
||||
However, this does not need to be a programming error. Advanced coders sometimes
|
||||
make use of the fact that instructions do not change condition codes and thus
|
||||
optimize the order of execution.
|
||||
|
||||
<!-- tooltip end -->
|
||||
Analysis is terminated at the next global label, macrocall or preprocessor statement.
|
||||
</body>
|
||||
</html>
|
||||
@ -0,0 +1,74 @@
|
||||
package de.platon42.intellij.plugins.m68k.inspections
|
||||
|
||||
import com.intellij.testFramework.fixtures.CodeInsightTestFixture
|
||||
import de.platon42.intellij.jupiter.MyFixture
|
||||
import org.assertj.core.api.Assertions.assertThat
|
||||
import org.junit.jupiter.api.Test
|
||||
|
||||
internal class M68kUnexpectedConditionalInstructionInspectionTest : AbstractInspectionTest() {
|
||||
|
||||
@Test
|
||||
internal fun movea_causes_warning_when_used_for_conditional_branching(@MyFixture myFixture: CodeInsightTestFixture) {
|
||||
myFixture.enableInspections(M68kUnexpectedConditionalInstructionInspection::class.java)
|
||||
myFixture.configureByText(
|
||||
"unexpectedcc.asm", """
|
||||
move.l d0,a1
|
||||
bne.s .cont
|
||||
rts
|
||||
.cont
|
||||
"""
|
||||
)
|
||||
assertHighlightings(myFixture, 1, "Condition codes unaffected by instruction (movea - Move Address)")
|
||||
}
|
||||
|
||||
@Test
|
||||
internal fun no_warning_on_consecutive_conditional_branches(@MyFixture myFixture: CodeInsightTestFixture) {
|
||||
myFixture.enableInspections(M68kUnexpectedConditionalInstructionInspection::class.java)
|
||||
myFixture.configureByText(
|
||||
"unexpectedcc.asm", """
|
||||
move.b P61_arplist(pc,d0),d0
|
||||
beq.b .arp0
|
||||
bmi.b .arp1
|
||||
"""
|
||||
)
|
||||
assertThat(myFixture.doHighlighting()).isEmpty()
|
||||
}
|
||||
|
||||
@Test
|
||||
internal fun no_warning_on_macro_call_inbetween(@MyFixture myFixture: CodeInsightTestFixture) {
|
||||
myFixture.enableInspections(M68kUnexpectedConditionalInstructionInspection::class.java)
|
||||
myFixture.configureByText(
|
||||
"unexpectedcc.asm", """
|
||||
move.l pd_PalCurShamPalPtr(a4),a1
|
||||
PALSTEPDOWN
|
||||
bne.s .loopline
|
||||
"""
|
||||
)
|
||||
assertThat(myFixture.doHighlighting()).isEmpty()
|
||||
}
|
||||
|
||||
@Test
|
||||
internal fun no_warning_flow_control_instruction(@MyFixture myFixture: CodeInsightTestFixture) {
|
||||
myFixture.enableInspections(M68kUnexpectedConditionalInstructionInspection::class.java)
|
||||
myFixture.configureByText(
|
||||
"unexpectedcc.asm", """
|
||||
bsr foo
|
||||
bne.s .loopline
|
||||
"""
|
||||
)
|
||||
assertThat(myFixture.doHighlighting()).isEmpty()
|
||||
}
|
||||
|
||||
@Test
|
||||
internal fun warning_on_conditional_set_series_with_suba(@MyFixture myFixture: CodeInsightTestFixture) {
|
||||
myFixture.enableInspections(M68kUnexpectedConditionalInstructionInspection::class.java)
|
||||
myFixture.configureByText(
|
||||
"unexpectedcc.asm", """
|
||||
sub.l a0,a0
|
||||
seq d0
|
||||
sne d1
|
||||
"""
|
||||
)
|
||||
assertHighlightings(myFixture, 1, "Condition codes unaffected by instruction (suba - Subtract Address)")
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue
Block a user