Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RISCV support ISRV32/ISRV64 #1401

Merged
merged 101 commits into from
Mar 9, 2019
Merged

RISCV support ISRV32/ISRV64 #1401

merged 101 commits into from
Mar 9, 2019

Conversation

fanfuqiang
Copy link
Contributor

This is based on PR#1198 and LLVM upstream commit b81d715c(Sat Feb 16 18:39:14 2019).
Also referenced the SyestemZ TableGen patchs.
I also add the TableGen patch at capstone/contrib/update_riscv.

porto703 and others added 30 commits July 5, 2018 11:40
… the TableGen files generated from llvm-tblgen. Add Disassembler.h
…ler_getInstruction, and RISCV_getInstruction
…o RISCVGenDisassemblerTables.inc. Add and modified RISCVGenSubtargetInfo.inc. Start creation of RISCVInstPrinter.h
…nor fixes to RISCVDisassembler.c. Working on RISCVInstPrinter
…and test_iter to work w/ the current code strcuture
…ents in struct initializer). Added RISCV tests to test_iter.c
@fanfuqiang
Copy link
Contributor Author

do you want to add the alias register FP to riscv.h?

Need add OPT? I will fix this when the RISCVC PR.
Maybe riscv will change assembler syntax.

@aquynh
Copy link
Collaborator

aquynh commented Mar 8, 2019

do you want to add the alias register FP to riscv.h?

Need add OPT? I will fix this when the RISCVC PR.
Maybe riscv will change assembler syntax.

i mean simply add this to riscv.h:

RISCV_REG_FP = RISCV_REG_S0

if you dont want to, never mind.

@fanfuqiang
Copy link
Contributor Author

fanfuqiang commented Mar 8, 2019

do you want to add the alias register FP to riscv.h?

Need add OPT? I will fix this when the RISCVC PR.
Maybe riscv will change assembler syntax.

i mean simply add this to riscv.h:

RISCV_REG_FP = RISCV_REG_S0

if you dont want to, never mind.

looks like already fixed, will depend on current instruction print s0/fp.
because I changed the RISCV_reg_name use the auto-generated register table.
And the removed reg_name_maps[] is used to contain the old s0/fp.

@aquynh
Copy link
Collaborator

aquynh commented Mar 8, 2019

there is one issue: you are using internal autogen register names, so the public register ID must follow the enum in RISCVGenRegisterInfo.inc.

you can see that cs_reg_name calls RISCV_reg_name(), which calls getRegisterName(). for this to work, the RISC_REG_xxx must match the enum in RISCVGenRegisterInfo.inc. however, these dont match, for example:

    RISCV_REG_F0_64,                                                                  
    RISCV_REG_F1_64,    

meanwhile:

  RISCV_F0_64 = 34,
  RISCV_F1_32 = 35,

you did not have this problem before, when you used reg_name_maps[]

now you can either make them match, or recover that reg_name_maps[]

@aquynh
Copy link
Collaborator

aquynh commented Mar 8, 2019

the mismatch starts from floating point registers, but not before that. this is why we dont have see any issues with test_riscv.c

another thing: given the current approach, we need to have alias registers in riscv.h. otherwise, we have:

0x1098:	srl	ra, sp, gp
	op_count: 3
		operands[0].type: REG = ra
		operands[1].type: REG = sp
		operands[2].type: REG = gp

users may ask where these RA, SP, GP registers are from, as they are nowhere in riscv.h.

(after update riscv.h, please update riscv_const.py by: cd bindings; make)

@aquynh
Copy link
Collaborator

aquynh commented Mar 8, 2019

one more issue is that RISCV_reg_name() should check the regid boundary, and return NULL if the ID is out of range. but right now getRegisterName() does this with assert(RegNo && RegNo < 97 && "Invalid register number!");, which aborts, and should be fixed.

@fanfuqiang
Copy link
Contributor Author

there is one issue: you are using internal autogen register names, so the public register ID must follow the enum in RISCVGenRegisterInfo.inc.

you can see that cs_reg_name calls RISCV_reg_name(), which calls getRegisterName(). for this to work, the RISC_REG_xxx must match the enum in RISCVGenRegisterInfo.inc. however, these dont match, for example:

    RISCV_REG_F0_64,                                                                  
    RISCV_REG_F1_64,    

meanwhile:

  RISCV_F0_64 = 34,
  RISCV_F1_32 = 35,

you did not have this problem before, when you used reg_name_maps[]

now you can either make them match, or recover that reg_name_maps[]

fix consistency with RISCVGenRegisterInfo.inc.

@aquynh
Copy link
Collaborator

aquynh commented Mar 8, 2019

there is one issue: you are using internal autogen register names, so the public register ID must follow the enum in RISCVGenRegisterInfo.inc.
you can see that cs_reg_name calls RISCV_reg_name(), which calls getRegisterName(). for this to work, the RISC_REG_xxx must match the enum in RISCVGenRegisterInfo.inc. however, these dont match, for example:

    RISCV_REG_F0_64,                                                                  
    RISCV_REG_F1_64,    

meanwhile:

  RISCV_F0_64 = 34,
  RISCV_F1_32 = 35,

you did not have this problem before, when you used reg_name_maps[]
now you can either make them match, or recover that reg_name_maps[]

fix consistency with RISCVGenRegisterInfo.inc.

cool, lets finish this with alias registers added to riscv.h (then update binding constants after that)

@aquynh
Copy link
Collaborator

aquynh commented Mar 8, 2019

That is just S0, but how about all other alias registers?

@fanfuqiang
Copy link
Contributor Author

fanfuqiang commented Mar 8, 2019

That is just S0, but how about all other alias registers?

s0 and fp just the public printable register name, I can not understand what is the alias register?
there is only one, just ont s0 and fp for riscv::x8 about this. no more.

@aquynh
Copy link
Collaborator

aquynh commented Mar 8, 2019 via email

@fanfuqiang
Copy link
Contributor Author

For ex, look at the output of test_riscv.c, what is s1 register? There is no such a thing in riscv.h.

I write after the comments.
OK, I add them all.

@aquynh
Copy link
Collaborator

aquynh commented Mar 8, 2019 via email

@aquynh
Copy link
Collaborator

aquynh commented Mar 8, 2019 via email

@fanfuqiang
Copy link
Contributor Author

No, we need the alias that users can use, not just comments. And please uppercase, like "FP", "S0", but not "fp", "s0".

what's the difference???

@aquynh
Copy link
Collaborator

aquynh commented Mar 8, 2019 via email

@aquynh
Copy link
Collaborator

aquynh commented Mar 9, 2019

i will merge. please stay around to help to maintain this work.

@aquynh aquynh merged commit a012f75 into capstone-engine:next Mar 9, 2019
@aquynh
Copy link
Collaborator

aquynh commented Mar 9, 2019

merged, thanks everyone for the amazing effort!

@fanfuqiang
Copy link
Contributor Author

merged, thanks everyone for the amazing effort!

cheers, hope have the opportunity to see you offline.

@aquynh
Copy link
Collaborator

aquynh commented Mar 9, 2019

Hope to see you in BJ, i guess?

@fanfuqiang
Copy link
Contributor Author

Hope to see you in BJ, i guess?
hope so

xanderlent added a commit to xanderlent/capstone that referenced this pull request Feb 4, 2021
@riptl riptl mentioned this pull request Jul 22, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants