Add V850E2M Architecture

[Aleckaj]

No description provided.

[mumbel]

just read through really quick, didn't ref the manual much, so hopefully most of my comments are applicable. one suggestion is you could make more tables for your actions to reduce some code duplication.

[mumbel]

might clean up whitespace in this area

[Aleckaj]

Oh right, I worked with VSCode. I will 'fix' that

[mumbel]

This is tricky, if 2731 == 1115 or 2731 == 0004 then the divide is corrupted. I would save the registers into locals and then you can safely do the right hand side of modulus and div calculation with those.

[mumbel]

same comment as div

[mumbel]

same comment as div

[mumbel]

same comment as div

[mumbel]

I'm not great at float code in SLEIGH, but is the float2float needed here?, i think its used to move between precision, but it looks like you're starting with 8-byte and ending in 8-byte

[esaulenka]

current code is correct, because R1115 (and any other register without x2 suffix) is 32-bits wide

[mumbel]

Oops, thanks @esaulenka, I commented on wrong line. ceilf.dul and ceilf.dl for example though

[mumbel]

same comment as the other div

[mumbel]

same comment as div

[mumbel]

you might add a pcodeop synce and just call it here

[Aleckaj]

I can't find the Operation synce in the P-code Table of SLEIGH

[mumbel]

You can define your own: define pcodeop synce;

In the function you can then call it:

{
    synce(); // you can have args here if you want, like an immediate or register
}

And then if it comes across that instruction the decompiler will insert synce(). Just a suggestion so you can at least see the instruction hitting instead of a NOP

just for example, you can see their toy processor:

ghidra/Ghidra/Processors/Toy/data/languages/toy_builder.sinc

Line 61 in bdb52bd

define pcodeop cop1;

[Aleckaj]

Alright, thank you!

[mumbel]

it looks like some float functions pass through here, you would need another set for f+

[Aleckaj]

Ahh alright, if I multiply a float number with an integer, it gets an integer afterwords with the Symbol '*'. I still had the old view from C

[esaulenka]

I think, its better to change to variant with if (...) goto <...> conditions.
It much easer for a man to understand.

[esaulenka]

I am already fixed this typo ;-)

[Aleckaj]

Thats crazy. I was 100% sure that I fixed that too :D

[esaulenka]

@Aleckaj, sorry, I forgot to reply to your email. I will definitely do it.

Please, sync your code with latest changes: added index for manual and fixed some bugs.

Also, why you added only synce() pcodeop? How do the others synXX differ?
Please add pcodeops for disable/enable interrupts and nop's too. Sometimes it is important to understanding program logic.

[Aleckaj]

@esaulenka You are totally right. I will add them as soon as possible and sync it with your rep ^^
Thank you for your help again

[mumbel]

I would not do this. I don't know the processor, but any sort of using nop for alignment is going to make for terrible decompiler output

[Aleckaj]

Should I leave PC = inst_next; or let this Implementation clean?

[mumbel]

That should be good, I kinda like that.

I have done something like:

local tmp = 0;
tmp = tmp;

To remove bookmarks about empty implementation

[esaulenka]

but any sort of using nop for alignment

I am still not familiar with all compilers, but three V850 binaries, that i am tested, uses NOPs only as very small delays in low-level drivers.

BTW, it also applies to Infineon binaries.

[esaulenka]

@ ghidra team
Do you need any test binaries for this MCUs ?

[esaulenka]

@Aleckaj, please add two files in Ghidra/Processors/V850E2M/ for builder subsystem

build.gradle

apply from: "$rootProject.projectDir/gradle/distributableGhidraModule.gradle"
apply from: "$rootProject.projectDir/gradle/processorProject.gradle"
apply plugin: 'eclipse'
eclipse.project.name = 'Processors v850'

and certification.manifest

##VERSION: 2.0
Module.manifest||GHIDRA||||END|
build.gradle||GHIDRA||||END|
data/languages/V850E2M.cspec||GHIDRA||||END|
data/languages/V850E2M.ldefs||GHIDRA||||END|
data/languages/V850E2M.pspec||GHIDRA||||END|
data/languages/V850E2M.slaspec||GHIDRA||||END|
data/languages/Helpers/Conditions.sinc||GHIDRA||||END|
data/languages/Helpers/Extras.sinc||GHIDRA||||END|
data/languages/Helpers/Macros.sinc||GHIDRA||||END|
data/languages/Helpers/Register.sinc||GHIDRA||||END|
data/languages/Helpers/Tokens.sinc||GHIDRA||||END|
data/languages/Helpers/Variables.sinc||GHIDRA||||END|
data/languages/Instructions/Arithmetic.sinc||GHIDRA||||END|
data/languages/Instructions/BitSearch.sinc||GHIDRA||||END|
data/languages/Instructions/BitManipulation.sinc||GHIDRA||||END|
data/languages/Instructions/Branch.sinc||GHIDRA||||END|
data/languages/Instructions/Conditional.sinc||GHIDRA||||END|
data/languages/Instructions/DataManipulation.sinc||GHIDRA||||END|
data/languages/Instructions/Divide.sinc||GHIDRA||||END|
data/languages/Instructions/Float.sinc||GHIDRA||||END|
data/languages/Instructions/HighSpeedDivide.sinc||GHIDRA||||END|
data/languages/Instructions/Load.sinc||GHIDRA||||END|
data/languages/Instructions/Logic.sinc||GHIDRA||||END|
data/languages/Instructions/Multiply.sinc||GHIDRA||||END|
data/languages/Instructions/MultiplyAccumulate.sinc||GHIDRA||||END|
data/languages/Instructions/Saturated.sinc||GHIDRA||||END|
data/languages/Instructions/Special.sinc||GHIDRA||||END|
data/languages/Instructions/Store.sinc||GHIDRA||||END|
data/manuals/v850.idx||GHIDRA||||END|

It can be just copied from other processor modules.

[esaulenka]

please rewrite all names of conditions in lower case, so that they match the rest of the code.

[esaulenka]

There is should be second jmp [reg1] table, without op0004=0x1F condition. It is used for GOTOs, specifed in any register.

[esaulenka]

@Aleckaj, thanks! I successfully parsed sample binary. Produced code works as real hardware!

[Aleckaj]

@esaulenka I'm happy to hear that :)
I hope the pull request will be accepted soon

[EgorKin]

Tested on binary for v850E1. Works really good. Only 1 "not a bug" - E1 & E2 vs E2M allow use R0 as reg1 in MOV reg2, reg1 instructions. Can you add workaround for it for E1 & E2 support too?

And I'm not sure that callt instructions shown right in decompile view - in my binary callt often use at begin & end of functions. Second callt does not shown as FUN_xxxxxxxx(); but as (*(code *)((int)pwVar2 + (uint)pwVar2[0x16]))(); and functions does not end at callt instruction and continues longer than it need.

In Memory Map this ROM section marked as Read & Execute (no Write). If I set Write first callt convert to(*(code *)((int)pwVar..... code too.
Is it my fault or something else?

[mumbel]

@EgorKin callt returns to the next instruction, stored in CTPC, in the sleigh code. Does fun_00002aba() change CTPC or other control flows so it would not return to the instruction after that callt? Also is 0x2996 structured data, making that memory into a struct may help in decompiler, not 100% sure though.

Sorry untested, so maybe broken/nothing/worse, but this might improve callt

calltadr: op0005 is op0005 { local adr:4 = CTBP + (op0005 << 1); local tmp:4 = CTBP + zext(*:2 adr); export tmp; }
# CALLT imm6 - 0000001000iiiiii
:callt calltadr is op0615=0x8 & calltadr
{
	CTPC = inst_next;
	CTPSW = PSW;
	PC = calltadr;
	call [PC];
}

[EgorKin]

0x2996 is marked as word data:

And yes, Fun_00002aba(); change CTPC reg:

But Fun_000029be() is:

000029be 80 07 21 00     prepare    { lp },0x0

000029c2 e0 07 44 01     ctret

[esaulenka]

@EgorKin, in your case 0x0? 0x00 is a not instruction, it is data, showing stack offset for current function. It should be used in CallT subroutines, but now I can not achive correct handling of CALLT instruction.
If you can share your binary, (or, may be, part of it) it can be discussed more detail.

[EgorKin]

@esaulenka Hi Alex, I already sent you this binary then we discuss your v850 proc support code a few days before.

@mumbel, your modification fixes now func length. It's a bit strange for me because I expected that callt offset changes to address (callt 0x0=>DAT_00002966 changes to callt 0x000029be for example)
But now after reanalyze:

I try play a bit with callt code and if I hardcode CTBP as 0x2966 then second callt decompiled successful. But I got a * WARNING: Subroutine does not return * remark.

CTBP is 0x2966 for all binary.

Don't know ho to combine it to proper solution.

[emteere]

@esaulenka If you can provide the pcodetest configuration for compiling the pcodetest binaries for the V850, that would help verify the processor. We try not to put binary files into the repository.
Once you have compiled them and they run, then attach them here so we can take a look. I'm sure we can probably locate a sample V850 binary, but known compiled code helps in finding issues and automating testing. If that is a tough ask, we might be able to come up with a v850 configuration and provide the test setup as we did for the tricore.

Ghidra/Extensions/SleighDevTools/pcodetest/pcode_defs.py

I'd like to see the pcodetest configuration files eventually split into each processor directory, but for now this is what we have.

[mumbel]

@EgorKin curious if you cleared the disassembly for that instruction and re-disassembled it (was expecting to see callt 0x16 though not sure if that would change anything) and your second screenshot of decompile is actually what I was thinking would happen.
Are you certain if FUN_00002ac8() is correct as a function, doesn't that make FUN_00002aba() a little off?

[EgorKin]

@mumbel As I understand v850 docs callt can be end at ctret instruction or at dispose 0, {lp}, [lp].
In my example callt 0x29BE is

prepare {lp}, 0
ctret

and callt 0x2ABA is

dispose 0, {lp}, [lp]

At another part of binary another func begin from callt 0x2A12

prepare {lp}, 0
prepare {r27-r29}, 0
stsr    CTPC, r29
ctret

and end at callt 0x2B3A

dispose 0, {r27-r29}
dispose 0, {lp}, [lp]

So I decrease FUN_00002aba() length

but it does not change anything.

I try re-disassembled code at 0x27aee but it still as at first sshot. I also try make new project with same binary and after first Auto Analyze I got too long code as I mention early. But if I made Auto Analyze again (with same settings) - func length fixed. So I back to original code from this pull request and got same result - second Analyze gave right func size at Decompile view.

Only when I changed callt in Special.sinc file to (hardcode 0x2966 value)

# CALLT imm6 - 0000001000iiiiii
:callt op0005 is op0615=0x8 & op0005 
{
	CTPC = inst_next;
	CTPSW = PSW;
	local adr:4 = 0x2966 + (op0005 << 1);
	PC = 0x2966 + zext(*:2 adr);
	call [PC];
}

I got second sshot with correct second callt disassembly record.

[GhidorahRex]

Overall the sleigh is very well done. A few minor changes and a couple more significant ones.

The project should be moved to Processors/V850 rather than V850E2M, to support the entire processor family under a single umbrella. Most of the .sinc files should be combined together to reduce clutter and make it easier to find instructions.

[GhidorahRex]

A couple additional changes based on the update

[Aleckaj]

Hi @GhidorahRex, i fixed the little bugs and added r20-r29 in the unaffected list of .cspec. If I try to add r30 and r31 the decompiler stops working. I really dont know why.

And I got a question to your function in Macros.sinc:

macro shift_right_logic(res, var, shift_)
{
	local shift = (shift_ & 0x1f);
	local mask = (zext(shift != 0) * var & (1 << (shift - 1)) + (zext(shift == 0) * 0);
	res = var >> shift;
	set_OV0_S_Z(res);
	$(CY) = ((mask != 0) && (shift != 0));
}

In the last calculation of mask (zext(shift == 0) * 0) you always multiply with 0. Can you explain me why it's necessary?

[GhidorahRex]

@Aleckaj : Regarding r30 and r31 - you've named them as ep and lp in the register definitions, so try adding <register name="ep"/> and <register name="lp"/>.

See mumbel's comment on my comment for the mask. I wrote down the short-circuiting math wrong. Sorry!

One final word that should be addressed: you need to add newlines to the end of your files.

[Aleckaj]

@GhidorahRex it worked out fine 👍 It's been a long time since I programmed it. Thank you for the help :)

[esaulenka]

@Aleckaj, please fill manifest with new file list.

diff --git a/Ghidra/Processors/V850/certification.manifest b/Ghidra/Processors/V850/certification.manifest      
index ed4cb836..6355d369 100644                                                                                 
--- a/Ghidra/Processors/V850/certification.manifest                                                             
+++ b/Ghidra/Processors/V850/certification.manifest                                                             
@@ -12,19 +12,8 @@ data/languages/Helpers/Register.sinc||GHIDRA||||END|                                         
 data/languages/Helpers/Tokens.sinc||GHIDRA||||END|                                                             
 data/languages/Helpers/Variables.sinc||GHIDRA||||END|                                                          
 data/languages/Instructions/Arithmetic.sinc||GHIDRA||||END|                                                    
-data/languages/Instructions/BitSearch.sinc||GHIDRA||||END|                                                     
-data/languages/Instructions/BitManipulation.sinc||GHIDRA||||END|                                               
-data/languages/Instructions/Branch.sinc||GHIDRA||||END|                                                        
-data/languages/Instructions/Conditional.sinc||GHIDRA||||END|                                                   
-data/languages/Instructions/DataManipulation.sinc||GHIDRA||||END|                                              
-data/languages/Instructions/Divide.sinc||GHIDRA||||END|                                                        
 data/languages/Instructions/Float.sinc||GHIDRA||||END|                                                         
-data/languages/Instructions/HighSpeedDivide.sinc||GHIDRA||||END|                                               
-data/languages/Instructions/Load.sinc||GHIDRA||||END|                                                          
+data/languages/Instructions/Load_Store.sinc||GHIDRA||||END|                                                    
 data/languages/Instructions/Logic.sinc||GHIDRA||||END|                                                         
-data/languages/Instructions/Multiply.sinc||GHIDRA||||END|                                                      
-data/languages/Instructions/MultiplyAccumulate.sinc||GHIDRA||||END|                                            
-data/languages/Instructions/Saturated.sinc||GHIDRA||||END|                                                     
 data/languages/Instructions/Special.sinc||GHIDRA||||END|                                                       
-data/languages/Instructions/Store.sinc||GHIDRA||||END|                                                         
-data/manuals/v850.idx||GHIDRA||||END|                                                                          
\ No newline at end of file                                                                                     
+data/manuals/v850.idx||GHIDRA||||END|                                                                          

[esaulenka]

I did some small tests with pcodetest utility. GCC seems to be able to handle 64-values. Please add

        <pentry minsize="5" maxsize="8">
          <addr space="join" piece1="r10" piece2="r11"/>
        </pentry>

in output section.
Unfortunately, I didnt know how to correctly specify this for input values.

[esaulenka]

If you can provide the pcodetest configuration for compiling the pcodetest binaries for the V850, that would help verify the processor

@emteere, @GhidorahRex it is ok, if pcodetest writes some warnings in log file? I cant figure out, why nm reports about undefined symbols r10, r24-r31.

Also I found only 32-bit compiler, and I ran it on virtual machine with old 32-bit CentOS.

[GhidorahRex]

@esaulenka Warnings in pcodetest compilations are not a big deal. They should be evaluated but in most cases they're acceptable.

[Aleckaj]

@GhidorahRex What is the status about the pull request?

[emteere]

@Aleckaj Thanks for submitting the pull request. Hopefully you are well, and I appologize for the world wide interruption. This was in the review pipeline before we became constrained.
I've started doing a secondary review on the V850 so we can move it towards getting merged. We may not be able to do a final merge until we have more time in the office. It is the plan to merge it into master. I'm considering the processor ID, given that others might extend the processor. If this encompasses the whole line, and there isn't any conflicts in other variants, then thats great. I normally strive for a single .sla file if the instructions/addressing/memory map doesn't collide.

I do have some initial queries about the processor that I noticed.

Most processors aren't split up into so many sections. It can make it difficult to find where the information is defined. I see from prior reviews that they have been collapsed into a smaller number. My personal preference is one larger file so the instructions map to the processor manual in alphabetical order so you can see what is missing, or what instructions are special to a processor. More of an organization for a maintenance. I'm using the SleighEditor in Eclipse which works for xrefs on definitions across files, so it isn't so difficult.
When you compile in Eclipse with build.xml, the errors are hyperlinked in the console. Right now there are issues with sub-directories where it doesn't navigate when selected on. That should be fixed.
In the InstructionInfo action on a disassembled instruction, the name of the original file doesn't show up, just a line number. It can be difficult to locate the intruction/subconstructor in many files. This should be fixed in Ghidra as well, but right now only a line number is stored within the parse tree.

I had a few initial questions:
The memory from 0-0x20 is volatile in the .pspec is this correct for the processor?
The register space should probably be dropped back to size 2. Not a big deal, but it really doesn't need that much space.

There are many patterns of the form R1115 & op1115!=0. In general the "!=", ">", "<" type match patterns should be avoided. This can cause the sleigh files to become large especially used on a 5-bit field. I believe the parse tree enumerates all the separate cases. This is a small processor so the .sla file is only 1.5Meg, so it isn't a huge issue, and removing the "!=0"s only dropped the .sla file to 1.3Meg. If you can put the logic into a DestR1115 variable match where the zero register is "_", then the value won't match. Doing it this way can cause issues with other instructions collisions if there aren't enough bits in common with two different instructions, for example if when R1115 is the destination, the instruction is a "TEST" instruction.
Also if the r0 register can never be used as a destination, I see places that don't restrict it's use, and are assigning to a unique (local) in this case which is probably OK. Same things goes for R2731 as a destination.
Lastly if there are more variants that create variant files (little/big/memory size/etc), the extra K adds up.

Do you have the sample PCODEUNIT test binary compiled for the V850? I saw earlier that it had been tested with it. Would also be good to have a .o/obj for the processor as well. I would like to review the decompiler results. What I saw in the above review looked good.

[EgorKin]

Emm, guys I'm really hope finding V850 support on next Ghidra release but can someone look at add imm, reg instruction (and similar) with negative imm values?

And can you fix this dumb "+ -value" to normal "-value"?
Also vote for r0 reg as destination for E1, E2 processor variants.

Thank's.

[emteere]

I didn't run across anything that changed the r0 from a fixed zero register to a register whose value can change.
If code allows assigning to it, and the value assignment is ignored, then no problem leaving it in.
Many processors that assign to the zero register change the nature of the instruction to a test.

There were instructions that specifically forbid assigning to the r0, others that didn't forbid it, but didn't say what happened if you used it as a destination. When forbidden, it is possible when r0 was part of the target the assembly was a different instruction.

[Martmists-GH]

What's preventing this from getting merged right now?

[emteere]

Been reviewed and should be pushed to github today.
There were a few changes necessary to the patterns files.
We need to add some error checks for:
No actions on a pattern
Impossible totalbits and postbits numbers

I've added a comment as to what they really should be.
But Warning or error messages would be good.

[mumbel]

Sorry, my fault on bad pattern file, guess I need to read through that schema again

[emteere]

@mumbel, no worries. I added a ticket to error check for this type of thing when the pattern file is loaded.
When it gets posted to github, I think we can close this one.

[Aleckaj]

@emteere Thank you for your assistance and merge :) I'm sorry that I'm only now replying, my student email has expired and because of that i got no more notifications. I added this processor for my bachelor degree and because of that I don't have that much knowledge about reverse engineering/memory mapping/etc.
Please feel free to use and change everything you want to. I hope my "work" helped you a bit.

Best regards
Alex