[RVC] Add support for variable length instructions in the InstructionModel

[mortbopet]

Currently, the InstructionModel (bottom right table in the processor tab) is built around the notion that all instructions in a program is equal in width. An example is the following, where we assume that an instruction is 4 bytes wide:

Ripes/src/instructionmodel.cpp

Lines 7 to 12 in 9a708c2

	AInt InstructionModel::indexToAddress(const QModelIndex& index) const {
	if (auto prog_spt = ProcessorHandler::getProgram()) {
	return (index.row() * 4) + prog_spt->getSection(TEXT_SECTION_NAME)->address;
	}
	return 0;
	}

Instead, we need a more advanced solution which informs the instruction model about relative addresses based on the result from disassembling. As a reminder, a disassembly result contains both the disassembled string as well as the number of bytes disassembled (instruction width): https://github.com/mortbopet/Ripes/blob/master/src/assembler/assembler.h#L73.

Currently we're simply taking whatever result we get from disassembling and setting that as the value in the column containing disassembled instructions, without it interacting with the rest of the instruction model logic:

Ripes/src/instructionmodel.cpp

Lines 132 to 134 in 9a708c2

	QVariant InstructionModel::instructionData(AInt addr) const {
	return ProcessorHandler::disassembleInstr(addr);
	}

[lcgamboa]

I changed the indexToAddress function to support RVC instructions. It was functional, but not very optimized.

AInt InstructionModel::indexToAddress(const QModelIndex& index) const {
    if (ProcessorHandler::currentISA()->extensionEnabled ("C"))
    {
        if (auto prog_spt = ProcessorHandler::getProgram()) {
          AInt addr=0;
          for(int rowC = 0 ; rowC < index.row(); rowC++)
            {
                const VInt instr = ProcessorHandler::getMemory().readMem(addr, 2);
                if((instr & 0b11) == 0b11) //32bits
                  {
                    addr += 2;
                  }
                addr += 2;
            }
          return addr +  prog_spt->getSection(TEXT_SECTION_NAME)->address;
        }
    }       
  else
    {
       if (auto prog_spt = ProcessorHandler::getProgram()) {
           return (index.row() * 4) + prog_spt->getSection(TEXT_SECTION_NAME)->address;
       }
    }
    return 0;
}

But it only worked on the processortab, on the edittab it didn't .

I looked, but I couldn't find where to make similar changes in edittab, any tips?

When it's more usable I'll put my modifications on github fork.

[mortbopet]

great progress! My initial thought is that it seems like the implementation is RISC-V-dependent. Now, this doesn't matter in the current state of Ripes (since only RISC-V is supported). However, code should in general work in an ISA-agnostic manner, such that we don't have to rewrite the entire UI whenever we switch to processors of other ISAs. If you find that there's some functionality lacking in the ISAInfo interface, then we should brainstorm on this.

As suggested in my initial post, a disassembly result contains both the disassembled string as well as the number of bytes disassembled (instruction width). I imagine that this (somehow) can help with the "index-to-address" logic.

Ripes/src/assembler/assemblerbase.h

Lines 14 to 21 in 7ec2c53

	struct OpDisassembleResult {
	/// Stringified representation of a disassembled machine word.
	QString repr;
	/// Number of bytes disassembled.
	unsigned bytesDisassembled;
	/// Optional error that occured during disassembly.
	std::optional<Error> err;
	};

I looked, but I couldn't find where to make similar changes in edittab, any tips?

Disassembly support for the edittab should hopefully already be implemented if you pull the latest version of the c_ext branch.

[lcgamboa]

I uploaded the code lcgamboa@b11c6b7 with the current modifications. I'm not a C++ template user, please check if the coding style is acceptable. Feel free to suggest modifications, as you have full knowledge of the project, you can have a better view than mine, which only studied some parts.

[mortbopet]

Would it be possible to split up the code into distinct PRs with respect to the issue in #125? This will make review a lot easier. A PR can be submitted against any branch (in this case, c_ext):

[lcgamboa]

Programviewer line highlighting does not currently work because of considering all instructions 32-bit .

Ripes/src/programviewer.cpp

Line 215 in 972390b

(addr - ProcessorHandler::get()->getTextStart()) / ProcessorHandler::currentISA()->instrBytes();

The information needed to calculate the line number already exists in the map I added to the instructionmodel
https://github.com/lcgamboa/Ripes/blob/7881089e3737c9b134f15820432ea8b53825902e/src/instructionmodel.h#L54

With the m_indexToAddress map in the instructionmodel object I believe I can't access it from the programviewer's blockForAddress method.

What do you think of putting the m_indexToAddress map in the ProcessorHandler class instead of the instructiomodel class? This would solve the blockForAddress method access problem.

I need to use programviewer instead of intructionmodel as I'm testing the unit test code #137.

[mortbopet]

@lcgamboa #142 should do this; took your function for calculating the address offset map from #135 and moved it to the Program. Made it as a PR to keep you in sync.

Next up is then using this in the ProgramViewer - I'd think a change is needed to objdump such that it can use this mapping (to avoid disassembling the program twice) and together with the m_labelAddrOffsetMap in ProgramViewer we should be able to correctly determine the block-to-address mapping.